? ?

Success

User tests: Successful: Unsuccessful:

avatar laoneo
laoneo
4 Jul 2017

JException is deprecated since Joomla 1.7. Guess it is time to get rid of it.

avatar laoneo laoneo - open - 4 Jul 2017
avatar laoneo laoneo - change - 4 Jul 2017
Status New Pending
avatar joomla-cms-bot joomla-cms-bot - change - 4 Jul 2017
Category Administration com_languages com_menus com_modules com_users Front End com_content External Library Libraries Plugins Unit Tests
avatar laoneo laoneo - change - 4 Jul 2017
Title
Remove JException
[4.0] Remove JException
avatar laoneo laoneo - change - 4 Jul 2017
Title
Remove JException
[4.0] Remove JException
avatar laoneo laoneo - edited - 4 Jul 2017
avatar wilsonge
wilsonge - comment - 4 Jul 2017

Umm I'm nervy about this. I'm not sure there's any point in taking the b/c hit of this like this PR does. Like I don't see a reason to rush it's removal. Of course it should die once JError is removed, but I don't see an amazing reason to remove it before then, and have people check for JException then generic exception, then whatever replaces JError

Willing to be convinced otherwise tho.

avatar laoneo
laoneo - comment - 4 Jul 2017

taking the b/c hit of this like this PR does

How else do you want to get rid of it when not in a new major version?

die once JError is removed

Why is this connected to JError removal?

I guess I don't get your concerns.

67cfe0d 4 Jul 2017 avatar laoneo CS
avatar laoneo laoneo - change - 4 Jul 2017
Labels Added: ? ?
avatar laoneo
laoneo - comment - 4 Jul 2017

One #16952 is in, then I will replace the JError::raise* calls with the replacements in the deprecate message here in 4, to get JError removed.

avatar mbabker
mbabker - comment - 4 Jul 2017

Here's part of the problem.

The JError internals are heavily primed toward working with JException. If you inspect the code, you'll see a lot of uses of API that only exists in our class and not the root Exception. So as is, this PR is actually fatally breaking JError.

IMO you have to work at this from outside the JError/JException pairing by converting the outside calls to it to use proper error handling mechanisms (JError::raiseError() is really just a proxy to triggering the error page without throwing an Exception in a way that nothing can even catch that error and try to do something else, the other raise*() methods just proxy to $app->enqueueMessage() now). You can't drop JException without dropping JError.

avatar laoneo
laoneo - comment - 5 Jul 2017

That's why I think we should add in J3 add proper deprecate messages (as I did in #16952), that we can replace the code which uses JError in J4 according to that deprecate messages. When this is done then this one here can be revaluated.

avatar laoneo
laoneo - comment - 6 Jul 2017

I prefer to go step by step. So removing JException on 4 first and then JError. I guess it will be hard to review if all will be done in one single pr.

avatar wilsonge
wilsonge - comment - 6 Jul 2017

I mean this https://github.com/Digital-Peak/joomla-cms/blob/67cfe0d6bfeffc3acb4ae736196c3f9eb91c2037/components/com_users/Model/Reset.php file which was directly using JException I'm happy to merge - as we shouldn't be using JException anywhere other than JError (although I'm very unconvinced by us returning exceptions!? - is that really the best thing to do?)

Otherwise all the other uses of JException are the result of using calling JError::raiseError (which if i recall returns an exception), and we should work our way through and work out what's appropriate where. I don't think just blindly removing JException from the internals of JError is going to be very useful on it's own

avatar laoneo
laoneo - comment - 6 Jul 2017

I guess rewriting the ResetModel should be done in it's own pr and shouldn't be part of this one.

raiseError shows the error page of the exception handler per default as I interprete this code here https://github.com/joomla/joomla-cms/blob/4.0-dev/includes/framework.php#L17

avatar brianteeman brianteeman - change - 19 Jul 2017
Milestone Added: Joomla 4.0
avatar brianteeman brianteeman - change - 19 Jul 2017
Milestone Added: Joomla 4.0
avatar joomla-cms-bot joomla-cms-bot - change - 26 Aug 2017
Category Administration com_languages com_menus com_modules com_users Front End com_content External Library Libraries Plugins Unit Tests Administration com_languages com_menus com_modules com_users Front End com_content Libraries Plugins Unit Tests
avatar laoneo
laoneo - comment - 26 Aug 2017

Conflicts fixed

avatar laoneo
laoneo - comment - 19 Nov 2017

If we are not going to remove it, then we should normally namespace it and remove the deprecated messages. This is deprecated since 1.7 and Joomla 4 is the chance to get rid of it, should we really need to wait another major?

avatar mbabker
mbabker - comment - 19 Nov 2017

We can't drop JException without JError being gone first. The JError internals are heavily coupled here.

If we can get JError and the 1.5 style error handling gone, this PR is easy to accept IMO. But, this PR is to me at the end of a long line of things that have to happen to reach the desired end state.

avatar laoneo
laoneo - comment - 19 Nov 2017

The removal of JError pr is #17669. Just for the record.

avatar wilsonge
wilsonge - comment - 2 Feb 2018

OK Get the conflicts done here and we can go through it

avatar joomla-cms-bot joomla-cms-bot - change - 2 Feb 2018
Category Administration com_languages com_menus com_modules com_users Front End com_content Libraries Plugins Unit Tests Administration com_languages com_menus com_modules com_users Front End com_content Libraries
avatar laoneo laoneo - change - 2 Feb 2018
Labels Removed: ?
avatar joomla-cms-bot joomla-cms-bot - change - 2 Feb 2018
Category Administration com_languages com_menus com_modules com_users Front End com_content Libraries Administration com_languages com_menus com_modules com_users Front End com_content Installation Libraries Unit Tests
avatar laoneo
laoneo - comment - 2 Feb 2018

Conflicts fixed and unit tests as well

avatar wilsonge
wilsonge - comment - 2 Feb 2018

Looking at the return types can we take the time to just check these are right - two of the first 3 i checked just are wrong - so may as well take this opportunity to correct them :)

avatar laoneo laoneo - change - 3 Feb 2018
Labels Added: ?
avatar laoneo
laoneo - comment - 3 Feb 2018

Cleaned up the return type doc block and changed some code parts where enqueueMessage is returned which is wrong is it is void but false should be returned in the models.

Actually cleaning up the return values should be done case by case in single pr's as it can break things and should be then tested individually.

avatar wilsonge wilsonge - close - 3 Feb 2018
avatar wilsonge wilsonge - merge - 3 Feb 2018
avatar wilsonge wilsonge - change - 3 Feb 2018
Status Pending Fixed in Code Base
Closed_Date 0000-00-00 00:00:00 2018-02-03 22:12:07
Closed_By wilsonge
avatar wilsonge
wilsonge - comment - 3 Feb 2018

Thank you

Add a Comment

Login with GitHub to post a comment