Symfony compliant error handling #18790
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
cedric-anne
force-pushed
the
11.0/symfony-error-handler
branch
from
d60e20e to
ee6ed5c
Compare
trasher
approved these changes
Jan 23, 2025
2 tasks
|
Need a rebase since #18801 was merged. |
| /** | ||
| * GLPI (instantiation and so on) | ||
| **/ | ||
| * @FIXME Find a better place for these constants. |
There was a problem hiding this comment.
Which constants are you talking about ?
There was a problem hiding this comment.
This class only contains constants now.
There was a problem hiding this comment.
I see. Well I guess we can just rename it and turn it into an enum.
cedric-anne
force-pushed
the
11.0/symfony-error-handler
branch
from
ee6ed5c to
4e52ed9
Compare
AdrienClairembault
approved these changes
Jan 23, 2025
SebSept
reviewed
Jan 24, 2025
| @@ -47,6 +47,9 @@ | |||
| $parameters->set('env(APP_SECRET_FILE)', $projectDir . '/config/glpicrypt.key'); | |||
| $parameters->set('kernel.secret', env('default:glpi.default_secret:file:APP_SECRET_FILE')); | |||
|
|
|||
| // Prevent low level errors (e.g. warning) to be converted to exception in dev environment | |||
There was a problem hiding this comment.
This is because of a too high number of this errors ?
There was a problem hiding this comment.
No, it is because we used to use warnings in GLPI for non blocking errors, and I wanted to preserve this behaviour for the moment.
3 tasks
2 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Checklist before requesting a review
Description
The Symfony error handler and our own error handler are currently executed in parallel. It result in unexpected side effects, including:
The purpose of this PR is to extend the Symfony error handler to reproduce the behaviour of our own legacy error handler., in order to fix these side effects.
Also, error messages are now only output when it is relevant. There is no need to manually call the
disableOutput()method to prevent outputing messages in a non HTML context.I was able to remove the
Kernel::sendRequest()method, as the exceptions thrown during the response sending are now correctly caught.I took the opportunity to add a test validating the format of our
php-errors.logfile. This file is probably parsed by some scripts and it is preferable to make its format stable over time. Testing the error handler itself is a bit complex (it would probably require a specific E2E test to check logs) and, as it extends the Symfony base handler, most of the code is already tested by Symfony itself.It is based on #18696 .