[#27632] - [4.0] Fix use of Symfony ErrorHandler, and update error handling in general
- Fixed in Code Base
- 20 Mar 2020
- Medium
- Build: 4.0-dev
- # 27632
- Diff
- Fedik:error-handler
- Pending Hound Hound is busy sniffing around... Details
User tests: Successful: Unsuccessful:
Summary of Changes
@wilsonge I found an issue in #27470
You have set HtmlErrorRenderer as ExceptionHandler that incorrect of course, because it a rendered not a handler
I have fixed it here.
And because ErrorHandler may show whole error detail on production environment I have disable it by default, a full trace may be enabled by enabling debug or changing "display errors" option in global config
Additionally I have made changes to Joomla\ExceptionHandler, now it will use global ExceptionHandler as "last resort", which allow to do a better debugging, now it also pre-save a full trace of both exceptions, instead of old useless "Error" message.
Testing Instructions
First step:
To test need to emulate a "heavy crash", for this add 2 exception to the cassiopeia template:
First in index.php : throw new Exception('The main error!');
Second at top of error.php: throw new Exception('The error of the error!');
Then go to the site, you should see the error:
Second step:
Go to administrator global configuration and set "Display Error" to "maximum", or enable "Debug"
Then go to the site, you should see detailed information about all exceptions that occurred:
(click on little +)
@mbabker please have a look also
| Status | New | ⇒ | Pending |
joomla-cms-bot
- | Category | ⇒ | Administration Libraries |
| Labels |
Added:
?
|
||
Use the system configuration for this or don’t introduce a new behavior.
Yeah, but I have no idea here, configuration.php not possible to use at this point.
It may be existing constant in git repository, and removed while release. But this also may be confusing.
Also, do not judge the use based solely on naming.
What do you mean? I not very understood.
And because ErrorHandler may show whole error detail on production environment I have disable it by default
If you reviewed the component then you’d know that by default it is in a state where only an error message is printed, there is no need for extra configuration except to enable additional debug information.
To me this PR comes across as someone not really understanding the code attempting to make changes for the sake of change. The only tangible benefit is the rethrow if rendering the error page fails but that code is convoluted and impossible to actually follow what the intended behaviors are.
Use the system configuration for this or don’t introduce a new behavior.
Yeah, but I have no idea here, configuration.php not possible to use at this point.
It may be existing constant in git repository, and removed while release. But this also may be confusing.
I’m saying don’t introduce a new conditionally defined constant that someone has to edit a file to enable. Use the configuration. The reason I never enabled debug output is because of the problem you noted, adding new constants doesn’t fix the bootstrapping problem (because ultimately the issue here is things are not loaded in a usable order, load it all in the right order and the need for a conditional constant goes away).
Also, do not judge the use based solely on naming.
What do you mean? I not very understood.
You’re saying it’s wrong based solely on the fact the class is named “renderer”. I’m saying that might not be the case.
If you reviewed the component then you’d know that by default it is in a state where only an error message is printed, there is no need for extra configuration except to enable additional debug information.
I am actually disabling it, because it enabled by default:
https://github.com/symfony/error-handler/blob/06443075f4274d908705191f3777d8e5a69357a7/ErrorHandler.php#L89-L91
You’re saying it’s wrong based solely on the fact the class is named “renderer”. I’m saying that might not be the case.
I did not made this conclusion by reviewing the name, that would be not very smart.
I found it while debugging.
In fact you cannot set custom Rendered:
https://github.com/symfony/error-handler/blob/06443075f4274d908705191f3777d8e5a69357a7/ErrorHandler.php#L698-L713
To do this we need to make own Handler class that extend symfone handler, if I right understood.
If you reviewed the component then you’d know that by default it is in a state where only an error message is printed, there is no need for extra configuration except to enable additional debug information.
I am actually disabling it, because it enabled by default:
https://github.com/symfony/error-handler/blob/06443075f4274d908705191f3777d8e5a69357a7/ErrorHandler.php#L89-L91
Then it should respect the error handling configuration, not be an all or nothing thing or based on a hidden configuration flag. Which still means the load order needs to be fixed.
I have made Error Handler configurable, without additional constant. Should be better now.
Testing instruction also updated.
You have set HtmlErrorRenderer as ExceptionHandler that incorrect of course, because it a rendered not a handler
? .
That's not really true? https://github.com/symfony/symfony/blob/master/src/Symfony/Component/ErrorHandler/ErrorHandler.php#L149 The renderer is still the default exception handler in your code - you just aren't calling it explicitely ;)
However I think largely the changes your making here are on the right lines - although I agree with Michael to an extent - the changes you are making to the exception handler are really confusing to me :D There has to be a cleaner way of dealing with it?
The renderer is still the default exception handler in your code - you just aren't calling it explicitely
That confusing part
Rendered cannot be as handler.
The code does not explode, because setExceptionHandler allow callback with the same argument as Renderer::render (\Throwable $exception)
in the same way we can set ClassNotFoundErrorEnhancer as handler and it will not explode, but it does not do anything usefull.
Look at the source, it just return FlattenException https://github.com/symfony/symfony/blob/524ee7acb676d3e2109c8f4afb8e9fa60711ee4f/src/Symfony/Component/ErrorHandler/ErrorRenderer/HtmlErrorRenderer.php#L67 , and does not produce any output.
The output going there https://github.com/symfony/symfony/blob/524ee7acb676d3e2109c8f4afb8e9fa60711ee4f/src/Symfony/Component/ErrorHandler/ErrorHandler.php#L692-L713 where the HtmlErrorRenderer or CliErrorRenderer are actualy used ;)
There has to be a cleaner way of dealing with it?
You mean nested exception? I think it okay, Learning something new it is okay :)
Here a bit about https://translate.google.com/translate?hl=en&sl=auto&tl=en&u=https%3A%2F%2Fqiita.com%2Fmpyw%2Fitems%2F7f5a9fe6472f38352d96
It seems old feature that not much know, but there many such things.
And the behavior covered by PHP test
here is a better explanation https://nikic.github.io/2017/04/14/PHP-7-Virtual-machine.html#finally-handling
read "Finally handling" section
Throw from finally: If there is a backed-up exception in the FAST_CALL temporary, chain it as the previous exception of the thrown one. Continue bubbling the exception up to the next try/catch/finally.
And I believe it part of this RFC (Implemented) https://wiki.php.net/rfc/finally wich has the same test files, unfortunately they lost in history.
I have updated comments, and use server error_reporting for initial set up.
Can we have some errors here please? I meant a tests
| Status | Pending | ⇒ | Fixed in Code Base |
| Closed_Date | 0000-00-00 00:00:00 | ⇒ | 2020-03-20 17:21:22 |
| Closed_By | ⇒ | wilsonge |
No more commented constants that have to be edited in code to access. Use the system configuration for this or don’t introduce a new behavior.
Also, do not judge the use based solely on naming. This may very well be the correct way to configure the component, or the rendering logic may require additional adjustments to do right.