WordPress.org

Make WordPress Core

#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 22 months ago.
21181.2.patch (2.3 KB) - added by dllh 22 months ago.
A more complete approach
21181.patch (2.9 KB) - added by dllh 22 months ago.
A more complete solution
21181.rework.diff (6.2 KB) - added by westi 22 months ago.
A much bigger rework of the upload space tracking code.
21181.3.patch (2.1 KB) - added by SergeyBiryukov 21 months ago.

Download all attachments as: .zip

Change History (30)

jkudish22 months ago

comment:1 follow-up: nacin22 months ago

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

comment:2 in reply to: ↑ 1 jkudish22 months 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 scribu22 months ago

Related: #21179

comment:4 follow-up: westi22 months 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: dllh22 months 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 westi22 months 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 nacin22 months 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.

dllh22 months ago

A more complete approach

dllh22 months ago

A more complete solution

comment:8 dllh22 months 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.

westi22 months ago

A much bigger rework of the upload space tracking code.

comment:9 westi22 months 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 dllh22 months ago

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

comment:11 nacin22 months ago

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

comment:12 westi21 months 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: Jayjdk21 months 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.

SergeyBiryukov21 months ago

comment:14 in reply to: ↑ 13 SergeyBiryukov21 months 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 westi21 months ago

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

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

Last edited 21 months ago by westi (previous) (diff)

comment:16 westi21 months 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 nacin19 months ago

  • Keywords has-patch removed

Looks like this only needs tests.

comment:18 nacin19 months ago

  • Keywords close added

comment:19 jkudish19 months ago

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

comment:20 follow-up: jkudish19 months 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: SergeyBiryukov19 months 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: jkudish19 months 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 SergeyBiryukov19 months 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 jkudish19 months 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 ryan18 months ago

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