REST API: Attachments controller should respect "Max upload file size" and "Site upload space" in multisite

[danielbachhuber]

When the REST API is in use on WordPress multisite, the WP_REST_Attachments_Controller should respect the "Max upload file size" and "Site upload space" site options.

If an uploaded file fails either check, then an error should be returned and WordPress shouldn't save the file.

Related ​https://github.com/WordPress/gutenberg/issues/6154

[flixos90]

Working on this was quite terrible, but at least I was able to figure out the sources of the issue, and 43751.diff​ is "working". Thoughts:

• The logic to check both these things with an upload is in check_upload_size().
• This function is hooked into wp_handle_upload_prefilter, but not into wp_handle_sideload_prefilter. That is one problem, because depending on how you upload the file via the REST API, it may use the latter.
• Related to that, in unit tests, we need to hook the function into yet another filter, wp_handle_mock_upload_filter because the REST API uses that with tests and $_FILES leveraged.
• The worst part about all of this (and the second problem) is that check_upload_size() has a terrible condition that it may just wp_die() under certain circumstances. I added the condition of a REST_REQUEST in there, so that it no longer dies during a REST API request. However, since that constant cannot be set during tests, I included a hack for unit tests in the WP_REST_Attachments_Controller that set $_POST['html-upload'], just so that it doesn't die. I'd love for this to not be necessary, therefore I vote for reopening #42061 and introducing that function, if only for the sake of unit tests. If that gets available, this hack would no longer be necessary.

[danielbachhuber]

Thanks for your work on this, @flixos90.

In 43751.2.diff, I've:

• Internalized check_upload_size() to WP_REST_Attachments_Controller so we can avoid the legacy code paths.
• Updated WP_REST_Attachments_Controller->check_upload_size() to return descriptive WP_Error objects instead of monkeying about with the $files data.
• Updated the unit tests accordingly.

I think it'd be great to land this in 4.9.8.

[pento]

@danielbachhuber: Looks like the unit tests are failing as a result of [43437]. 🙂

[danielbachhuber]

@pento The tests pass locally for me. I think you forgot to run grunt build.

$ grunt patch:https://core.trac.wordpress.org/attachment/ticket/43751/43751.2.diff
$ grunt build
$ phpunit --group 43751 -c tests/phpunit/multisite.xml
Installing...
Installing network...
Running as multisite...
Not running ajax tests. To execute these, use --group ajax.
Not running ms-files tests. To execute these, use --group ms-files.
Not running external-http tests. To execute these, use --group external-http.
PHPUnit 5.7.22 by Sebastian Bergmann and contributors.

....                                                                4 / 4 (100%)

Time: 3.01 seconds, Memory: 82.00MB

OK (4 tests, 16 assertions)

[pento]

🙃

[pento]

In 43462:

REST API: Attachments controller should respect upload limits.

When the REST API is in use on WordPress multisite, the WP_REST_Attachments_Controller should respect the "Max upload file size" and "Site upload space" site options.

Props flixos90, danielbachhuber.
Fixes #43751.

[pento]

Reopen for backport.

[SergeyBiryukov]

In 43489:

REST API: Attachments controller should respect upload limits.

When the REST API is in use on WordPress multisite, the WP_REST_Attachments_Controller should respect the "Max upload file size" and "Site upload space" site options.

Props flixos90, danielbachhuber.
Merges [43462] to the 4.9 branch.
Fixes #43751.

[pedromendonca]

@pento shouldn't the variable in the string This file is too big. Files must be less than %s KB in size. be changed to %d to merge with already the existent This file is too big. Files must be less than %d KB in size. string in core?

[EDITED]
Nevermind @pento, just checked that the other strings were already changed in the trunk, it's all merged and consistent :)

[danielbachhuber]

@pedromendonca Gave me a minor heart attack there.

[pedromendonca]

@danielbachhuber sorry :)
The current GlotPress included the new beta strings, but still have the old deprecated ones. Later I've checked the current trunk code and everything is ok.