WordPress.org

Make WordPress Core

Opened 5 months ago

Closed 2 months ago

Last modified 2 months ago

#50038 closed defect (bug) (fixed)

Check for `file_uploads` in Site Health

Reported by: dd32 Owned by: whyisjake
Milestone: 5.5 Priority: normal
Severity: normal Version:
Component: Site Health Keywords: has-patch dev-feedback commit
Focuses: Cc:

Description

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.

Attachments (6)

Screen Shot 2020-05-15 at 9.45.10 PM.png (84.1 KB) - added by donmhico 5 months ago.
Here's what it will look like if file_uploads directive is turned off in PHP ini. The messages / description could be better.
upload-settings.png (25.3 KB) - added by ayeshrajans 5 months ago.
50188.patch (2.7 KB) - added by ayeshrajans 5 months ago.
50038-debug-data.patch (2.7 KB) - added by ayeshrajans 5 months ago.
Please ignore 50188.patch.
50038.diff (5.8 KB) - added by whyisjake 2 months ago.
50038.2.diff (6.2 KB) - added by whyisjake 2 months ago.

Download all attachments as: .zip

Change History (34)

This ticket was mentioned in PR #273 on WordPress/wordpress-develop by donmhico.


5 months ago

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

This ticket was mentioned in Slack in #core by donmhico. View the logs.


5 months ago

@donmhico
5 months ago

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

#3 @donmhico
5 months ago

  • Keywords has-patch added

#4 @SergeyBiryukov
5 months ago

  • Milestone changed from Awaiting Review to 5.5

#5 @JavierCasares
5 months ago

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.

#6 follow-up: @JavierCasares
5 months ago

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?

#7 in reply to: ↑ 6 ; follow-up: @SergeyBiryukov
5 months ago

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?

#8 in reply to: ↑ 7 @JavierCasares
5 months ago

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

#9 @ayeshrajans
5 months ago

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
5 months ago

@ayeshrajans
5 months ago

Please ignore 50188.patch.

#10 @JavierCasares
5 months ago

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.

#11 @ayeshrajans
5 months ago

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

This ticket was mentioned in Slack in #core-site-health by afragen. View the logs.


4 months ago

This ticket was mentioned in Slack in #core-site-health by afragen. View the logs.


3 months ago

#14 @Clorith
3 months ago

  • Keywords needs-refresh added; has-patch removed

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)

#15 @prbot
3 months ago

donmhico commented on PR #273:

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

#16 @donmhico
3 months ago

@Clorith - Sure. Working on it right now.

This ticket was mentioned in PR #363 on WordPress/wordpress-develop by donmhico.


3 months ago

  • Keywords has-patch added; needs-refresh removed

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">

  1. 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">

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

#18 @donmhico
3 months ago

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.

#19 @ayeshrajans
3 months ago

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.

#20 @donmhico
3 months ago

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

#21 @Clorith
3 months ago

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 :) )

This ticket was mentioned in Slack in #core by david.baumwald. View the logs.


3 months ago

@whyisjake
2 months ago

@whyisjake
2 months ago

#23 @whyisjake
2 months ago

  • Keywords dev-feedback commit added
  • Owner set to whyisjake
  • Status changed from new to accepted

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

#24 @whyisjake
2 months ago

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

#25 @whyisjake
2 months ago

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

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.

#26 @SergeyBiryukov
2 months ago

In 48538:

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

Follow-up to [48535].

See #50038.

#27 @SergeyBiryukov
2 months ago

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.

#28 @SergeyBiryukov
2 months ago

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.

Note: See TracTickets for help on using tickets.