? ? ? Success

User tests: Successful: Unsuccessful:

avatar ethernidee
ethernidee
23 Sep 2017

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.

avatar joomla-cms-bot joomla-cms-bot - change - 23 Sep 2017
Category Libraries
avatar ethernidee ethernidee - open - 23 Sep 2017
avatar ethernidee ethernidee - change - 23 Sep 2017
Status New Pending
avatar ethernidee ethernidee - change - 24 Sep 2017
Labels Added: ?
avatar wilsonge
wilsonge - comment - 24 Sep 2017

I get there's an element of single responsibility here - but is there a practical reason for breaking this up?

avatar ethernidee
ethernidee - comment - 24 Sep 2017

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.
avatar mbabker mbabker - change - 11 Dec 2017
Milestone Added:
avatar brianteeman brianteeman - change - 8 May 2018
Milestone Removed:
avatar brianteeman brianteeman - change - 8 May 2018
Milestone Added:
avatar SharkyKZ
SharkyKZ - comment - 19 Sep 2019

@ethernidee Are you still around? Can you fix the conflict please?

avatar ethernidee
ethernidee - comment - 19 Sep 2019

@ethernidee Are you still around? Can you fix the conflict please?
Done!

avatar Milglius
Milglius - comment - 12 Jan 2020

What is still needed to close this pull ?

avatar HLeithner
HLeithner - comment - 7 Apr 2020

I moved this to 3.10 in my opinion it makes more sense there @zero-24

avatar zero-24
zero-24 - comment - 25 Sep 2020

I'm happy to merged this into 3.10 once we got some tests here. @joomla/bug-squad

avatar SharkyKZ
SharkyKZ - comment - 25 Sep 2020

@zero-24 Please read comments.

avatar zero-24
zero-24 - comment - 25 Sep 2020

@zero-24 Please read comments.

I have just removed the code mention lets see what drone says nowdays. Tests for the changed code are still required.

avatar zero-24
zero-24 - comment - 26 Sep 2020

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 :)

avatar zero-24 zero-24 - change - 26 Sep 2020
Labels Added: ?
avatar particthistle particthistle - test_item - 26 Sep 2020 - Tested successfully
avatar particthistle
particthistle - comment - 26 Sep 2020

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.
image

Before test

Shows new error page in 3.10 that is contained in the template
Protostar:
image
Template without error.php:
image

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:
image

Template without error.php:
image

avatar particthistle particthistle - test_item - 26 Sep 2020 - Tested successfully
avatar particthistle
particthistle - comment - 26 Sep 2020

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.

avatar softforge softforge - test_item - 26 Sep 2020 - Tested successfully
avatar softforge
softforge - comment - 26 Sep 2020

I have tested this item successfully on cb7c936

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.

avatar alikon alikon - change - 26 Sep 2020
Status Pending Ready to Commit
avatar alikon
alikon - comment - 26 Sep 2020

RTC


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/18088.

avatar zero-24
zero-24 - comment - 26 Sep 2020

Thanks 👍

avatar zero-24
zero-24 - comment - 26 Sep 2020

And thanks @ethernidee for you patience

avatar zero-24 zero-24 - change - 26 Sep 2020
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: ?
avatar zero-24 zero-24 - close - 26 Sep 2020
avatar zero-24 zero-24 - merge - 26 Sep 2020
avatar SharkyKZ
SharkyKZ - comment - 26 Sep 2020

@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.

avatar zero-24
zero-24 - comment - 26 Sep 2020

Ok can you do a PR else i can do one next week.

avatar SharkyKZ
SharkyKZ - comment - 26 Sep 2020

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.

avatar HLeithner
HLeithner - comment - 26 Sep 2020

So what did I missed why this is wrong? Can you fix it or at least explain what should be changed? @SharkyKZ

avatar SharkyKZ
SharkyKZ - comment - 26 Sep 2020

See comments #18088 (review).

avatar zero-24
zero-24 - comment - 26 Sep 2020

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?

avatar SharkyKZ
SharkyKZ - comment - 26 Sep 2020

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.

avatar zero-24
zero-24 - comment - 26 Sep 2020

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 :)

avatar SharkyKZ
SharkyKZ - comment - 26 Sep 2020

In that case the code you removed should be restored but without preg_match('~\Atest[A-Z]~', $callerFunction) check.

avatar zero-24
zero-24 - comment - 26 Sep 2020

Ok can you do a PR for that?

avatar zero-24
zero-24 - comment - 27 Sep 2020

PR has been done here: #30786 @SharkyKZ

Add a Comment

Login with GitHub to post a comment