Make WordPress Core

Opened 4 months ago

Closed 3 months ago

Last modified 2 months ago

#61638 closed defect (bug) (fixed)

Site Health Tool - Directory sizes stuck in loading state in WordPress v6.6 RC3

Reported by: kowsar89's profile kowsar89 Owned by: peterwilsoncc's profile peterwilsoncc
Milestone: 6.6.2 Priority: normal
Severity: normal Version: 6.6
Component: Site Health Keywords: has-patch has-screenshots has-testing-info commit fixed-major dev-reviewed
Focuses: javascript, administration Cc:

Description

In the WordPress Site Health tool, directory sizes are no longer displayed and remain in a Loading… state. Screenshot: https://prnt.sc/psKV31FudpPa

This issue was not present in WordPress v6.5.5 but has appeared in v6.6 RC3.

Attachments (2)

61638-before-patch.png (32.8 KB) - added by shailu25 4 months ago.
Before Patch
61638-after-patch.png (35.8 KB) - added by shailu25 4 months ago.
After Patch

Download all attachments as: .zip

Change History (26)

This ticket was mentioned in Slack in #core by clorith. View the logs.


4 months ago

#2 @Clorith
4 months ago

  • Milestone changed from Awaiting Review to 6.6
  • Version set to trunk

This was introduced with [58299], although technically it could have happened before as well if the circumstances were correct, this is due to a logical flaw in when to send an error state back at wp-admin/includes/class-wp-debug-data.php#L1723-L1737

The newly introduced filesize check for the fonts directory looks for a wp-content/uploads/fonts folder, if this does not exist (which it does not by default), it will return an error state of not available, and with it being the last entry to be checked, it sets $size_total as null; This is also the variable used to determine the overall error state of the size function.

Using the $size_total variable for validation later down in the function was probably a bad idea, but since WordPress would always generate the directories being checked, it never showed up as an issue before the fonts directory (which is conditionally generated) was added to the list of places to check.

This should probably be changed to look for a failure of *all* checks, and not just the last to be ran.

This ticket was mentioned in Slack in #core by clorith. View the logs.


4 months ago

#4 @Clorith
4 months ago

  • Milestone changed from 6.6 to 6.6.1

After some discussions, we've concluded that as the impact is limited (in that this does not affect site visitors, or other areas of the WordPress admin interface), and the problem is resolved whenever the Fonts API is used, along with how close the next release is, that this will be resolved in a minor update after release.

Moving this to the 6.6.1 milestone for tracking, and thank you for testing and discovering this!

This ticket was mentioned in Slack in #core by hellofromtonya. View the logs.


4 months ago

#6 @hellofromTonya
4 months ago

  • Keywords needs-patch added
  • Milestone changed from 6.6.1 to 6.6.2

Tentatively earmarked a 6.6.1 RC for late tomorrow (July 18th).

As a patch isn't ready yet and, as previously noted, the "impact is limited", moving this to 6.6.2.

This ticket was mentioned in PR #7056 on WordPress/wordpress-develop by @Clorith.


4 months ago
#7

  • Keywords has-patch added; needs-patch removed

This introduces an additional check to verify a directory exists before trying to iterate over its sizes.

Although recurse_dirsize will return a false if is_dir() returns a falsey value, it is mixed with other scenarios as well (such as permission checks and similar), by checking this against is_dir before relying on the WordPress core function, the user can be given a clear indication of why a directory has not been included in the calculation, instead of a generic one about permissions which may not be relevant for their setup.

Open to input on potential better wording for the message returned whe na directory does not exist, alternatively if we do not wish to introduce a new string at this time, the first failure scenario text could be reused, although is not 100% accurate

__( 'The size cannot be calculated. The directory is not accessible. Usually caused by invalid permissions.' );

A new ticket should also be created to handle the scenario where a WP_Error is returned by the REST endpoint used to return filesizes, as the JavaScript does not currently know to handle this properly. I'd like to keep the two issues separate, to allow us to fix the directory existence issue as the current cause for this failing fairly consistently, and is a regression, while the lack of proper handling of errors form the endpoint has been around for a few releases now.

Trac ticket: https://core.trac.wordpress.org/ticket/61638

#8 @ironprogrammer
4 months ago

  • Keywords has-screenshots has-testing-info needs-testing added

Thank you for the ticket, @kowsar89, and @Clorith for the patch! This fix resolves the issue in my environment, but I've added needs-testing to see if we can get some more variety in environments.

Test Report

Patch tested: https://github.com/WordPress/wordpress-develop/pull/7056

Steps to Reproduce or Test

  1. Override the font storage location using the `font_dir` filter, setting it to a non-existent location (example mu plugin).
  2. In Site Health navigate to the Info tab and expand the "Directories and Sizes" accordion.
  3. Observe the directory size results.

Expected Results

When reproducing the bug:

  • ❌ The size results display "Loading..." for all directories.

When testing a patch to validate it works as expected:

  • ✅ Size results are displayed for valid directories, with a "does not exist" message for those that are inaccessible.

Environment

  • Hardware: MacBook Pro Apple M1 Pro
  • OS: macOS 14.5
  • Browser: Safari 17.5
  • Server: nginx/1.27.0
  • PHP: 8.3.9
  • MySQL: 8.0.27
  • WordPress: 6.7-alpha-58576-src
  • Theme: twentytwentyfour v1.2
  • Active Plugins:
    • change-fonts-dir-location (mu plugin that sets fonts dir to /wp-content/fonts-not-exist)

Actual Results

Before patch:

  • ❌ Size results for all directories showed "Loading..."

After patch:

  • ✅ All directories except for fonts displayed valid sizes, and the fonts directory showed "The directory does not exist."

Supplemental Artifacts

Before patch:

https://cldup.com/FkqSWIjPwI.png

After patch:

https://cldup.com/aoKg-mdbkY.png

#9 @shailu25
4 months ago

Test Report

Patch Tested: https://github.com/WordPress/wordpress-develop/pull/7056

Environment:

WordPress - 6.6
PHP - 8.2.12
Browser: Firefox 128.0
OS: Windows 10/11
Theme: Twenty Twenty-Four 1.2
MU Plugins: None activated
Plugins: None activated

Steps to Reproduce:

  • In Site Health navigate to the Info tab and expand the Directories and Sizes.
  • The size results display "Loading..." for all directories.🐞

Actual Results:

  • Directories Size results are displayed with a "The directory does not exist." message for those that are inaccessible.✅

Supplemental Artifacts:

  • Added Attachments.
Last edited 4 months ago by shailu25 (previous) (diff)

@shailu25
4 months ago

Before Patch

@shailu25
4 months ago

After Patch

#10 @swissspidy
4 months ago

#61736 was marked as a duplicate.

This ticket was mentioned in PR #7080 on WordPress/wordpress-develop by @narenin.


4 months ago
#11

Trac ticket: https://core.trac.wordpress.org/ticket/61638

Before Applying Patch:

https://github.com/user-attachments/assets/7b7702fd-6766-4dfc-869a-8df31b74e347

After Applying Patch:

https://github.com/user-attachments/assets/1c1dafea-11aa-47f9-aa0a-51dafa8dea8d

#12 @leedxw
4 months ago

#61745 was marked as a duplicate.

#13 @sabernhardt
3 months ago

#61791 was marked as a duplicate.

#14 @markussss
3 months ago

I am not sure if this is directly related and sorry, I couldn't read the entire thread yet. But the data that is shown in the "Directories and Sizes" in "Site Health" is also not acurate or up to date. This was already before WordPress 6.6 though. That means, from my experience to get updated data you must delete the transient "dirsize_cache".

I am usually not commenting on trac, so I don't know the rules in here.

@peterwilsoncc commented on PR #7056:


3 months ago
#15

This tests well and resolves the issue of being stuck in a loading state.

https://github.com/user-attachments/assets/f854f1e5-13b5-4bd9-9f1d-809bdbdb7311

To test I simply deleted the uploads directory and noticed that it was created along with the year and month directories when loading the site health screen, eg uploads/20204/08 was created. This is due to the use of wp_upload_dir() rather than wp_get_upload_dir().

Now that the use case of a non-existent directory is handled, it might be worth adding a follow up ticket to avoid creating the directory when it's not needed.

#16 @hellofromTonya
3 months ago

  • Keywords commit added; needs-testing removed

Patch: https://github.com/WordPress/wordpress-develop/pull/7056

With multiple test reports, removing the needs-testing keyword.

With patch approval and committer positive test results, marking it for commit consideration.

#17 @peterwilsoncc
3 months ago

@markussss It's not directly related to this issue so would could you please open another ticket so folks can be sure the issue doesn't go missing when the fix for this ticket is committed.

#18 @peterwilsoncc
3 months ago

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

In 58884:

Site Health: Check if directories exist before checking size.

Prevents the Site Health Debug tab from stalling when reporting directory sizes if the directory does not exist.

Props clorith, aristath, narenin, kowsar89, hellofromTonya, ironprogrammer, shailu25.
Fixes #61638.

#19 @peterwilsoncc
3 months ago

  • Keywords fixed-major added
  • Resolution fixed deleted
  • Status changed from closed to reopened

Reopening for backport consideration.

#22 @hellofromTonya
3 months ago

  • Keywords dev-reviewed added

Tested [58884] on the 6.6 branch. It resolves the issue.

LGTM for backport to the 6.6. branch. I'll backport it shortly.

#23 @hellofromTonya
3 months ago

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

In 58891:

Site Health: Check if directories exist before checking size.

Prevents the Site Health Debug tab from stalling when reporting directory sizes if the directory does not exist.

Reviewed by hellofromTonya.
Merges [58884] to the 6.6 branch.

Props clorith, aristath, narenin, kowsar89, hellofromTonya, ironprogrammer, shailu25.
Fixes #61638.

This ticket was mentioned in Slack in #core by hellofromtonya. View the logs.


2 months ago

Note: See TracTickets for help on using tickets.