Make WordPress Core

Opened 4 years ago

Closed 4 years ago

Last modified 3 years ago

#51466 closed defect (bug) (fixed)

Site Health: don't give a warning when post_max_size is set to 0 (unlimited)

Reported by: pixolin's profile pixolin Owned by: sergeybiryukov's profile SergeyBiryukov
Milestone: 5.7 Priority: normal
Severity: normal Version: 5.5.1
Component: Site Health Keywords: has-patch has-screenshots
Focuses: Cc:

Description

Site health displays a warning, if the post_max_size PHP value is lower than upload_max_filesize.

This is wrong, if post_max_size is set to "unlimited" (value 0) (see Description of core php.ini directives).

Attachments (3)

51466.diff (817 bytes) - added by ratneshk 4 years ago.
Additional check to check the post_max_size != 0
Capture d’écran 2021-02-09 à 12.48.17.png (130.9 KB) - added by audrasjb 4 years ago.
Before applying the pull request: I can reproduce the false positive
Capture d’écran 2021-02-09 à 12.57.24.png (171.6 KB) - added by audrasjb 4 years ago.
After patch: the message displays informations about setting post_max_size to zero/unlimited

Download all attachments as: .zip

Change History (16)

#1 @SergeyBiryukov
4 years ago

  • Keywords needs-patch added
  • Milestone changed from Awaiting Review to 5.6

#2 @SergeyBiryukov
4 years ago

Good catch, thanks for the report!

Just noting this is related to [48806] / #50945.

#3 @Clorith
4 years ago

  • Keywords 2nd-opinion added

Hmmm, I'm not sure if this should change, as the documentation mentions there are scenarios where a value of 0 means a literal zero as well.

The current implementation provides a recommendation if the value is lower than the upload_max_filersize setting, and I think it's reasonable to retain it as a recommendation then, as it may benefit from changing to something specific, but it's not a critical issue if you leave it as it is. Would love a 2nd opinion on this though.

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


4 years ago

#5 @helen
4 years ago

  • Milestone changed from 5.6 to 5.7

The setting for %1$s is smaller than %2$s, this could cause some problems when trying to upload files.

I agree that the warning itself is incorrect and should be fixed; however, I think there's some room to debate whether it should still trigger a warning about it not being set to a good default value. The message isn't severely incorrect or alarming so I'm going to move this out of 5.6 for the moment because I don't see anybody working on it, but if there's an approach+patch very soon it could probably make its way back in.

@ratneshk
4 years ago

Additional check to check the post_max_size != 0

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


4 years ago

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


4 years ago

This ticket was mentioned in PR #986 on WordPress/wordpress-develop by Clorith.


4 years ago
#8

  • Keywords has-patch added; needs-patch removed

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

Adds more descriptive text in scenarios where post_max_size and upload_max_filesize differ, and post_max_size is set to a value of 0.

This because of how PHP may, in some scenarios, read 0 as a literal zero size, and not as unlimited, which it also means in other scenarios.

See https://www.php.net/manual/en/ini.core.php#ini.post-max-size for details, as PHP 5.3.4 introduced this behavior for absolute-null interpretation when the content type of a request is that of application/x-www-form-urlencoded.

Core does not currently do this in any locations I'm aware of, but plugins or themes may have code that does, so this is more a precaution (and having a fixed limit to avoid excessive memory consumption if something goes wrong is also always nice I guess), as such the text is geared to let users know that this is a recommendation, and it may affect plugins or themes.

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


4 years ago

@audrasjb
4 years ago

Before applying the pull request: I can reproduce the false positive

@audrasjb
4 years ago

After patch: the message displays informations about setting post_max_size to zero/unlimited

#10 @audrasjb
4 years ago

  • Keywords has-screenshots added; 2nd-opinion removed

I tested the patch and it works great on my side (see screenshots above) 👍

#11 @SergeyBiryukov
4 years ago

  • Owner set to SergeyBiryukov
  • Resolution set to fixed
  • Status changed from new to closed

In 50263:

Site Health: Clarify the recommendation in file uploads test when post_max_size is defined as 0.

This adds a more descriptive text in scenarios where post_max_size and upload_max_filesize differ, and post_max_size is set to a value of 0.

In some scenarios, PHP may read 0 as a literal zero size, and not as unlimited, which it also means in other scenarios.

See https://www.php.net/manual/en/ini.core.php#ini.post-max-size for details, as PHP 5.3.4 introduced this behavior for literal zero interpretation when the content type of a request is application/x-www-form-urlencoded or is not registered with PHP.

Props Clorith, pixolin, helen, ratneshk.
Fixes #51466.

Clorith commented on PR #986:


4 years ago
#12

Fixed in trunk

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


3 years ago

Note: See TracTickets for help on using tickets.