Check for `file_uploads` in Site Health

[dd32]

I've just spent 5 minutes debugging someones site, trying to figure out why uploads were failing with a very specific and 'unique' error - Specified file failed upload test..

Turns out the hosts config has file_uploads = Off (They blamed WordPress for not being able to upload files) and Site Health was giving a nice green Good sign.

The ini setting being disabled should perhaps show up in either the Server or Media categories in the Debug section, and quite possibly as part of a "Media upload problem" critical issue, perhaps one that also verified that the upload setting was set to >= 2MB or something.

Add a new test in Site Health which checks if the file_uploads directive in PHP ini is turned off and show a critical issue if it is.

Trac ticket: https://core.trac.wordpress.org/ticket/50038

[donmhico]

Here's what it will look like if file_uploads directive is turned off in PHP ini. The messages / description could be better.

[JavierCasares]

Works for me.

About the texts:

File uploads is on in PHP ini
--> PHP allows you to upload files

The <code>file_uploads</code> directive in <code>php.ini</code> determines if uploading to is allowed in your WordPress.
--> The <code>file_uploads</code> directive in <code>php.ini</code> determines to allow HTTP file uploads.

file_uploads is boolean, so we should say true / false (file_uploads boolean: Whether or not to allow HTTP file uploads.)

<code>file_uploads</code> is set to <code>0</code>. You won\'t be able to upload files in your WordPress.
--> <code>file_uploads</code> is set to <code>false</code>. You won\'t be able to upload files in your WordPress.

[JavierCasares]

Also, this ticket gives me a suggestion...

Should we prevent the file upload forms in Media from working if the directive is disabled?

Right now the system allows the file to be uploaded, it shows the progress bar, but at the end it returns an error...

If the system doesn't allow file uploads, shouldn't we just say "you can't upload anything, check the Site Health" and not show the forms?

[SergeyBiryukov]

Replying to JavierCasares:

Should we prevent the file upload forms in Media from working if the directive is disabled?

Yes, I think that's a good idea. It seems out of scope for this ticket though, could you create a new one with that suggestion?

[JavierCasares]

Should we prevent the file upload forms in Media from working if the directive is disabled?

Yes, I think that's a good idea. It seems out of scope for this ticket though, could you create a new one with that suggestion?

Done at #50188

[ayeshrajans]

I'm so sorry if this looks like hijacking a thread. In response to #50188, I worked on a patch that adds a "File upload settings" section to Debug page. It shows the PHP ini settings to the user, and an effective file size which is the minimum of post_max_size and max_file_size.

I will re-upload the screenshot and patch to this issue which I believe is the more relevant one.

[ayeshrajans]

Please ignore 50188.patch.

[JavierCasares]

Great @ayeshrajans

A suggestion... usually people wants to uploads big files... so, if upload_max_filesize is greater than post_max_size, should we notify something like "if you want to upload files up to upload_max_filesize (whatever the size is) you should update post_max_size to upload_max_filesize (the value).

For example:

• upload_max_filesize = 2G
• post_max_size = 8M

If you want to upload files up to 2G you should change post_max_size to 2G.

Or something like that. This maybe at the Status tab.

[ayeshrajans]

@JavierCasares - I agree, it's often confusing when the upload_max_filesize is increased, but the post_max_size set too low, which often doesn't yield meaningful error messages either.

How do you think the message should say? I suppose suggesting an excessively post_max_size is not good either. Perhaps if the upload_max_filesize * max number of files is bigger than post_max_size?

[Clorith]

It seems we've somehow mixed two tickets suddenly here, so gonna try and summarize real quick.

I like the approach from @donmhico here of introducing a check for it, it'll need some copy-editing to hit the right tone and relay what we need the user to know about this.

Adding a warning/recommendation if the post size and upload sizes mis-match seems like a logical addition to this same check, and introducing the value under Media in the Info tab is a good addition alongside this, and would clearly help with debugging.

So what we need here for this to land is a refresh of the patch, as well as adding the information about sizes at the same time, is this something you're up for @donmhico, as you made the original patch?

(Also gonna ping @marybaum as our resident copy-expert, and knower of who may help if not her)

Deleting to refresh the patch. I kinda messed up this branch by trying to rebase it.

[donmhico]

@Clorith - Sure. Working on it right now.

This PR does a few things.

1. Display file_uploads, post_max_size, upload_max_filesize, upload_max, and max_file_uploads in Media Handling section in Site Health > Debug tab.

<img width="1680" alt="Screen Shot 2020-06-26 at 10 13 30 PM" src="​https://user-images.githubusercontent.com/5747475/85866918-bd11f880-b7fa-11ea-959d-4b776c346365.png">

2. Display a "Recommended" notice in Site Health Status page for mismatch post_max_size and upload_max_filesize.

<img width="1680" alt="Screen Shot 2020-06-26 at 10 18 17 PM" src="​https://user-images.githubusercontent.com/5747475/85867171-124e0a00-b7fb-11ea-8a53-44832e1ff775.png">

3. Display a "Critical" notice in Site Health Status page if file_uploads is off.

<img width="1680" alt="Screen Shot 2020-06-26 at 10 21 12 PM" src="​https://user-images.githubusercontent.com/5747475/85867432-7a045500-b7fb-11ea-92bb-67dcc5489e8a.png">

Trac ticket: https://core.trac.wordpress.org/ticket/50038

[donmhico]

Submitted a new PR that hopefully address points from @Clorith's comment. Props to @ayeshrajans for his patch. The new PR is pretty much the combination of our patches.

[ayeshrajans]

I guess GitHub code review comments don't sync here. Repeating my comment on the PR

$effective = min( $post_max_size, $upload_max_size );

I'm not quite sure that this will work as intended. For example, min('4M', '500K') returns 4M, and not 500K as expected. I think these values should go through parse_ini_size() method before comparison.

[donmhico]

Thanks for the comment in the PR @ayeshrajans. I pushed the resolution.

[Clorith]

Thanks for the updated patch that unifies both approaches.

I've got some thoughts on the texts, which I've put as feedback on the PR it self (easier to reference the lines that way :) )

[whyisjake]

I think I have the feedback integrated that @Clorith was looking for. Could you take a review when possible?

[whyisjake]

Props to @ipstenu and @sabernhardt for some i11n help on Slack.

[whyisjake]

In 48535:

Site Health: Include new tests to check for the ability to upload files.

Several new checks:

• max_file_uploads
• file_uploads
• post_max_size
• upload_max_filesize
• upload_max
• max_file_uploads

In addition, new function parse_ini_size() that converts shorthand byte strings to bytes. Useful for size comparisons.

Fixes #50038.
Props dd32, donmhico, JavierCasares, SergeyBiryukov, ayeshrajans, Clorith, ipstenu, sabernhardt, whyisjake.

[SergeyBiryukov]

In 48538:

Site Health: Remove parse_ini_size(), use the existing wp_convert_hr_to_bytes() function instead.

Follow-up to [48535].

See #50038.

[SergeyBiryukov]

In 48539:

Site Health: Move post_max_size and upload_max_filesize out of a translatable string in file upload checks.

Simplify some other strings, use a consistent format for translator comments.

Follow-up to [48535].

See #50038.

[SergeyBiryukov]

In 48544:

Site Health: Rename upload_max array key in file upload checks to max_effective_size for clarity.

Follow-up to [48535].

See #50038.