Refactored ExceptionHandler: separated logging and rendering #18088
Conversation
ethernidee
force-pushed
the
pr_ex_handler
branch
2 times, most recently
from
0c1b2bd
to
d3cbfd6
Compare
ethernidee
force-pushed
the
pr_ex_handler
branch
from
d3cbfd6
to
39a759d
Compare
|
I get there's an element of single responsibility here - but is there a practical reason for breaking this up? |
|
Yes. The separation allows to render default error page from plugins without logging. My clients were using custom loggers on almost all projects.
|
ghost
added
the
|
@ethernidee Are you still around? Can you fix the conflict please? |
|
| // Clear buffered output at all levels in non-test mode | ||
| $callerFunction = static::getCallerFunctionName(); | ||
|
|
||
| if ($callerFunction === false || !preg_match('~\Atest[A-Z]~', $callerFunction)) |
There was a problem hiding this comment.
What is the purpose of checking for test here?
There was a problem hiding this comment.
Cleaning buffered output during unit-testing makes tests fail, because they rely on all output. In production mode everything outputed before error must be discarded.
There was a problem hiding this comment.
This wasn't an issue before? I don't think it's right hardcoding a function name here. And what if you have a testSomething() function in production code?
There was a problem hiding this comment.
If I remember correctly, it was an an issue, because only single level of buffering was discarded.
if (ob_get_level) {ob_get_clean()}.
But when exception is raised, there can be multiple level of buffering, thus I clean them all. Because tests began to fail, I had to add some check for test mode. Do you have any other ideas?
There was a problem hiding this comment.
No, I'm dumb with tests. Maybe someone more familiar can comment. @Hackwar @HLeithner ?
Adapted file prolog to Joomla style according to reviewer recommendations
|
What is still needed to close this pull ? |
|
Drone is happy now so whatever was there in the past that is now fixed and that extra code is not needed. Tests would be great :) |
|
Applying patch has no effect on the 404. Note: Check you have a Joomla .htaccess file in correctly first otherwise you'll be looking at a server level error message instead! Test on 3.9.21 - Baseline of expectationsShows the 3.9 Joomla error page. Before testShows new error page in 3.10 that is contained in the template After textShows the same error page as before the test. Not sure whether some other PR has already done what this PR is trying to achieve? Template without error.php: |
|
I have tested this item ✅ successfully on cb7c936 This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/18088. |
|
I have tested this item ✅ successfully on cb7c936 This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/18088. |
|
RTC This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/18088. |
|
Thanks 👍 |
|
And thanks @ethernidee for you patience |
|
@zero-24 This should not have been merged as is. Only part of the code you removed was related to tests. Rest of the code is necessary. Otherwise PHP notices appear on the error page. |
|
Ok can you do a PR else i can do one next week. |
|
Sorry, I missed a word in the comment. It should have said "This should not have been merged". Neither the original code, nor the merged code is fine. |
|
So what did I missed why this is wrong? Can you fix it or at least explain what should be changed? @SharkyKZ |
|
See comments #18088 (review). |
I'm sorry I can not follow you. Based on that comment it was about an failing unit test that does not longer fail right? |
|
No, we do need to clear the buffer. But this is, apparently, causing issues in tests so @ethernidee added a check for function name as a workaround. But it's not reliable as you can easily have functions starting with |
|
Yes that check has been removed and the test issues mention does not seem to happen anymore with drone. We have to take into account that this was an 2017er PR with a totally different testing setup :) |
|
In that case the code you removed should be restored but without |
|
Ok can you do a PR for that? |
Pull Request for Issue #18088 Add back the check to clear the buffered output.
Summary of Changes
Refactored ExceptionHandler class (previously JErrorPage): separated logging and rendering, added helper methods, replaced old class names (JXxx) with namespaced ones.
Now if
ExceptionHandler::render()is called manually from anywhere, no logging is performed. IfExceptionHandler::handleException()is called, both logging and rendering is performed.Testing Instructions
Enter non-existing url in browser to trigger 404 page.
Expected result
Default template error page should be displayed, as usual.
Documentation Changes Required
No changes.