[#18088] - Refactored ExceptionHandler: separated logging and rendering
- Fixed in Code Base
- 26 Sep 2020
- Medium
- Build: staging
- # 18088
- Diff
- ethernidee:pr_ex_handler
- Success hound No violations found. Woof! Details
- Success continuous-integration/drone/pr the build was successful Details
- Success continuous-integration/travis-ci/pr The Travis CI build passed Details
- Success continuous-integration/appveyor/pr AppVeyor build succeeded Details
User tests: Successful: Unsuccessful:
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. If ExceptionHandler::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.
joomla-cms-bot
- | Category | ⇒ | Libraries |
| Status | New | ⇒ | Pending |
| Labels |
Added:
?
|
||
Yes. The separation allows to render default error page from plugins without logging. My clients were using custom loggers on almost all projects.
- A few minor improvements, like Joomla 4.0 class names, better readability, better cleaning of buffered output.
mbabker
- | Milestone |
Added: |
||
brianteeman
- | Milestone |
Removed: |
||
| Milestone |
Added: |
||
@ethernidee Are you still around? Can you fix the conflict please?
@ethernidee Are you still around? Can you fix the conflict please?
Done!
What is still needed to close this pull ?
I'm happy to merged this into 3.10 once we got some tests here. @joomla/bug-squad
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 :)
| Labels |
Added:
?
|
||
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 expectations
Shows the 3.9 Joomla error page.
Before test
Shows new error page in 3.10 that is contained in the template
Protostar:
Template without error.php:
After text
Shows the same error page as before the test. Not sure whether some other PR has already done what this PR is trying to achieve?
Protostar:
Template without error.php:
I have tested this item
This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/18088.
I have tested this item
There is no change to the 404 page with and without patch applied. And I have checked and the redirect plugin logging is still logging 404s
This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/18088.
| Status | Pending | ⇒ | Ready to Commit |
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
| Status | Ready to Commit | ⇒ | Fixed in Code Base |
| Closed_Date | 0000-00-00 00:00:00 | ⇒ | 2020-09-26 07:54:09 |
| Closed_By | ⇒ | zero-24 | |
| Labels |
Added:
?
|
||
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.
See comments #18088 (review).
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 test in production code.
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 preg_match('~\Atest[A-Z]~', $callerFunction) check.
Ok can you do a PR for that?
I get there's an element of single responsibility here - but is there a practical reason for breaking this up?