-
-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
Support alias overrides and overrides for namespaced classes #14822
Support alias overrides and overrides for namespaced classes #14822
Conversation
One general question: Are we doing any single class loading in 4? or are we going to use the composer autoloader on the whole folder? |
As for the current set up composer does load the whole folder https://github.com/joomla/joomla-cms/blob/3.8-dev/composer.json#L22. Why? |
Lazy loading, from a dev's perspective ;) |
Because do we need JLoader than any longer? And @dgt41 this is implied :P |
Now I get it. Don't know if composer offers that alias support. |
Not sure either, overriding is a bit against some principles (extend not override SOLID (the o)), so there is probably no easy way What you can do definitely is namespace overriding.. But this is not always just one class :-/ https://getcomposer.org/apidoc/1.0.0/Composer/Autoload/ClassLoader.html |
Changed the pr the way, that the Joomla namespace in src is loaded trough |
@@ -246,7 +246,7 @@ public static function import($key, $base = null) | |||
public static function load($class) | |||
{ | |||
// Sanitize class name. | |||
$class = strtolower($class); | |||
$key = strtolower($class); | |||
|
|||
// If the class already exists do nothing. | |||
if (class_exists($class, false)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this if here really needed? I mean when it tries to load the class, it will not exist anyway.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If it get's called a second time in the code will it not run through this?
Thanks for being patient with this. i know I've held you up on this for nearly two weeks! |
…14822) * Add override plugin * Support overrides of namespaced classes * Better function name * Move override plugin to rep * Revert to none namespaced version as on staging * Load the Joomla namespace trough JLoader * JLoader alis load class support * CS * Only load the alias when the original class exists * One more space, outch * Add unit tests * Add note about use of composer
Summary of Changes
A task on the roadmap of Joomla 4 is to namespace the libraries and extensions in the core. This move is complicated as we don't want to break all extensions and make a very hard BC break in J4.
A target should be to make the transition for the core and the extension developers as painless as possible. The most important point is that the extension developers are not forced to develop their extensions from scratch or have to maintain two code bases for J3 and J4.
The approach
The approach is to namespace the classes in the libraries/joomla and libraries/cms folder like in this pr #14155 and to create an alias for the none namespaced class in a classmap file. For example
JModelList
can still be used but it is proxied toJoomla\Cms\Model\ListModel
. It is planed to do that for 3.8, that the extension developers can use the same code base for their extensions in the Joomla 3 series and in Joomla 4.At the same time we will carry on the none namespaced aliases to J4, which means that any none namespaced extension will run on J4.
What's the problem?
JLoader, which is the Joomla class loader, has a little feature which does allow to override core classes. More details about that topic can be found here. This is problematic for the move to namespaces. As we have now a different name for a class.
If an extension is overriding
JModelList
, then it would mean we can't change any code in core of Joomla 3.8 to useJoomla\Cms\Model\ListModel
. It is also not possible for any extension to run namespaced in 3.8, otherwise it would not be compatible with extensions which do overrides core classes. This will end up that the extension dev (if wants to use the same code base) needs to use the none namespaced version also in Joomla 4. But then it is again not compatible with the extensions which do override the namespaced core classes in J4.If we say, ok, then lets just make it possible to override the namespaced version only, we are breaking BC for 3.8.
The solution
The class loader needs to be able to handle overrides for aliases. Assume an extension is overriding
JModelList
. If the code or an extension uses the classJModelList
orJoomla\Cms\Model\ListModel
, then in both cases the override ofJModelList
needs to be used. The same goes when an override is registered forJoomla\Cms\Model\ListModel
. Then code which is usingJModelList
orJoomla\Cms\Model\ListModel
in core or an extension must use the namespaced override class.Example
I'v prepared an override plugin in a test repo which does override the list model classes. Basically it does:
It can be downloaded from here. If you install and activate that plugin on the 3.8 branch with this patch, then you should see a message which shows that the none namespaced override is called on every list model in the back end
If you do the changes from the test instructions in the file components/com_content/models/articles.php to use the namespaced list model, then you should see the following message when you go to Content -> Articles
The plugin has settings which do allow to control if the namespaced or none namespaced override should be registered.
THIS PLUGIN IS ONLY FOR DEMONSTRATION PURPOSES.
What is done in this pr?
JLoader
and not anymore trough composer. Like that we keep the two worlds separated. Composer is managing the external libraries (vendor dir) whileJLoader
is managing the core classes. So we don't have to introduce class overriding in composer.JLoader::load()
function where an if check never returns true because it makes no sense to do a test on a lower case class name.JLoader::stripFirstBackslash()
.Testing Instructions
class ContentModelArticles extends \Joomla\Cms\Model\ListModel
Case 1
Set Override JModelList to yes and Override Joomla\CMS\Model\ListModel to yes.
Case 2
Set Override JModelList to no and Override Joomla\CMS\Model\ListModel to yes.
Case 3
Set Override JModelList to yes and Override Joomla\CMS\Model\ListModel to no.
Case 4
Set Override JModelList to no and Override Joomla\CMS\Model\ListModel to no.
Expected result
Case 1
On every list in the back end the green message box should be displayed except on the articles list the yellow box for the ContentModelArticles should be displayed.
Case 2
On every list in the back end the yellow message box should be displayed.
Case 3
On every list in the back end the green message box should be displayed.
Case 4
No override message box should be displayed.
Actual result
Case 1
On every list in the back end the green message box should be displayed except on the articles list, there no box should be displayed.
Case 2
On every list in the back end no message box should be displayed.
Case 3
On every list in the back end the green message box should be displayed except on the articles list, there no box should be displayed.
Case 4
No override message box should be displayed.
Deprecation
This changes are basically needed for Joomla 3.8. If we want to support only overrides for the namespaced version, we can then revert back some changes in Joomla 4.
If we want to completely get rid of overriding core classes, then we can deprecate it right now in
JLoader
and remove the code in Joomla 5 at earliest then.Performance
There is a minimal performance loss in the composer classloader wrapper, as an extra lookup is made to detect if an override exists in JLoader for a namespaced class.
Warning
This pr should not encourage people to do class overrides as it is NOT the right approach to hook itself into the core!! It just deals with a feature which interferes with the transition to namespaces.