[4.0] Fix use of Symfony ErrorHandler, and update error handling in general by Fedik · Pull Request #27632 · joomla/joomla-cms

Fedik · 2020-01-25T10:45:02Z

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

mbabker · 2020-01-25T18:04:03Z

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.

mbabker · 2020-01-25T18:09:33Z

libraries/src/Exception/ExceptionHandler.php

+				}
+				finally
+				{
+					throw $e;


You’re better off using Reflection to alter the Exception properties than trying this black hat trickery. No way anyone can actually read and understand what this try/finally block is actually doing.

Reflection was a first thing I tried, but I got another Exception "Property previous not exist" when doing:

$refl = new ReflectionClass($e); $property = $refl->getProperty('previous');

Try/finally is one that work, it "collect" exceptions

That’s not a valid way of “collecting” Exceptions. At that point one can not evaluate the code and determine what is the intended order of operations or the intended end result, you’re basically relying on some magic and probably undocumented PHP behavior and that is too risky.

You’re better off rethrowing the original Exception without the error page exception than trying this trickery.

You just do not like a magic :-p
Because it not documented it does not means that it "not a valid", there many stuf undocumented, whole "finally" documentation has only 1-2 sentence.
As long as it works and allow to see nested exceptions it is okay, this is better than hiding errors that not possible to debug later.
I do not see any big risk in this particular case.

Relying on magic behaviors or undocumented engine behaviors is what gets you in trouble later on. Code is ALWAYS more stable if it has a predictable behavior, AFAIK this behavior is not specified in the RFC introducing finally support to PHP or any documentation detailing its use. Inherently this means it is a risky behavior to use as it may very well not be an engine design but something that works by accident in the way you want it to.

I found at least 2 test cases for this behavior:
exactly for this case : https://github.com/php/php-src/blob/master/Zend/tests/try/try_finally_002.phpt
and more complex case: https://github.com/php/php-src/blob/master/Zend/tests/try/catch_finally_004.phpt

Does not looks like a random engine behavior, which has test cases, or?
This part of the exception nesting API. There zero detail about it, but it does not means that it wrong.
I think it pretty stable code to use, I will leave it.
One issue I see that not all may understand how it work, but that not a code problem.

If you’re going to insist on keeping it this way (I’m not a fan of it but whatever) then the comment block for this needs to be a lot more clear about what it’s doing then. Because it’s not really helping to explain what you’re doing here at all or what the end result you’re creating is.

Fedik · 2020-01-25T18:13:07Z

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.

mbabker · 2020-01-25T18:15:52Z

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.

mbabker · 2020-01-25T18:20:01Z

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.

Fedik · 2020-01-25T18:29:12Z

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.

mbabker · 2020-01-25T21:17:15Z

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.

Fedik · 2020-01-26T10:20:27Z

I have made Error Handler configurable, without additional constant. Should be better now.
Testing instruction also updated.

wilsonge · 2020-01-26T12:51:09Z

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?

Fedik · 2020-01-26T13:19:27Z

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

Fedik · 2020-01-26T13:30:05Z

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.

Fedik · 2020-01-26T13:43:05Z

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.

mbabker · 2020-01-26T18:02:00Z

libraries/bootstrap.php

+
+// Make sure we do not display sensitive data in production environments
+// Disable a detailed error message by default
+$errorHandler->scopeAt(0, true);


I would still flat out remove this line. There’s no reason for the libraries bootstrap to make this opinionated of a decision without having the system configuration available. About the only acceptable default at this point in the bootup sequence to me would be to match the scope to the current error_reporting settings, which would be coming from the server’s PHP configuration.

libraries/bootstrap.php

Fedik · 2020-01-27T16:54:17Z

I have updated comments, and use server error_reporting for initial set up.

Fedik · 2020-02-09T17:52:56Z

Can we have some errors here please? I meant a tests 🙃

wilsonge · 2020-03-20T17:21:28Z

Merging this on review for now. Seems to have addressed @mbabker 's comments and I think he knows more of this than I clearly do :)

Fix use of Symfony ErrorHandler, and update error handling in general

b57ab6d

joomla-cms-bot added the PR-4.0-dev label Jan 25, 2020

Fedik added 4 commits January 25, 2020 12:51

phpcs

282400c

Restore previous comment with explanation

9248a03

typo

18febb5

Move out from index file

1b07527

mbabker reviewed Jan 25, 2020

View reviewed changes

Configurable error handler

faeaa80

mbabker reviewed Jan 26, 2020

View reviewed changes

libraries/bootstrap.php Show resolved Hide resolved

Use default error_reporting, and update comments

d19bcec

wilsonge merged commit 8bda312 into joomla:4.0-dev Mar 20, 2020

wilsonge added this to the Joomla 4.0 milestone Mar 20, 2020

Fedik deleted the error-handler branch March 20, 2020 17:40

Fedik mentioned this pull request Mar 28, 2020

[4.0] Exception handler with debug enabled, second try #28494

Merged

Uh oh!

Conversation

Fedik commented Jan 25, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary of Changes

Testing Instructions

Uh oh!

mbabker commented Jan 25, 2020

Uh oh!

mbabker Jan 25, 2020

Choose a reason for hiding this comment

Uh oh!

Fedik Jan 25, 2020

Choose a reason for hiding this comment

Uh oh!

mbabker Jan 25, 2020

Choose a reason for hiding this comment

Uh oh!

Fedik Jan 25, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mbabker Jan 25, 2020

Choose a reason for hiding this comment

Uh oh!

Fedik Jan 26, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mbabker Jan 26, 2020

Choose a reason for hiding this comment

Uh oh!

Fedik commented Jan 25, 2020

Uh oh!

mbabker commented Jan 25, 2020

Uh oh!

mbabker commented Jan 25, 2020

Uh oh!

Fedik commented Jan 25, 2020

Uh oh!

mbabker commented Jan 25, 2020

Uh oh!

Fedik commented Jan 26, 2020

Uh oh!

wilsonge commented Jan 26, 2020

Uh oh!

Fedik commented Jan 26, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Fedik commented Jan 26, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Fedik commented Jan 26, 2020

Uh oh!

mbabker Jan 26, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Fedik commented Jan 27, 2020

Uh oh!

Fedik commented Feb 9, 2020

Uh oh!

wilsonge commented Mar 20, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Fedik commented Jan 25, 2020 •

edited

Loading

Fedik Jan 25, 2020 •

edited

Loading

Fedik Jan 26, 2020 •

edited

Loading

Fedik commented Jan 26, 2020 •

edited

Loading

Fedik commented Jan 26, 2020 •

edited

Loading