Skip to content

Platform check: Trigger native PHP error #9414

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
Seldaek merged 4 commits into from
Nov 4, 2020
Merged

Platform check: Trigger native PHP error #9414

Seldaek merged 4 commits into from
Nov 4, 2020

Conversation

@jakubboucek
Copy link
Contributor

@jakubboucek jakubboucek commented Nov 4, 2020

Following up #9412 and #9410 I found current error handling with mechanism header(); echo …; die(1+); is not satisfied for success debugging on most implementations.

General monitoring tools prefers to monitor native error log instead to parsing response body (tested on Google App Engine and Google Run).

Here are two reasons:

  • native error handling in any level (regardless to app-level or sys-level),
  • security – printing too detailed internal error message in runtime is very sensitive, it can provide implementation details for attackers,
  • indepentent to SAPI (cli, cgi, etc.), environment, and others, because it's produce native error state in app runtime lifecycle.

This PR is replacement to #9410 and it's conflicts with them. Only one can be merged, I prefer this PR.

FAQ

This PR is cause to show empty page without body.

That's right. I understand it's unpleasantly. This is standard way to propagate raw errors. User can change this behavior by setting php runtime like:

ini_set('display_errors', 1);
ini_set('display_startup_errors', 1);
error_reporting(E_ALL);

or by custom error handling.

@jakubboucek jakubboucek mentioned this pull request Nov 4, 2020
Merged
stof
stof reviewed Nov 4, 2020
View reviewed changes
@naderman
Copy link
Member

naderman commented Nov 4, 2020

@jakubboucek Thanks for the PR I definitely prefer this over what is currently happening, we should be using standard PHP error handling mechanisms, as I already documented in the last point on the checklist here: #9412

The main concern now would be that people who e.g. use wordpress and just update some plugin will be confronted with a blank page with no idea what caused it. We shouldn't display internal info, but I suppose a hint like "Composer detected an incompatible PHP version" with a link to our docs may be output anyway if sapi is web and headers haven't been sent yet?

@jakubboucek
Copy link
Contributor Author

jakubboucek commented Nov 4, 2020 •
edited
Loading

@naderman Thanks. I am thinking about it for while time and I changing my opinion: the any variant of fatal error is undesirable for default behavior, until developer expressly require it.

Composer v1 was been very useful utility to compose app.

Composer v2 has now new face, it's not yet helper, friend, Jenkins for my work. Now is getting new despotic face, it's can violently break my job when anything reqs is missed.

Here should be E_USER_WARNING at most. But never break runtime in default setup, until developer change default behavior if he is aware of the consequences of his actions.

@naderman naderman mentioned this pull request Nov 4, 2020
Closed
6 tasks
@Seldaek Seldaek merged commit ec960d1 into composer:master Nov 4, 2020
@Seldaek
Copy link
Member

Seldaek commented Nov 4, 2020

Thanks, merged and added some extra output back in case errors are not visible, see 8c1355f

@Seldaek Seldaek added this to the 2.0 milestone Nov 4, 2020
@jakubboucek jakubboucek deleted the feature/jb-platform-check-tigger-error branch November 4, 2020 21:51
@jakubboucek jakubboucek mentioned this pull request Sep 30, 2022
Closed
@Krinkle Krinkle mentioned this pull request Jun 12, 2025
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@naderman naderman naderman left review comments

+1 more reviewer

@stof stof stof left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

2.0

Development

Successfully merging this pull request may close these issues.

4 participants

@jakubboucek @naderman @Seldaek @stof