? ? ? Pending

User tests: Successful: Unsuccessful:

avatar yvesh
yvesh
19 Mar 2017

Summary of Changes

Lightweight approach making the dispatcher namespace aware as an entry point for loading the controller injecting the app and the input..

Testing Instructions

RFC and WIP.. Test that namespaces in Controllers are now working and non-namespaced Controllers are still working

Expected result

Namespaces working

Actual result

Namespacing in Dispatcher not working

Documentation Changes Required

Yes

avatar yvesh yvesh - open - 19 Mar 2017
avatar yvesh yvesh - change - 19 Mar 2017
Status New Pending
avatar joomla-cms-bot joomla-cms-bot - change - 19 Mar 2017
Category SQL Administration com_admin Postgresql Installation Libraries
avatar yvesh yvesh - change - 19 Mar 2017
Title
Modified the dispatcher to support namespaced extensions (Lightweight Approach)
[4.0] Modified the dispatcher to support namespaced extensions (Lightweight Approach)
avatar yvesh yvesh - edited - 19 Mar 2017
c0dc4fa 19 Mar 2017 avatar yvesh cs
avatar yvesh yvesh - change - 19 Mar 2017
Labels Added: ?
avatar wilsonge
wilsonge - comment - 19 Mar 2017

Added docs about the Controller constructor b/c break here https://docs.joomla.org/Potential_backward_compatibility_issues_in_Joomla_4#Legacy_Controller Dispatcher docs still required

avatar yvesh yvesh - change - 19 Mar 2017
Labels Added: ?
avatar joomla-cms-bot joomla-cms-bot - change - 19 Mar 2017
Category SQL Administration com_admin Postgresql Installation Libraries SQL Administration com_admin Postgresql Installation Libraries Unit Tests
avatar mbabker
mbabker - comment - 19 Mar 2017

Unless this namespace column is going to be used for all extensions, it should not be added as a separate column to the database.

A separate JComponentHelper::getNamespace() method should not be added. Add the column to the query issued by JComponentHelper::load() and the property to JComponentRecord. The data will therefore be included in the cache and eliminate the need for extra queries just to get an extension namespace (JComponentHelper::getComponent('com_content')->namespace).

avatar wilsonge
wilsonge - comment - 19 Mar 2017

JComponentHelper will not be the only thing calling the dispatcher. This is going to be the public 'interface' for extensions to call another extension (hence why the dispatcher itself isn't namespaced). It allows e.g. a module to call a component.

avatar mbabker
mbabker - comment - 19 Mar 2017

JComponentHelper::load() is already generally preloading component records into memory anyway (and supports writing the result to cache so the database isn't being read for minimum 5 or 6 components on every request). Adding the property to JComponentRecord and the loader will be perfectly acceptable. Otherwise a separate JComponentHelper::getNamespace() with no caching WILL be a performance issue.

avatar yvesh yvesh - change - 19 Mar 2017
Labels Added: ?
avatar joomdonation
joomdonation - comment - 19 Mar 2017

+1 for using JComponentHelper::getComponent('com_content')->namespace to get namespace would be the right way. The reason Michael already mentioned, plus from my experience when I was working on Controllers, Models classes, look like we will have to get namespace of component from those classes ( to build Model, View, Table classes) as well, so using cache of already loading data would be better.

avatar wilsonge
wilsonge - comment - 19 Mar 2017

So we undeprecate passing in a parameter then? I think the dispatcher should be the only thing that should need to map a namespace to a path. Everything else inside a given component MVC layer is in the folder path and should be component and namespace aware.

avatar mbabker
mbabker - comment - 19 Mar 2017

Undeprecate passing a parameter where?

avatar wilsonge
wilsonge - comment - 19 Mar 2017

JComponentHelper::load

avatar mbabker
mbabker - comment - 19 Mar 2017

Well, it depends. Right now it can be pretty flaky and inconsistent (look at the code). The intent was to treat it similarly to JPluginHelper::load() and have all the component records loaded into memory/cache instead of potentially having to do one query per component.

So it has an advantage of there really is only one read operation required to load component data (params, namespace, enabled flag, etc.) into memory on each request. The disadvantage though with Joomla's current architecture is that you're loading a lot of unneeded data into memory (a single request is probably easily loading data from 4-5 components, but we have 32 of them in core). In comparison, not all plugins are getting dispatched on every request (legitimately a good chunk of them only are used on write operations or authentication), but the same data is loading into the static data store in JPluginHelper.

avatar wilsonge
wilsonge - comment - 19 Mar 2017

The big question here is are we happy requiring a JApplicationCms object for the controller. I don't think there are many people creating new instances so the b/c break there is limited. But there are going to be a decent number overloading the constructor.

avatar mbabker
mbabker - comment - 19 Mar 2017

For 4.0 you probably have to do it as optional. And you definitely can't change the parameter order (so it has to be the options array then the new objects).

Either that or you're going to have a pretty tricky B/C layer going through the constructors.

avatar yvesh
yvesh - comment - 19 Mar 2017

Yep it's a B/C break, but are many people using new instead of JController::getInstance for controllers (couldn't come up with one in core)? So is it worth it? :-)

avatar mbabker
mbabker - comment - 19 Mar 2017

Many may not be direct instantiating themselves, but for controllers with extended constructors that call the parent constructor, it does become an issue (because ultimately JControllerLegacy::getInstance() calls new ContentControllerArticles($options); (https://github.com/joomla/joomla-cms/blob/staging/administrator/components/com_content/controllers/articles.php)).

avatar yvesh
yvesh - comment - 19 Mar 2017

Hmm okay, so we make it optional? (Again, had that in the beginning..) and keep the ordering $config = array(), $app = null, $input = null .. :-)

avatar wilsonge
wilsonge - comment - 19 Mar 2017

getInstance isn't a problem because we are forceably injecting JFactory::getApplication() there. So I don't think there is a problem instantiating a controller with this b/c break.

I think the b/c problem is the number of extensions calling parent::__construct for example to register a custom task.

avatar wilsonge wilsonge - change - 20 Mar 2017
Status Pending Fixed in Code Base
Closed_Date 0000-00-00 00:00:00 2017-03-20 01:16:37
Closed_By wilsonge
avatar wilsonge wilsonge - close - 20 Mar 2017
avatar wilsonge wilsonge - merge - 20 Mar 2017
avatar wilsonge
wilsonge - comment - 20 Mar 2017

This will do for round 2 of refactoring

Add a Comment

Login with GitHub to post a comment