Make WordPress Core

Opened 10 years ago

Closed 10 years ago

#34037 closed defect (bug) (fixed)

Setting blog upload limit to 0 doesn't work

Reported by: boonebgorges's profile boonebgorges Owned by: jeremyfelt's profile 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 10 years ago.
34037-media-library-after-34037.diff.png (45.0 KB) - added by jeremyfelt 10 years ago.
34037.diff (1.7 KB) - added by jeremyfelt 10 years ago.

Download all attachments as: .zip

Change History (16)

#1 @jorbin
10 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
10 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 10 years ago by azaozz (previous) (diff)

#3 @jeremyfelt
10 years ago

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

#4 @jeremyfelt
10 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
10 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
10 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
10 years ago

More history:

#8 @jeremyfelt
10 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
10 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
10 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
10 years ago

#11 @jeremyfelt
10 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
10 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
10 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.