Fix platform check error message #9418
Conversation
| @@ -735,10 +735,11 @@ protected function getPlatformCheck($packageMap, array $ignorePlatformReqs, $che | |||
| \$issues = array(); | |||
| ${requiredPhp}${requiredExtensions} | |||
| if (\$issues) { | |||
| if (!ini_get('display_errors')) { | |||
| if (ini_get('display_errors')) { | |||
There was a problem hiding this comment.
I think you misunderstood here :) The point is to ensure that if we break a site and it has no errors visible, we print the message so it's not a blank page and gives a hint to people. Please revert.
There was a problem hiding this comment.
@Seldaek That sounds like a very bad idea.
Reasons:
- Developers is turns off this flat to prevent any error message to output. Their customers does not care about any internal bugs. It's look seriously non-proffesional.
- Developers often have in their projects custom error handling which is render custom error page. This
echois bypass clean handling. - The message is leaking too detailed specifics about app/server configuration, it's very unsecure.
There was a problem hiding this comment.
Well I think we're gonna have to agree to disagree here :)
- Now that I changed this to only check php version by default, and to only include non-dev dependencies in the check, the chance that this outputs some "internal bug" which is irrelevant is very unlikely. Stuff will most likely break if the PHP version is too low.
- The project error handler will in most cases not be loaded yet when the platform check runs as it is loaded with the autoloader, which is typically one of the first lines of code running.
- It's not leaking any application info anymore, as it now str_replaces the php version away when outputting on non-CLI SAPIs.
On top of that, if display_errors is on, the error will be shown already, so echoing it again on top is completely useless and duplicate.
There was a problem hiding this comment.
Your arguments are strong, but I'm still convinced the direct printing error message to output in runtime (expecially at non-CLI SAPI) is subject of exceeding the boundary of responsibility that users can fairly expect. :)
Reverted, thanks.
|
Makes sense to add the 500 back, totally forgot, thanks. |
Fixes bug from commit: 8c1355f
Prints error to output only whenini_get('display_errors')is truly,500 Internal server errorto error message (see Fix HTTP status when Platform check failed #9410 discussion;note the).trigger_error()below doesn't change status, becauseechois already sent data to output@Seldaek however, in any case, your solution is better than my previous solution. Thanks for your great work!