Make WordPress Core

Opened 5 years ago

Closed 5 years ago

Last modified 5 years ago

#46707 closed defect (bug) (fixed)

Site health: notify the user while gathering site data

Reported by: azaozz's profile azaozz Owned by: azaozz's profile azaozz
Milestone: 5.2 Priority: normal
Severity: normal Version:
Component: Site Health Keywords: site-health has-patch needs-testing needs-design-feedback
Focuses: ui, performance Cc:

Description

Follow-up from #46645.

The directory size calculation is typically very slow. On a "standard" install with about 30 plugins and themes, and several hundreds uploads it may take 10 - 15 seconds. On installs with a great number of files it times out after 30 seconds.

During that time the page is half-loaded and there aren't any notifications. Ideally we should show the data that is "fast to get" and ask the users to wait for the size calculations to complete.

Attachments (11)

46707.diff (19.6 KB) - added by azaozz 5 years ago.
comment_12_example.jpg (222.7 KB) - added by xkon 5 years ago.
46707.1.diff (19.7 KB) - added by azaozz 5 years ago.
46707.2.diff (20.0 KB) - added by xkon 5 years ago.
46707.3.diff (6.4 KB) - added by azaozz 5 years ago.
title attribute on the spinner.png (64.1 KB) - added by afercia 5 years ago.
Title attribute on the spinner icon.
46707.4.diff (9.7 KB) - added by azaozz 5 years ago.
dirs-and-sizes.png (28.0 KB) - added by azaozz 5 years ago.
With 46707.4.diff.
folder_sizes_example.jpg (131.5 KB) - added by xkon 5 years ago.
46707.5.diff (9.8 KB) - added by azaozz 5 years ago.
46707.5.png (44.6 KB) - added by azaozz 5 years ago.

Download all attachments as: .zip

Change History (58)

#1 @azaozz
5 years ago

  • Focuses ui performance added

#2 @azaozz
5 years ago

This depends on the fixes in #46645. Will make a patch as soon as they are committed.

Testing the approaches that were suggested by @afercia and @xkon, seems it will be enough to "ajaxify" only the dir size calculations. Gathering the rest of the data performs well.

Last edited 5 years ago by azaozz (previous) (diff)

#3 @SergeyBiryukov
5 years ago

  • Component changed from General to Administration

#4 @afercia
5 years ago

  • Keywords site-health added

#5 @Clorith
5 years ago

  • Keywords needs-patch added

Did you have a plan of attack for this @azaozz? Just so we don't lose track on this as the dependency has gone in now. If not I'll have a look at it over the weekend.

#6 @azaozz
5 years ago

  • Keywords 2nd-opinion added

Yeah, was thinking we can do an AJAX request to get the dir sizes as soon as the page loads. That would make it much faster.

But then, what's the real debug value of knowing these dir sizes?

  • We know WP's size anyways, not even sure we need to get that.
  • Knowing the /plugins and /themes dir sizes tells us... What exactly? Perhaps breaking that into individual plugins and themes sizes may be a bit more useful, but... Don't really see this as essential info.
  • Knowing the /uploads dir size perhaps may be useful in some limited fashion.

Then the size we calculate is not exactly the "size on disk", so the numbers are off. A more comprehensive size can be seen in the hosting control panel. I'm even thinking we should mention that somewhere there.

In these terms I'm thinking we should either stop showing the sizes, or move them to another "section" and load them on demand (AJAX request when the user clicks a button, etc.).

Last edited 5 years ago by azaozz (previous) (diff)

#7 follow-up: @xkon
5 years ago

Imho it's exactly a feature that can easily fill a gap on some occasions ( rare yes, but it also saves 1 extra question back & forth with users ). Various general metrics are always good to have in my opinion even if rarely needed, for example I would also like to know how many files a website has in it's Media Library ( but that's for another ticket :D ).

I've seen occasions for example where a user was given a WordPress installation and SSH access only from their host, so they had no idea even were to start looking for information like this.

#8 in reply to: ↑ 7 @azaozz
5 years ago

Replying to xkon:

Imho it's exactly a feature that can easily fill a gap

Yeah, you're right. It is more of a "curiosity" thing than a "real" debug info. The fact that the total size is a bit incorrect (compared to the size the hosting company sees as "used") doesn't really matter that much.

I would also like to know how many files a website has

Yep, was just thinking the same. Would be quite interesting to know how many files there actually are. Since we loop through them anyway, why not count them too. Should be quite easy :)

#9 @Clorith
5 years ago

On shared hosts you have limited space, and many issues with installs failing, updates failing etc are as likely to be caused by your account having exceeded or hit the limits of your install. It's a very common debugging value to provide.

I'd say file count is... not really something that comes up in a debug relation? I'd prefer we stick to that for now, to limit the scope creep in the initial iteration if anything, and instead look at doing that if it's something that legitimately crops up as a need.

@azaozz
5 years ago

#10 @azaozz
5 years ago

  • Keywords has-patch needs-testing added; needs-patch 2nd-opinion removed

In 46707.diff:

  • "Ajaxify" fetching the dir sizes and add them to the "Directories and Sizes" section and the clipboard data.
  • Move the sizes calculation to WP_Debug_Data::get_sizes().
  • Show a notice while waiting for the ajax call, then swap it with a "success" or "errors" notice when done fetching.
  • Add wp.a11y.speak() to read the appropriate notice.

@Clorith @afercia could you test please :) Ideally this should go in before the soft strings freeze tomorrow...

Also, any text suggestions or corrections?

#11 follow-up: @afercia
5 years ago

@azaozz great job :) I can't follow everything but I've tested a bit and noticed a few things:

1
I have lots of plugins (some of them with node_modules directories) so in my case it's expected to timeout. However, it seems it timeouts before 30 seconds in ~22 seconds. Is this expected?

2
Pre-existing issue, just a bit more evident now. If I'm not wrong, the dirsize calculation is cached for one hour. This might be confusing for users when they know their directories size has changed and they will still see the previous value. For example:

  • I've moved out all my plugins from the plugins directories, except hello dolly and index.php
  • ran the calculation, got: Plugins Directory Size 22.23 KB
  • moved back all my plugins into the plugins directory
  • ran again the calculation
  • got the cached result: Plugins Directory Size 22.23 KB

This may happen any time users add or remove plugins (and also for themes, attachments, db). Not a blocker I guess but maybe users should be informed these size values are cached. Optionally, maybe consider to delete the transient when plugins / themes are installed / deleted. Also, for uploads / attachments I see the dirsize_cache transient gets deleted but only if multisite. As said, this part is pre-existing code but maybe worth considering some improvements for the future?

3
When the dirsize values are cached, the results load almost immediately. In this case, I'd consider to not display and speak the message "Successfully calculated directory sizes."

4
Minor: I'd consider to place the spinner on the right:

  • for consistency: I think spinners in WordPress are almost always on the right
  • to avoid the text "jumps" when the spinner disappears and new text appears

5
Minor: jshint isn't happy with a couple extra commas:

   src/js/_enqueues/admin/site-health.js
    215 |            _wpnonce: SiteHealth.nonce.site_status_result,
                                                                  ^ Extra comma. (it breaks older versions of IE)
    222 |            dataType: 'json',
                                     ^ Extra comma. (it breaks older versions of IE)

I guess this rule should be removed as WordPress doesn't support old IE versions any longer, but for now it will error.

#12 follow-up: @xkon
5 years ago

This is awesome! Thank you @azaozz .

Not sure how much time we have for this but just a note for future maybe, the "Successfully calculated directory sizes." staying there after whole intro text doesn't make sense ( if you actually miss the loading part ). Especially on small installs or cached since it shows pretty much immediately as @afercia says.

Maybe we could move the spinner to the Accordion and simply use "Calculating..." instead of "Not calculated" on the values that are not yet gathered?

#13 in reply to: ↑ 11 @azaozz
5 years ago

Replying to afercia:

I have lots of plugins (some of them with node_modules directories) so in my case it's expected to timeout. However, it seems it timeouts before 30 seconds in ~22 seconds. Is this expected?

Yes, currently the timeout is capped at ~20 seconds. Thinking to increase that to 25.

...the dirsize calculation is cached for one hour. This might be confusing for users when they know their directories size has changed and they will still see the previous value.

Yeah, was thinking about that too. The users may try deleting unused themes and plugins and then re-running this so check the size again. As this is ajax based now, we can calculate sizes without caching them. For that we'll probably need to add another param to get_dirsize() or use recurse_dirsize() directly.

When the dirsize values are cached, the results load almost immediately. In this case, I'd consider to not display and speak the message "Successfully calculated directory sizes."

Yeah, looking at it again all these calculations are perhaps too "prominent". Thinking what @xkon suggests above would probably be better.

Minor: jshint isn't happy with a couple extra commas:
...
I guess this rule should be removed as WordPress doesn't support old IE versions any longer, but for now it will error.

Yep, no point of that rule any more. It's against the updated JS coding standards too.

#14 in reply to: ↑ 12 @azaozz
5 years ago

Replying to xkon:

Not sure how much time we have for this but just a note for future maybe, the "Successfully calculated directory sizes." staying there after whole intro text doesn't make sense ( if you actually miss the loading part ).

Agreed. If the calculations are fast, like 3-4 seconds, all these extra messages are pretty pointless.

Maybe we could move the spinner to the Accordion and simply use "Calculating..." instead of "Not calculated" on the values that are not yet gathered?

That's pretty close to what I tested first... Perhaps should go back to it, and just display a title on the spinner. This will address afercia's concern above too.

Patch coming up.

@azaozz
5 years ago

#15 @azaozz
5 years ago

In 46707.1.diff:

  • Implement the above suggestions and fix the errors.
  • Fix RecalculateProgression() so it can be used on the info tab.

@Clorith @xkon @afercia thinking this is ready for 5.2, test again please :)

#16 follow-up: @xkon
5 years ago

Thank you @azaozz ! Just finished checking out 46707.1.diff and this looks really good to me. Not having a cached value isn't actually bad at all to be honest especially if the timeout is close to 30 or under, users are most likely already "comfortable" waiting until that point from other various functionalities from plugins etc so it doesn't feel awkward every-time you get in there.

Stats wise I'm counting: 29,508 Files, 5,738 Folders, 478.34 MB and finishes in about 18 seconds so that's cool.

One thing I'm seeing and I'm a bit confused is on the totals, I think the calculation is going wrong somewhere. Let me explain with my stats:

My folder when inspected (on Windows) is:

Size:    478MB (501,578,362 bytes)
On disk: 526 MB (551,817,216 bytes).

The Total installation size should be WordPress Directory Size + Database size. It seems though that it adds all of the previous folders again to make the total. I know it was mentioned we're having some difference from actual disk size, but this is x2 on my case at least.

Scan stats:

4.21 MB   - Uploads Directory Size	
8.90 MB   - Themes Directory Size	
407.73 MB - Plugins Directory Size	

478.34 MB - WordPress Directory Size	
720.00 KB - Database size	

899.89 MB - Total installation size	

Not really sure about this but it looked a bit weird that's why I'm mentioning.

Also getting an PHP Notice: Undefined index: path in D:\laragon\www\wp-trunk\src\wp-admin\includes\class-wp-debug-data.php on line 1182 that's the $is_subdir = ( strpos( $all_sizes[ $name ]['path'], ABSPATH ) === 0 ); trying to see if that's on my end also only and might be related to the sizes etc.

Last edited 5 years ago by xkon (previous) (diff)

@xkon
5 years ago

#17 @xkon
5 years ago

46707.2.diff is based on 46707.1.diff but changes the calculation a little bit ( lines 1136 - 1203 ).

Adds an extra value of unformatted_size on the path sizes for internal use of calculations and converts the Total size to use the WordPress Directory Size + Database Size.

My finished calculations now are and look more reasonable ( if I was correct on this :D ):

4.21 MB   - Uploads Directory Size	
8.90 MB   - Themes Directory Size	
407.73 MB - Plugins Directory Size	

478.34 MB - WordPress Directory Size	
720.00 KB - Database size	

478.98 MB - Total installation size	

By removing the is_subdir check, the debug notice from my previous reply is also gone. It was always empty for me so I'm not really sure what was it's purpose and I might be way off on this part but with the new "total" size calculation I don't think it's needed either way.

Could you please give this a go as well just in case I wasn't wrong on this? Nothing else was touched except from the way we calculate the Total.

#18 @afercia
5 years ago

Aside: why the recurse_dirsize() documentation says the returned value is in MB?

@return int|false|null Size in MB if a valid directory. False if not. Null if timeout.

Seems to me it returns bytes.

#19 @afercia
5 years ago

From an UI perspective, 46707.1.diff looks good to me. However, as @xkon noticed, I get the same wrong total size: plugins+themes+uploads are added to the WP size (which already includes them). 46707.2.diff seems to fix it but I'm not sure about the original intent of the $is_subdir check.

In a conversation with @xkon we've also explored a couple more things:

1
If recurse_dirsize() was able to accept multiple directories to exclude, performance could be improved. Right now, the size calculation on the plugins, themes, and uploads directories runs twice:

  • as part of the WP size calculation
  • as individual size calculation for each of them

The former is unnecessary because there's already the need to run the calculation on plugins, themes, and uploads individually. So it could be:

  • run the individual plugins, themes, and uploads calculations
  • run the WP size calculation excluding plugins, themes, and uploads (so this would be much faster)
  • just sum 1 to 2 to get the actual WP size

2
WordPress uses some constants to define KB, MB, GB, etc. and they're based on the 1024 binary value. Worth noting size_format() uses these constants. This used to be the industry standard. However, in recent years a new IEC-SI standard has been adopted by various operating systems and it's based on a value of 1000. As far as I know, Mac OS starting from version 10.6 and Ubuntu starting from version 10.10 switched to 1000. I think Windows still uses 1024.

This doesn't matter so much when testing locally: in this case we should just be aware the values reported by Site Health won't match the ones reported by macOS and various Linux distros. However, it does matter when it comes to hosting.

WordPress sites are hosted somewhere and it's very likely the vast majority of the hosting companies use some flavours of Linux. I guess this should be verified on a per-hosting company basis, but it's very likely their OSes use the new 1000 value. Site Health is primarily meant to collect data for debugging and the sizes data will very likely mismatch with the sizes data on the hosting side.

To give you an idea, here's what I get testing on macOS (reduced example for brevity): the data in the third column is the size reported by the macOS Finder:

Default:

Uploads Directory Size    96.22 MB     100.889.632 bytes 
Themes Directory Size     9.00 MB      9.442.020 bytes
Plugins Directory Size    36.29 MB     38.054.335 bytes 

Changing the WP constants to `1000`:

Uploads Directory Size    100.89 MB    100.889.632 bytes 
Themes Directory Size     9.44 MB      9.442.020 bytes
Plugins Directory Size    38.05 MB     38.054.335 bytes 

As you can see, they match only after changing the related WP constants to 1000. At this point, I'm not sure what is the future of these constants defined in wp_initial_constants() :) That's probably something that should be discussed and addressed separately.

For now, one possible option would be to report these sizes data in bytes: after all these are technical data meant to be reported to a support service or tech-savvy team and don't necessarily need to be in a "human readable" format.

#20 in reply to: ↑ 16 @azaozz
5 years ago

@xkon @afercia Thanks for testing and the reviews! :)

One thing I'm seeing and I'm a bit confused is on the totals, I think the calculation is going wrong somewhere.
...
Also getting a PHP Notice.

Uh, sorry about that, it's a "copy/paste" bug I introduced. Should have been strpos( $attributes['path'].... Fixing.

Adds an extra value of unformatted_size.

Yeah, that may be useful in the future, or perhaps if plugins reuse this (although reusing is not really intended I think).

If recurse_dirsize() was able to accept multiple directories to exclude...

Yeah, was looking into this too. It's really pointless to calculate /plugins, /themes, /uploads directory sizes twice (when they are in the default location). We should be able to make recurse_dirsize() accept a string (one path) or an array of paths.

WordPress uses some constants to define KB, MB, GB, etc. and they're based on the 1024 binary value. Worth noting size_format() uses these constants. This used to be the industry standard. However, in recent years a new IEC-SI standard has been adopted by various operating systems and it's based on a value of 1000. As far as I know, Mac OS starting from version 10.6 and Ubuntu starting from version 10.10 switched to 1000. I think Windows still uses 1024.

Yep, that's why we have these constants, we can change them to the "new standard" whenever it's appropriate. Thinking we need a new ticket for that, and do a bit of investigating what units most hosts use.

BTW the reported size is off not only because of these calculations. Most OS report "size on disk" as the used space. That may be quite different when there are a lot of small files. Typically the smallest "size on disk" is 4kb even when a file is only a few bytes.

#21 @azaozz
5 years ago

Lets fix the $attributes['path'] bug and get that in so it is in beta-3 (string freeze). Will add the improvements/refinements later.

#22 @azaozz
5 years ago

In 45176:

Site health: Load the "Info" tab immediately and notify the user while gathering site data. Changes the Info tab to work similarly to the Status tab: it does separate request to fetch the directories sizes and doesn't "block" the loading of the page.

Props xkon, afercia, Clorith, azaozz.
See #46707.

@azaozz
5 years ago

#23 @azaozz
5 years ago

In 46707.3.diff:

  • Changed recurse_dirsize() to accept an array of excluded paths.
  • Changed so we don't calculate the sizes of dirs in wp-content twice.
  • Added the size in bytes to the "debug" into.

Now the sizes in the copied "site info" look like this:

### wp-paths-sizes ###

uploads_path: WordPress/content/uploads/
uploads_size: 681.68 MB (714788586 bytes)
themes_path: WordPress/content/themes/
current_theme_path: WordPress/content/themes/twentyseventeen
themes_size: 8.90 MB (9333971 bytes)
plugins_path: WordPress/content/plugins/
plugins_size: 823.28 MB (863271905 bytes)
wordpress_path: WordPress/trunk1/build/
wordpress_size: 50.27 MB (52716749 bytes)
database_size: 25.96 MB (27218602 bytes)
total_size: 1.55 GB (1667329813 bytes)

#24 @afercia
5 years ago

Testing 46707.3.diff with a decent amount of plugins (37), it's definitely faster.

  • trunk: ~17 seconds
  • patched: ~10 seconds

Interesting to note: adding a "big" plugin that has lots of files (Jetpack) increases the time by ~2 seconds.

Note: shouldn't the displayed wordpress_size: 50.27 MB be the sum of:
wordpress_size + uploads_size + themes_size + plugins_size?

#25 @afercia
5 years ago

I'd propose to remove the title attribute added on the spinner in [45176]:

title="<?php esc_attr_e( 'Calculating directory sizes. This may take a while&hellip;' ); ?>">

WordPress has been progressively removing many title attributes for the many reasons explained in #24766 and many following tickets. It would be nice to not add new title attributes in core :)

Title attributes are only available to a minority of users. In this specific case, I'm not sure anyone will ever hover the mouse on the spinner to get any useful information from the title attribute (see screenshot attached below).

The accessibility team recommendation is:

  • if the information provided by the title attribute is relevant, then consider to put it in some visible plain text in the page
  • if it's not relevant, it should be removed

@afercia
5 years ago

Title attribute on the spinner icon.

#26 follow-up: @Clorith
5 years ago

Sorry about the late followup here.

This seems to be working really well, I am wondering if we should have made this more accessible to plugins and themes though. I run the Jetpack beta, and they've implemented support for the new debug and test pages already. I notice that some of their debug information collectors are very heavy and added more to the load times than the dirsize counter, the users would benefit from them also using this ajaxified approach I believe.

#27 @azaozz
5 years ago

shouldn't the displayed wordpress_size: 50.27 MB be the sum of: wordpress_size + uploads_size + themes_size + plugins_size?

Yeah, it is a bit unclear what is wordpress_size. We can certainly add up the dir sizes (when wp-content is at the default location) but then what is "total size" for? :)

IMHO it is better as it is now. Separate the three dirs: plugins, themes, uploads, as they are the "variable size" (they may also be outside the WP install dir). Then have "the rest" a.k.a. wordpress_size, and then add it all up in "total size". That way it makes more sense when looking at the numbers too, otherwise the "total" looks wrong :)

I'd propose to remove the title attribute

This is the most "subtle" way to inform the user about what's going on. Alternatively we can show it --somewhere-- and remove it when the ajax request completes, but... Where? :)

As the wait time can be quite substantial, often more than 10 seconds, there should be some explanation on what's going on. We expect the users to wait, we need to tell them so in a nice way :)

#28 in reply to: ↑ 26 @azaozz
5 years ago

Replying to Clorith:

I am wondering if we should have made this more accessible to plugins and themes...

Was looking at that too. This functionality is pretty easy to do, doesn't require any "heavy lifting" etc. Generally there are two ways plugins and themes can take:

  1. We add a "minimal" API for it and they use it. Downside is it may not work well for all cases, so plugins will need to bypass or extend it. Then when we fix something they may need to update the way they use it.
  2. We add some more docs (perhaps) and encourage them to copy the way core works. This will be a bit more work on their end, but would cover all cases, and once done shouldn't need any changes.

Ideally we should "hear" from plugin and theme devs on what works best for them :)

Last edited 5 years ago by azaozz (previous) (diff)

#29 @azaozz
5 years ago

  • Keywords needs-design-feedback added

#30 @xkon
5 years ago

Now that the support is added to exclude the extra folders easily I think it might actually make more sense to have the wordpress_size on it's own.

Would it make more sense maybe if we as well "invert" the list of the folders (basically move the wordpress_size at the top)?

As an example in a "file manager" view we would have:

WordPress
--Uploads
--Themes
--Plugins

Maybe this way it's a little bit more easier to understand that that size "doesn't" include the folders that are mentioned "after the WP folder" ?

I'm ok with either to be honest as both versions ( full files size vs just WP folder without the others included as well ) serve different purposes, so I don't have a trully strong opinion on it :/ .

Last edited 5 years ago by xkon (previous) (diff)

#31 @azaozz
5 years ago

@xkon yeah, good idea! Was just thinking the same as "WordPress Directory" is usually the parent dir, and the rest should be under it.

Also, perhaps needs better label for "total size before DB". Something like "All files"? That should still be just above the DB size (IMHO). Then it would make sense to show the size of all files, add the DB size and have a "Total installation size" at the bottom?

I'll make an "alternative patch" :)

Hmm, actually "All files" is too generic perhaps. Thinking "WordPress size" as a label for all the installed files would be better.

Last edited 5 years ago by azaozz (previous) (diff)

@azaozz
5 years ago

#32 @azaozz
5 years ago

In 46707.4.diff:

  • Add a custom DOM event after the dir sizes request is done. Can be used by plugins to "daisy-chain" more requests.
  • Move "WordPress directory location" at the top as it is the parent dir.
  • Rename "WordPress directory size" to "WordPress size" as it better describes that value. The uploads, themes and plugins directories may be outside ABSPATH.
  • Fix the labels capitalization. All other labels weren't capitalized, just some of the "sizes" labels.

With this patch the copied site info is:

### wp-paths-sizes ###

wordpress_path: WordPress\trunk1\build
uploads_path: WordPress/content/uploads
uploads_size: 681.68 MB (714788586 bytes)
themes_path: WordPress/content/themes
current_theme_path: WordPress/content/themes/twentyseventeen
themes_size: 8.90 MB (9333971 bytes)
plugins_path: WordPress/content/plugins
plugins_size: 131.26 MB (137640114 bytes)
wordpress_size: 872.12 MB (914485075 bytes)
database_size: 25.96 MB (27219046 bytes)
total_size: 898.08 MB (941704121 bytes)

(Note that in this case the plugins, themes and uploads dirs are outside of the WP root dir).

@azaozz
5 years ago

With 46707.4.diff.

#33 @Clorith
5 years ago

Perfect, as we don't have an API for it as is, any implementation made will be a developers own, if they need something more fixed from us we'll surely hear of it post-release and can provide that for a future iteration, I'm happy with this approach.

The event in 46707.4.diff is a great addition, I'm a bit worried about the modified strings in there though. Due to the current release timeline that's not ideal and would be a blocker for the other improvements in the patch, wouldn't it? Perhaps splitting this up so that any actual string changes are in a separate patch/ticket for 5.3?

#34 @xkon
5 years ago

46707.4.diff works really well! Maybe I'm overthinking this but (viewing as somebody that hasn't read the code etc of course) I'm still missing "something" to understand how the current "WordPress size" works since there's always a "size difference" from the previously mentioned folders.

I'm adding a follow up image that adds an extra WordPress directory size right after the WordPress directory location that has the size without the subfolders and an extra Total files size that is the sum of all folders mentioned previously. So when reading in order you get

all folders
total file size
+ db
total installation size

I think this is "as clear" as it can get without having "hidden" numbers with the current list that we show since all folders have it's own size as well and then we just sum the totals.

The similar should be on the debug side of things of course, as it would be helpful I think to identify for example an 1gb file that's roaming somewhere either in ABSPATH or /wp-content etc that are not counted under the other folders :).

--

As an extra note, I'm not sure if the Current theme directory is actually needed in this list since it has no distinct size as well :) . Maybe this could be moved in the Active Theme accordion in a different ticket.

Last edited 5 years ago by xkon (previous) (diff)

#35 @azaozz
5 years ago

I'm a bit worried about the modified strings in there

Yeah, should have noticed the capitalization thing earlier. We are in a "soft" strings freeze so maybe we can still fix it as it doesn't really change the strings or the meaning. On the other hand it will still make these strings "fuzzy" in GlotPress (I think) and will need a translator to "accept" the translation again.

Apart from that, there is only one string "shortening". If we go with what @xkon suggests, we won't need to change it.

I'm still missing "something" to understand how the current "WordPress size" works since there's always a "size difference" from the previously mentioned folders.

Yeah, I see how that may be a bit unclear. What we need there are the three dirs, uploads, plugins, themes, and then another "size" for the whole install root minus these dirs. Perhaps keeping the existing field label "WordPress directory size" and not adding "Total files size" would be better? Then we won't need to change that string.

Thinking that if we have one we don't need the other as it can be easily calculated, i.e. if you want to find the total files size you subtract the DB size from the total install size :)

Going to make another patch with this.

This ticket was mentioned in Slack in #polyglots by azaozz. View the logs.


5 years ago

#37 @azaozz
5 years ago

I'm not sure if the Current theme directory is actually needed in this list since it has no distinct size as well. Maybe this could be moved in the Active Theme (section)?

Yeah, logically the active theme dir is a bit "out of place" there and we don't even calculate its size. Lets move it one section down in the "Active theme" :)

@azaozz
5 years ago

#38 @azaozz
5 years ago

46707.5.diff is based on 46707.4.diff with the following changes:

  • Move "WordPress directory location" and "WordPress directory size" to the top.
  • Move "Theme directory" to the "Active theme" section.

This makes the dirs and sizes section better organized, all paths have a corresponding size, and all sizes are summed at the bottom.

Thinking this is ready to go in, @xkon @Clorith @afercia any last reviews, suggestions, changes? :)

Note: there are a few capitalization changes in translatable strings. That would make them "fuzzy" but they won't need new translation, just to be approved. There is one string change (shortening).

@azaozz
5 years ago

#39 @xkon
5 years ago

I believe this is ready @azaozz. Everything went fine on my end (tested with various folders/files setups again as well) and the folders/sizes make total sense now by getting them in order and showing the actual sizes. Thank you for all the updates!

#40 @azaozz
5 years ago

  • Owner set to azaozz
  • Resolution set to fixed
  • Status changed from new to closed

In 45220:

Site health, info tab:

  • Change recurse_dirsize() to accept an array of excluded paths.
  • Change so we don't calculate the sizes of dirs in wp-content twice.
  • Add the size in bytes to the "debug" into.
  • Add a custom DOM event after the dir sizes request is done. Can be used by plugins to "daisy chain" more requests.
  • Move "WordPress directory location" and "WordPress directory size" to the top in the "Directories and Sizes" section.
  • Move "Theme directory location" to the "Active Theme" section.
  • Fix labels capitalization.

Props xkon, afercia, Clorith, azaozz.
Fixes #46707.

#41 @SergeyBiryukov
5 years ago

In 45221:

PHPCS: Fix WPCS violations in [45220].

See #46707.

#42 @afercia
5 years ago

In 45228:

Site Health: Remove title attribute from the directory sizes spinner.

Amends [45176].
See #46707.

#43 @azaozz
5 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

Don't think removing the title in [45228] is a good thing to do. This largely undermines the purpose of this ticket which is to tell the users why they need to wait for up to 20 seconds.

Now we are left with an audible alert asking the users to wait (and another one when sizes request is complete), but it is only for people that use screen readers. What about the bulk of the rest of the users? Shouldn't we tell them what they are waiting for too?

I agree, HTML titles may not be good in some contexts, but that doesn't mean they are "bad for everything, everywhere". They add pretty much the same functionality like the editor button tooltips for example.

A title/tooltip on the spinner is the most subtle way to let anybody know what's going on and why there is a spinner there. Was actually thinking to move that title to the <button> element or the parent div so it is easier to trigger and more users would see it.

#44 @azaozz
5 years ago

In 45237:

Site health info tab: replace "Not calculated" with "Loading..." in the "Directories and Sizes" section (that's an existing string and wouldn't cause problems for translators).

See #46707.

#45 @azaozz
5 years ago

  • Resolution set to fixed
  • Status changed from reopened to closed

After a discussion in Slack https://wordpress.slack.com/archives/C02RP4X03/p1555525954229400 (thanks @karmatosed and @melchoyce), this seems good as-is for now.

Summary:

Last edited 5 years ago by azaozz (previous) (diff)

#46 @azaozz
5 years ago

In 45241:

Site Health info tab: fix replacing the size strings in the copied info.

Props xkon.
See #46707.

#47 @spacedmonkey
5 years ago

  • Component changed from Administration to Site Health
Note: See TracTickets for help on using tickets.