WordPress.org

Make WordPress Core

Opened 3 years ago

Closed 3 years ago

#21181 closed enhancement (fixed)

simplify and add filter to is_upload_space_available

Reported by: jkudish Owned by: westi
Milestone: 3.5 Priority: normal
Severity: normal Version: 3.4.1
Component: Multisite Keywords: needs-unit-tests close
Focuses: Cc:

Description

add a filter to the return value of is_upload_space_available in instances (such as on WordPress.com) when upload_space_check_disabled is set to true but an upload quota still exists.

Attachments (5)

21181-1.diff (660 bytes) - added by jkudish 3 years ago.
21181.2.patch (2.3 KB) - added by dllh 3 years ago.
A more complete approach
21181.patch (2.9 KB) - added by dllh 3 years ago.
A more complete solution
21181.rework.diff (6.2 KB) - added by westi 3 years ago.
A much bigger rework of the upload space tracking code.
21181.3.patch (2.1 KB) - added by SergeyBiryukov 3 years ago.

Download all attachments as: .zip

Change History (30)

@jkudish3 years ago

comment:1 follow-up: @nacin3 years ago

I'm confused. Can you provide a use case for that?

comment:2 in reply to: ↑ 1 @jkudish3 years ago

Replying to nacin:

I'm confused. Can you provide a use case for that?

We want to improve the experience of uploading media when you’re over quota. Part of the task is omitting the upload form for users who are over quota. In core, when the quota is exceeded the form isn’t displayed

However, on WordPress.com, is_upload_space_available() always returns true, because the upload_space_check_disabled option is set to true. All of these functions that try to calculate filesystem disk usage don't work on dot com as we check/store that information differently.

tl;dr: the purpose of the filter is to be able to hide the upload form while keeping upload_space_check_disabled set to true.

comment:3 @scribu3 years ago

Related: #21179

comment:4 follow-up: @westi3 years ago

I think a better solution here is to add a pre_ filter into get_upload_space_available so that a replacement method of tracking used upload space can easily be plugged into core.

This should probably happen after the upload_space_check_disabled check so as to give that feature continuing functionality.

comment:5 in reply to: ↑ 4 ; follow-up: @dllh3 years ago

  • Cc daryl@… added

Replying to westi:

I think a better solution here is to add a pre_ filter into get_upload_space_available so that a replacement method of tracking used upload space can easily be plugged into core.

21181.patch adds this functionality.

comment:6 in reply to: ↑ 5 @westi3 years ago

Replying to dllh:

Replying to westi:

I think a better solution here is to add a pre_ filter into get_upload_space_available so that a replacement method of tracking used upload space can easily be plugged into core.

21181.patch adds this functionality.

Looks good.

I think there are some other places where we calculate space used directly instead of calling a function which need reworking to have filtering available too to allow a custom method of measurement.

e.g. display_space_usage and check_upload_size

We need to review anywhere where the option to disable is used and come up with a suitable filter for accounting for space in a different way.

comment:7 @nacin3 years ago

A heads up, in 3.5, I am thinking about going on a tear through these functions while also removing ms-files.php, since blogs.dir is baked into quite a bit of this. Perhaps most of this can just go away.

@dllh3 years ago

A more complete approach

@dllh3 years ago

A more complete solution

comment:8 @dllh3 years ago

21181.patch (replaced the original by accident) includes fixes for other places that call get_dirsize(). I started to implement a filter in get_dirsize() instead of duplicating filters here but decided that wouldn't work, since get_upload_space_available() calls get_dirsize() per file if it gets past the new pre_get_upload_space_available filter. This would cause get_dirsize() to return the filtered version on every call even when we wanted to walk the file tree.

I suppose an alternate approach would be to still filter within get_dirsize() and just remove the filter in get_upload_space_available(). I'm sticking with the more explicit, transparent (if slightly repetitive) solution that calls the blog_space_used filter everywhere needed for now.

@westi3 years ago

A much bigger rework of the upload space tracking code.

comment:9 @westi3 years ago

  • Component changed from General to Multisite
  • Milestone changed from Awaiting Review to 3.5
  • Owner set to westi
  • Status changed from new to accepted

In 21181.rework.diff I have made a more major reworking of the code including:

  • Moving some admin only functions into wp-admin/includes/ms.php from wp-includes/ms-functions.php
  • Reworked the variable naming to be more in line with the Coding Standards
  • Introduced a new get_space_used() function instead of calculating it in multiple places.

Has only had light testing so far.

comment:10 @dllh3 years ago

I've tested 21181.rework.diff a bit and so far no problems.

comment:11 @nacin3 years ago

At the very least, 21181.rework.diff appears to pass WPTestMS::test_upload_is_user_over_quota().

comment:12 @westi3 years ago

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

In [21387]:

Multisite: Rework the upload space usage tracking code so as to be fully pluggable.

  • Moves some admin only functions into wp-admin/includes/ms.php from wp-includes/ms-functions.php
  • Reworked the variable naming to be more in line with the Coding Standards
  • Introduced a new get_space_used() function instead of calculating it in multiple places.

Fixes #21181 props dllh and jkudish for inital work on this.

comment:13 follow-up: @Jayjdk3 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

One of my sites currently use 1.84 MB of space but after this commit, the dashboard says that is using 1930466 MB

1930466 is ~1.84 * 1024 * 1024

The code before used to calculate it:

$used = get_dirsize( BLOGUPLOADDIR ) / 1024 / 1024;

The code used now (wp_dashboard_quota(), wp-admin/includes/dashboard.php line 1100):

$used = get_space_used();

But the get_space_used() function uses get_dirsize( BLOGUPLOADDIR ) without dividing with 1024 / 1024.

@SergeyBiryukov3 years ago

comment:14 in reply to: ↑ 13 @SergeyBiryukov3 years ago

Replying to Jayjdk:

One of my sites currently use 1.84 MB of space but after this commit, the dashboard says that is using 1930466 MB

Confirmed. Moreover, on media-new.php I get "Sorry, you have used all of your storage quota of 10 MB", though I've only uploaded a single 424 KB image.

There's currently an inconsistency between the expected and actual results:

  • get_space_allowed() returns megabytes.
  • get_space_used() returns bytes.
  • wp_dashboard_quota(), get_upload_space_available(), upload_is_user_over_quota(), and display_space_usage() expect get_space_used() to return megabytes.

21181.3.patch fixes the inconsistency and adds missing PHPDocs.

P.S. display_space_usage() was introduced in mu:1069 (MU 1.3) and altered in mu:1071, but is unused since mu:1272 (MU 1.5.1) and can probably be deprecated.

comment:15 @westi3 years ago

I failed to add a See so here it is for reference:

Multisite: Fix the new get_space_used() function to correctly calculate Megabytes used and update some phpdoc. Props SergeyBiryukov.

Version 0, edited 3 years ago by westi (next)

comment:16 @westi3 years ago

  • Keywords needs-unit-tests added

We also need to add some more unit tests for this functionality to catch things like the stupid bug I introduced.

comment:17 @nacin3 years ago

  • Keywords has-patch removed

Looks like this only needs tests.

comment:18 @nacin3 years ago

  • Keywords close added

comment:19 @jkudish3 years ago

See #UT132, I will work on attaching a patch with some unit tests there shortly.

comment:20 follow-up: @jkudish3 years ago

Took a closer look at this and it seems to me like this existing test covers our ground pretty well. Am I missing something?

comment:21 in reply to: ↑ 20 ; follow-up: @SergeyBiryukov3 years ago

Replying to jkudish:

Took a closer look at this and it seems to me like this existing test covers our ground pretty well. Am I missing something?

When [21387] introduced an inconsistency between the expected and actual results, the test should have indicated that.

comment:22 in reply to: ↑ 21 ; follow-up: @jkudish3 years ago

Replying to SergeyBiryukov:

Replying to jkudish:

Took a closer look at this and it seems to me like this existing test covers our ground pretty well. Am I missing something?

When [21387] introduced an inconsistency between the expected and actual results, the test should have indicated that.

I agree. However, could it be that the test was skipped?

comment:23 in reply to: ↑ 22 @SergeyBiryukov3 years ago

Replying to jkudish:

I agree. However, could it be that the test was skipped?

I guess not. Once the ticket is marked as fixed, the associated tests are no longer skipped by default.

Looks like the test covers upload_is_user_over_quota(), is_upload_space_available(), and get_space_allowed(). We should also add some assertions for get_space_used(), if possible.

comment:24 @jkudish3 years ago

The existing test gets skipped when I run the test suite with the following error:

This test is broken when blogs.dir does not exist.

This is as intended, but even when I add a blogs.dir folder, it gets skipped. Maybe it's a case of me doing_it_wrong() in which case I'd appreciate a nudge in the right direction. Anyways, I have a suspicion that this may be why the error in [21387] never got caught, as testing upload_is_user_over_quota() should also in theory test get_space_used().

I still think having a separate test for get_space_used() is a good idea, but I am wondering what the best way to test it is. As is, it will return 0, since there's nothing in the upload directory. I am thinking uploading a temporary file in the test, calculating it's size, making sure get_space_used() matches it and then deleting the file, is what the test should do. What do you think?

comment:25 @ryan3 years ago

  • Resolution set to fixed
  • Status changed from reopened to closed
Note: See TracTickets for help on using tickets.