Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[4.0] Remove JError #17669

Merged
merged 6 commits into from Feb 2, 2018
Merged

[4.0] Remove JError #17669

merged 6 commits into from Feb 2, 2018

Conversation

laoneo
Copy link
Member

@laoneo laoneo commented Aug 22, 2017

JError is deprecated since ages and it was marked to be removed in Joomla 4 in the deprecate message. This pr cleans up the core from JError and removes the class.
It is the followup pr of #16952 and #16967 which is the last piece to have JExceptionand JError being removed.

@@ -354,10 +354,6 @@ public function setIntegrationObject($key, $object)
*/
public function setErrorHandling($level, $log_level, $options = array())
{
if (version_compare(JVERSION, '3.0', 'lt') )
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't need to edit FOF do we?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Definitely not. It's a vendor file.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As far as I know, Joomla is maintaining the FOF code till it gets removed. So we should keep it clean to a degree where it is not breaking obviously.

Copy link
Contributor

@wilsonge wilsonge Aug 23, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's being removed #17687 :P so not sure it matters for changes in 4.x

@wilsonge
Copy link
Contributor

can you fix conflicts here please?

…e-jerror

# Conflicts:
#	libraries/legacy/error/error.php
@laoneo
Copy link
Member Author

laoneo commented Aug 24, 2017

Conflicts fixed

…e-jerror

# Conflicts:
#	libraries/fof/platform/platform.php
@laoneo
Copy link
Member Author

laoneo commented Aug 26, 2017

Conflicts fixed

@laoneo
Copy link
Member Author

laoneo commented Nov 19, 2017

What is the status of this one?

@laoneo laoneo mentioned this pull request Nov 19, 2017
@mbabker
Copy link
Contributor

mbabker commented Nov 19, 2017

Are we positive that the use of getError and setError in JObject subclasses (or any place doing this still really) are OK with proper Exceptions and have no internal couplings to JError or JException mechanisms? If we can drop this side of the functionality without completely breaking those two methods which still have heavy use I think it's worth re-syncing this PR and doing a heavy testing round to make sure we're OK.

@laoneo
Copy link
Member Author

laoneo commented Nov 20, 2017

I'm going to resync when I get the ok from @wilsonge that it is getting in when all is fine.

@wilsonge
Copy link
Contributor

JObject's setError method definitely just adds (what's doc-block documneted as) strings from an internal array https://github.com/joomla/joomla-cms/blob/staging/libraries/src/Object/CMSObject.php#L229-L243 . In the getters we also deal with that string actually being an exception https://github.com/joomla/joomla-cms/blob/staging/libraries/src/Object/CMSObject.php#L131-L182

So we're definitely safe for JObject classes. I guess I need to go back through this again and just check what classes are actually using JError directly - but most of this stuff is outdated docblocks iirc

@wilsonge wilsonge self-assigned this Nov 20, 2017
@laoneo
Copy link
Member Author

laoneo commented Nov 20, 2017

As I did this pr, I removed every trace of JError.

…e-jerror

# Conflicts:
#	libraries/legacy/error/error.php
@laoneo
Copy link
Member Author

laoneo commented Nov 20, 2017

Was a rather painless merge, so we are in sync again.

@wilsonge
Copy link
Contributor

On review this looks fine to me. Let's get some heavy testing on this

@wilsonge wilsonge merged commit bf8194a into joomla:4.0-dev Feb 2, 2018
@wilsonge wilsonge deleted the j4/remove-jerror branch February 2, 2018 00:00
@wilsonge
Copy link
Contributor

wilsonge commented Feb 2, 2018

OK Well I'm sure we'll break some things - but clearly we need this and it's not getting tests

@skurvish
Copy link

In my component migration to namespaces in preparation of J4 I am finding JError in src/Mail/Mail.php so it looks like not all traces of JError are gone.


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

@laoneo
Copy link
Member Author

laoneo commented Nov 21, 2019

@skurvish can you post here a link to the line you see this error? I can't find a reference to the class, just an entry of the logger with jerror.

@skurvish
Copy link

In the current joomla git you can find JError at lines 118 & 154 in Mail/Mail.ph. You can also find it in Application/CMSApplication.php, Cache/Storage/RedisStorage.php, Client/ClientWrapper.php, Installer/Adapter/ComponentAdapter.php, Menu/SiteMenu.php, MVC/View/CategoryFeedView.php, MVC/Model/AdminModel.php & MVC/View/CategoryView.php. These are all in libraries/src

@mbabker
Copy link
Contributor

mbabker commented Nov 21, 2019

Are you looking at the right version of those files? It’s expected to still be there for 3.x versions, but should be gone in 4.0 (just looking at the Mail.php file it isn’t used in the 4.0 file).

@skurvish
Copy link

skurvish commented Nov 21, 2019

Current github and my installed 3.9 version. Problem I am having is that I am running a namespaced WebApplication which I have loaded the framework (included defines.php & framework.php), created a class of WebApplication. If an error happens in Mail.php it is calling \JError which is not found.

Perhaps there is some other autoloader I need to call beforehand so that JError is found?

@wilsonge
Copy link
Contributor

In J3 it is expected JError exists. We're only removing it in J4. You should be covered by the framework.php include which in turn loads the autoloaders in libraries/import.legacy.php which should autoload everything in libraries/legacy

@skurvish
Copy link

Hmmm, but it doesn't seem to autoload JError. Where should I look for answers? I assume this area is solely for J4.

@wilsonge
Copy link
Contributor

This Pull Request was to the J4 branch (hence why we assumed you were referring to J4 in your comment here). If you create a fresh issue and drop in a stack trace of the issue your getting we should be able to help more :)

@skurvish
Copy link

OK. I fixed the reason JError was being called so will have to revert to the state which caused the error and then will post a fresh issue. Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants