User tests: Successful: Unsuccessful:
JException is deprecated since Joomla 1.7. Guess it is time to get rid of it.
|Category||⇒||Administration com_languages com_menus com_modules com_users Front End com_content External Library Libraries Plugins Unit Tests|
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.
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.
Here's part of the problem.
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
IMO you have to work at this from outside the
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
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.
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
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
|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|
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?
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.
|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|
|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|
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 :)
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.
|Status||Pending||⇒||Fixed in Code Base|
|Closed_Date||0000-00-00 00:00:00||⇒||2018-02-03 22:12:07|