Routing: using the dependency injected $app and $menu in the core routers #5168

Closed
wants to merge 2 commits into
from

Conversation

Projects
None yet
5 participants
@Hackwar
Member

Hackwar commented Nov 22, 2014

In one of the last routing PRs, the JApplicationCms and JMenu objects were handed over to the constructor of the component routers. This allows to have test-objects be injected into the routers or to change the original objects where necessary.

This PR implements this change in the core routers by modifying the JComponentRouterBase class and subsequently changing the code in the core routers to use the new objects of the class.

I've been trying to decide if this is backwards compatible for a while longer. Not so much because the core routers are slightly changed, but because JComponentRouterBase is changed in that it gets a constructor and that this constructor accepts 2 parameters. If a developer wrote his own router class which extends from JComponentRouterBase and which has its own constructor, but does not call the parent constructor, $this->app and $this->menu would not be available to the router. But then again, those routers would not use $this->app and $this->menu in their code anyway. The only way this could go wrong, would be if a developer extends a core router and has his own constructor written for it, that does not call the parent constructor. This is a VERY unlikely construct and I would say that this is equally possible to winning the lottery and being struck by lightning at the same time. So I think we are safe here. (I'm just trying to cover all our bases here.)

Testing instructions: Apply the change and simply see that nothing is broken.

This was made possible through the generous donation of the people mentioned in the following link via an Indiegogo campaign: http://joomlager.de/crowdfunding/5-contributors

@dgrammatiko

This comment has been minimized.

Show comment
Hide comment
@dgrammatiko

dgrammatiko Nov 22, 2014

Member

@test success Works like a charm 👍

Member

dgrammatiko commented Nov 22, 2014

@test success Works like a charm 👍

libraries/cms/component/router/base.php
+ *
+ * @since 3.4
+ */
+ public function __construct($app = false, $menu = false)

This comment has been minimized.

@wilsonge

wilsonge Nov 23, 2014

Contributor

Just a personal thing but I'd rather the defaults be null - it's more consistent with most the rest of the Joomla classes.

@wilsonge

wilsonge Nov 23, 2014

Contributor

Just a personal thing but I'd rather the defaults be null - it's more consistent with most the rest of the Joomla classes.

This comment has been minimized.

@Hackwar

Hackwar Nov 23, 2014

Member

Will change that. I was also thinking about type-hinting here, but decided against it. Testing will be easier without it, I think...

@Hackwar

Hackwar Nov 23, 2014

Member

Will change that. I was also thinking about type-hinting here, but decided against it. Testing will be easier without it, I think...

This comment has been minimized.

@wilsonge

wilsonge Nov 23, 2014

Contributor

Well I don't see any of these router classes actually using JApplication - they've only been using it to get the menu object? So I'm not quite clear why we are injecting it anyhow? I'd certainly typehint the JMenu class. And as I dunno what the use case is for the application couldn't really comment as to whether you need to typehint that or not

@wilsonge

wilsonge Nov 23, 2014

Contributor

Well I don't see any of these router classes actually using JApplication - they've only been using it to get the menu object? So I'm not quite clear why we are injecting it anyhow? I'd certainly typehint the JMenu class. And as I dunno what the use case is for the application couldn't really comment as to whether you need to typehint that or not

@@ -27,16 +27,8 @@ class FinderRouter extends JComponentRouterBase
*/
public function build(&$query)
{
- static $menu;

This comment has been minimized.

@wilsonge

wilsonge Nov 23, 2014

Contributor

Are there any implications of this no longer being a static?

@wilsonge

wilsonge Nov 23, 2014

Contributor

Are there any implications of this no longer being a static?

This comment has been minimized.

@Hackwar

Hackwar Nov 23, 2014

Member

No, the router object is instantiated once per pageload and the menu object is set then. This static var was only necessary because we had no classes/objects for routers, but simple functions. Since the static wasn't accessible from outside the function anyway, there are also no B/C issues here.

@Hackwar

Hackwar Nov 23, 2014

Member

No, the router object is instantiated once per pageload and the menu object is set then. This static var was only necessary because we had no classes/objects for routers, but simple functions. Since the static wasn't accessible from outside the function anyway, there are also no B/C issues here.

@wilsonge

This comment has been minimized.

Show comment
Hide comment
@wilsonge

wilsonge Nov 23, 2014

Contributor

Why are we injecting the application object? It's not being used by any of these classes except to get the menu object?

Contributor

wilsonge commented Nov 23, 2014

Why are we injecting the application object? It's not being used by any of these classes except to get the menu object?

@Hackwar

This comment has been minimized.

Show comment
Hide comment
@Hackwar

Hackwar Nov 23, 2014

Member

Yes, so far that is true, but I wanted to be proactive and have this available, among other things to have access to the application configuration. Think of it as "Advanced SEF" not per component but globally.

Member

Hackwar commented Nov 23, 2014

Yes, so far that is true, but I wanted to be proactive and have this available, among other things to have access to the application configuration. Think of it as "Advanced SEF" not per component but globally.

@wilsonge

This comment has been minimized.

Show comment
Hide comment
@wilsonge

wilsonge Nov 23, 2014

Contributor

Fair enough 😄

Contributor

wilsonge commented Nov 23, 2014

Fair enough 😄

@chivitli

This comment has been minimized.

Show comment
Hide comment
@chivitli

chivitli Nov 24, 2014

Contributor

+1 from me. I have a component with router that extends JComponentRouterBase, everything works fine still.

Contributor

chivitli commented Nov 24, 2014

+1 from me. I have a component with router that extends JComponentRouterBase, everything works fine still.

@Hackwar

This comment has been minimized.

Show comment
Hide comment
@Hackwar

Hackwar Nov 24, 2014

Member

That makes it 2 successfull tests and a happy @wilsonge. Could someone (George?) set this to RTC? I don't want to do it myself, since I provided the patch...

Member

Hackwar commented Nov 24, 2014

That makes it 2 successfull tests and a happy @wilsonge. Could someone (George?) set this to RTC? I don't want to do it myself, since I provided the patch...

@wilsonge

This comment has been minimized.

Show comment
Hide comment
@wilsonge

wilsonge Nov 24, 2014

Contributor

RTC

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

Contributor

wilsonge commented Nov 24, 2014

RTC

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

@Hackwar

This comment has been minimized.

Show comment
Hide comment
@Hackwar

Hackwar Nov 27, 2014

Member

thx

Member

Hackwar commented Nov 27, 2014

thx

@Hackwar Hackwar deleted the Hackwar:routingappmenu branch Dec 10, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment