WordPress.org

Make WordPress Core

Opened 3 years ago

Closed 3 years ago

Last modified 3 years ago

#30202 closed defect (bug) (fixed)

In multisite, the "storage space used" is incorrectly calculated for the main blog/site.

Reported by: rebekahickey Owned by: jeremyfelt
Milestone: 4.3 Priority: normal
Severity: normal Version: 4.0
Component: Networks and Sites Keywords: has-patch
Focuses: multisite Cc:

Description

Recently identified an issue in a few multisite installs where the "space used" that displays on the dashboard of the main site is calculating everything in /wp-content/uploads/, including the subsite uploads in /wp-content/uploads/sites/. For instance, the main site has only 4MB in its Media Library, but the dashboard shows that it's exceeded the storage limit of 150MB and prevents users from uploading files. The additional storage used corresponds with the files uploaded to the network's subsites in /wp-content/uploads/sites/.

Notes:

  • This is not an issue on other multisite instances that are using the /blogs.dir/ path for uploads.
  • Organize uploads by year/month is disabled on all instances.

Attachments (6)

30202.diff (2.2 KB) - added by earnjam 3 years ago.
30202.tests.diff (1.6 KB) - added by earnjam 3 years ago.
30202.1.diff (3.2 KB) - added by earnjam 3 years ago.
30202.2.diff (5.4 KB) - added by wonderboymusic 3 years ago.
30202.3.diff (6.4 KB) - added by jeremyfelt 3 years ago.
30202.4.diff (5.1 KB) - added by jeremyfelt 3 years ago.

Download all attachments as: .zip

Change History (25)

#1 @rebekahickey
3 years ago

  • Summary changed from In multisite on the main blog/site, the space used includes all files in upload directory. to In multisite, the "storage space used" is incorrectly calculated for the main blog/site.

@earnjam
3 years ago

#2 @earnjam
3 years ago

  • Keywords has-patch added

In 30202.diff

  • Added optional $exclude parameter to get_dirsize() to allow the exclusion of a directory from the total
  • Added optional $exclude parameter to recurse_dirsize() to allow the exclusion of a directory from the total
  • In get_space_used() added check for is_main_site(), and if true, pass the uploads/sites sub-directory as the exclude value to get_dirsize() so that we are only counting the files for the main site from the main site dashboard.

I also cleaned up the individual edited lines to match the coding standards.

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


3 years ago

This ticket was mentioned in Slack in #core-multisite by earnjam. View the logs.


3 years ago

#5 @jeremyfelt
3 years ago

  • Milestone changed from Awaiting Review to 4.2

Thanks for the ticket @rebekahickey!

@earnjam is working on some tests for the above patch as well.

This ticket was mentioned in Slack in #core-multisite by paaljoachim. View the logs.


3 years ago

#7 @jeremyfelt
3 years ago

  • Keywords needs-unit-tests added
  • Milestone changed from 4.2 to Future Release

We should get some unit tests on this before it goes in. Overall, it's a relatively low impact issue (main site only), though very annoying for anyone experiencing it. Let's go for 4.3.

#8 follow-up: @earnjam
3 years ago

Just thought about a scenario here...since we cache the return from get_dirsize() as a transient, it's possible that if it was called directly outside of core with the new exclude parameter passed to it, it would cache that value instead of the expected core usage.

Not sure the best way to prevent that.

Perhaps don't cache the value if the exclude parameter is passed to it unless it is for the main site uploads directory?

@earnjam
3 years ago

@earnjam
3 years ago

#9 @earnjam
3 years ago

30202.tests.diff is some tests for get_space_used()

30202.1.diff adds in some additional checks to prevent caching or serving a cached value when the exclude parameter is passed unless it is on the main site. This way we don't end up with incorrect values.

#10 @earnjam
3 years ago

Ah, the diff on 320202.1.diff is kinda messy to read. I probably shouldn't have made the whitespace changes inside the get_dirsize() and recurse_dirsize() functions. I'll add another patch with just the necessary functional changes if needed.

#11 @jeremyfelt
3 years ago

  • Milestone changed from Future Release to 4.3

#12 @wonderboymusic
3 years ago

  • Keywords needs-refresh added; needs-unit-tests removed

30202.2.diff is a refresh but 4 unit tests fail. This sucks because it is hard to toggle the value for blog_upload_space and then reset it to the proper value between tests while taking into account attachments from src install.

#13 @obenland
3 years ago

  • Owner set to jeremyfelt
  • Status changed from new to assigned

Can we get this finished before beta2 or should we punt?

This ticket was mentioned in Slack in #core-multisite by jeremyfelt. View the logs.


3 years ago

#15 @jeremyfelt
3 years ago

I'm going to take one more close look at this in the next day or so. That testing is tough is a worry, but we'll figure out something. :)

@jeremyfelt
3 years ago

#16 in reply to: ↑ 8 ; follow-up: @jeremyfelt
3 years ago

  • Keywords needs-refresh removed

Replying to earnjam:

Just thought about a scenario here...since we cache the return from get_dirsize() as a transient, it's possible that if it was called directly outside of core with the new exclude parameter passed to it, it would cache that value instead of the expected core usage.

Not sure the best way to prevent that.

Perhaps don't cache the value if the exclude parameter is passed to it unless it is for the main site uploads directory?

I went with this as well in the latest patch, but I'm thinking we may just want to leave it alone. get_dirsize() has always returned the cached value when requested, so it's a very specific function and use for anything else would lead to strange results in general. If we want to make this more accessible to developers, there's probably a better way to do it.

Replying to wonderboymusic:

30202.2.diff​ is a refresh but 4 unit tests fail. This sucks because it is hard to toggle the value for blog_upload_space and then reset it to the proper value between tests while taking into account attachments from src install.

I've gotten around this in 30202.3.diff.

  • Moved the tests to their own file to match the pattern we've been using elsewhere recently.
  • New sites are created until it is detected that we have an empty uploads directory for that site.
  • We then know it's safe to remove the folder and its contents entirely when the test is complete.
  • The main site can be handled directly in the individual test by checking space used before any new site is created.

I'm going to think through this a bit more and make some of the transient changes. Should be close.

#17 in reply to: ↑ 16 @jeremyfelt
3 years ago

Replying to jeremyfelt:

get_dirsize() ..... If we want to make this more accessible to developers, there's probably a better way to do it.

I think the right answer now is to not add $exclude to get_dirsize() and allow it to function as normal. We can then detect is_main_site() there and pass exclude to recurse_dirsize() if needed. If a developer wanted to retrieve the size of a directory while excluding another, recurse_dirsize() would be available, though without caching.

If we add $exclude to get_dirsize(), it should go hand in hand with #19879.

@jeremyfelt
3 years ago

#18 @jeremyfelt
3 years ago

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

In 33184:

Exclude individual site directories when calculating space used for the main site.

  • Add an $exclude parameter to recurse_dirsize().
  • Use this parameter in get_dirsize() to exclude /sites when on the main site.
  • Add tests for main site and switched site.

Props @earnjam, @jeremyfelt.
Fixes #30202.

#19 @jeremyfelt
3 years ago

In 33185:

Add explicit testing for pre_get_space_used() filter.

See #30202.

Note: See TracTickets for help on using tickets.