WordPress.org

Make WordPress Core

Opened 4 years ago

Closed 4 years ago

#34037 closed defect (bug) (fixed)

Setting blog upload limit to 0 doesn't work

Reported by: boonebgorges Owned by: jeremyfelt
Milestone: 4.4 Priority: normal
Severity: normal Version:
Component: Media Keywords: has-patch
Focuses: administration, multisite Cc:

Description

There's a check in get_space_allowed() that looks like this:

$space_allowed = get_option( 'blog_upload_space' );
// ...
if ( empty( $space_allowed ) ... ) {
    $space_allowed = 100;
}

This means that setting 'blog_upload_space' to 0 can never work. This seems to be a bug: I'd expect that setting it to 0 would mean that uploads were effectively disabled for that site.

This behavior was previously noted https://core.trac.wordpress.org/ticket/19538#comment:10 and changed [19639], but was later reverted [19652] to avoid breaking existing behavior in a minor release.

Attachments (3)

34037-post-new-media-after-34037.diff.png (16.3 KB) - added by jeremyfelt 4 years ago.
34037-media-library-after-34037.diff.png (45.0 KB) - added by jeremyfelt 4 years ago.
34037.diff (1.7 KB) - added by jeremyfelt 4 years ago.

Download all attachments as: .zip

Change History (16)

#1 @jorbin
4 years ago

In 34606:

Revert change from true to false in multisite upload test

This change was introduced in [34601]. 0 is a special snowflake. The code treats it as if it should fall back to the default, rather than respecting 0 as 0. Essentially, 0 means 100. WAT? This has been discussed previusly in https://core.trac.wordpress.org/ticket/19538#comment:10. This test should return True right now, but the behavior should be discussed and in the future 0 may mean 0.

See #34037
Props jorbin, boonebgorges

#2 @azaozz
4 years ago

Seems the proper solution is to revert r19652. However wondering what changes to the UI will be needed to support this case (uploads are not allowed). Should the "Media" menu and the "Add Media" button above the editor be hidden? Or should they still be shown but with the appropriate error messages? Empty media library looks... sad.

Last edited 4 years ago by azaozz (previous) (diff)

#3 @jeremyfelt
4 years ago

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

#4 @jeremyfelt
4 years ago

In 34898:

Tests: Improve tests for get_space_allowed().

  • Move all tests to a new multisite/getSpaceAllowed.php
  • Store original blog_upload_space values before the class is loaded.
  • Restore these values after each test tear down to avoid pollution.
  • Add a comprehensive set of expectations and results via data provider.

See #34037.

#5 @jeremyfelt
4 years ago

In 34899:

Tests: Improve tests for is_upload_space_available().

  • Move all tests to a new 'multisite/isUploadSpaceAvailable.php'
  • Store original blog_upload_space values before the class is loaded.
  • Restore these values after each test tear down to avoid pollution.

Commit to testing is_upload_space_available() itself by always filtering the amount of space used through pre_get_space_used. This allows us to sanely test without worrying about the local environment. In the future, we may be able to remove some of these tests as get_upload_space_available() tests are built out and reliable.

See #34037.

#6 @jeremyfelt
4 years ago

In 34900:

Tests: Account for non-numeric strings in tests for get_space_allowed().

Provides a specific test for a case where a non-numeric strings are assigned to blog_upload_space at both the site and network levels.

See #34037.

#7 @jeremyfelt
4 years ago

More history:

#8 @jeremyfelt
4 years ago

Once we fix get_space_allowed() so that 0 is a valid return, we then run into upload_is_user_over_quota():

	$space_allowed = get_space_allowed();
	if ( empty( $space_allowed ) || !is_numeric( $space_allowed ) )
		$space_allowed = 10; // Default space allowed is 10 MB

Which could then calculate slightly wrong. More tests needed!

#9 @jeremyfelt
4 years ago

In 34901:

Tests: Improve tests for upload_is_user_over_quota().

  • Move all tests to a new 'multisite/uploadIsUserOverQuota.php'
  • Use get_space_allowed and pre_get_space_used filters to help avoid test pollution.

See #34037.

#10 @jeremyfelt
4 years ago

In 34902:

Tests: Remove static capture of space used and space allowed.

Originally introduced in [34601], this was necessary to avoid test pollution. Now that things have been separated and written to play nicely, we can remove this.

See #34037.

@jeremyfelt
4 years ago

#11 @jeremyfelt
4 years ago

  • Keywords has-patch added
  • Status changed from assigned to reviewing

34037.diff modifies get_uploads_space() to only accept a numeric value and drops the check for empty. It also adjusts the existing tests accordingly.

While the matching UI isn't perfect, it does exist for other cases where uploads have been restricted. The screenshots attached to this ticket show the current state when attempting to add media while space allowed is 0. We can probably improve this experience with a nicer message in another ticket.

#12 @jeremyfelt
4 years ago

In 35016:

MS: Allow for a blog_upload_space setting of 0 to restrict uploads.

Previously, an value matching empty() would have been bypassed in favor of the default setting for 100MB.

Related #19538, r19639, r19652, where we saw the bug, fixed the bug, and then unfixed the bug so that it was not a surprise in a point release.

See #34037.

#13 @jeremyfelt
4 years ago

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

In 35017:

MS: Handle the possibility of 0 when checking a user's upload quota.

Upload space of 0 is now more possible via r35016 and should be respected rather than modified to a default of 10MB.

Fixes #34037.

Note: See TracTickets for help on using tickets.