simplify and add filter to is_upload_space_available

[jkudish]

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.

[nacin]

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

[jkudish]

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.

[scribu]

Related: #21179

[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.

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

[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.

[westi]

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.

[nacin]

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.

[dllh]

A more complete approach

[dllh]

A more complete solution

[dllh]

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.

[westi]

A much bigger rework of the upload space tracking code.

[westi]

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.

[dllh]

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

[nacin]

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

[westi]

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.

[Jayjdk]

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.

[SergeyBiryukov]

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.

[westi]

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.

[westi]

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

[nacin]

Looks like this only needs tests.

[jkudish]

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

[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?

[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.

[jkudish]

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?

[SergeyBiryukov]

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.

[jkudish]

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?