[4.0] Remove JException

[laoneo]

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

[wilsonge]

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.

[laoneo]

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.

[laoneo]

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.

[mbabker]

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.

[laoneo]

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.

[laoneo]

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.

[wilsonge]

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

[laoneo]

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

[laoneo]

Conflicts fixed

[laoneo]

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?

[mbabker]

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.

[laoneo]

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

[wilsonge]

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

[laoneo]

Conflicts fixed and unit tests as well

[wilsonge]

This doesn't return an exception anymore

[wilsonge]

This is returning bool|void

[wilsonge]

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

[laoneo]

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.

[wilsonge]

Thank you