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

Routing: JComponentRouterRulesMenu to find right Itemid #5446

Closed
wants to merge 52 commits into
base: staging
from

Conversation

Projects
None yet
10 participants
@Hackwar
Member

Hackwar commented Dec 16, 2014

This rule implements finding the right Itemid as a generic rule for all component routers that extend from JComponentRouterView. It makes most of the parts of the *HelperRoute classes obsolete.

How to test

  • Apply this change
  • Change the router of com_content by replacing
class ContentRouter extends JComponentRouterBase
{

with

class ContentRouter extends JComponentRouterView
{
    function __construct($app = null, $menu = null) {

        $categories = new JComponentRouterViewconfiguration('categories');
        $categories->setKey('id');
        $this->registerView($categories);
        $category = new JComponentRouterViewconfiguration('category');
        $category->setKey('id')->setParent($categories, 'id')->setNestable()->addLayout('blog');
        $this->registerView($category);
        $article = new JComponentRouterViewconfiguration('article');
        $article->setKey('id')->setParent($category, 'catid');
        $this->registerView($article);
        $this->registerView(new JComponentRouterViewconfiguration('archive'));
        $this->registerView(new JComponentRouterViewconfiguration('featured'));
        $this->registerView(new JComponentRouterViewconfiguration('form'));

        parent::__construct($app, $menu);
        $this->attachRule(new JComponentRouterRulesMenu($this));
    }

    public function getCategorySegment($id, $query)
    {
        $category = JCategories::getInstance($this->getName())->get($id);
        if ($category)
        {
            return array_reverse($category->getPath());
        }

        return array();
    }

    public function getCategoriesSegment($id, $query)
    {
        return $this->getCategorySegment($id, $query);
    }

    public function getArticleSegment($id, $query)
    {
        return array($id);
    }
  • Comment line 62 and 111 from /components/com_content/helpers/route.php to disable the lookup of the Itemid there.
  • See that the right menu items are still found for the items

This was updated on May 22nd 2015. All discussions prior to that point have either been integrated or are obsolete.

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

@johanjanssens

This comment has been minimized.

Show comment
Hide comment
@johanjanssens

johanjanssens Dec 16, 2014

@Hackwar By having this method your are coupling the categories implementation to the routing API. Do we need to have a this method here ? Wouldn't this be something either for a more specialised router that understands categories, or for a specific implementation in a component router ?

@Hackwar By having this method your are coupling the categories implementation to the routing API. Do we need to have a this method here ? Wouldn't this be something either for a more specialised router that understands categories, or for a specific implementation in a component router ?

This comment has been minimized.

Show comment
Hide comment
@Hackwar

Hackwar Dec 17, 2014

Owner

I'm indeed pondering if this method should be there or not. As I wrote in the description, I'm interested in having get methods to retrieve the necessary data and this would be one of those methods. Right now, most of our core frontend components need this method and a lot of third party extensions work with categories, too. So I could either have this here or implement another class between this one and the actual router to simply implement that class. Trying to strike a balance here between abstraction and over-engineering, I put it in this class. Also, I could quite frankly not come up with yet another class name for such an intermediate class. 😉

Now, I would leave it at that, you would move it into its own class, so I would call for a third party to cast a vote what to do. 😄

Owner

Hackwar replied Dec 17, 2014

I'm indeed pondering if this method should be there or not. As I wrote in the description, I'm interested in having get methods to retrieve the necessary data and this would be one of those methods. Right now, most of our core frontend components need this method and a lot of third party extensions work with categories, too. So I could either have this here or implement another class between this one and the actual router to simply implement that class. Trying to strike a balance here between abstraction and over-engineering, I put it in this class. Also, I could quite frankly not come up with yet another class name for such an intermediate class. 😉

Now, I would leave it at that, you would move it into its own class, so I would call for a third party to cast a vote what to do. 😄

This comment has been minimized.

Show comment
Hide comment
@johanjanssens

johanjanssens Dec 25, 2014

Could these methods not be exposed by the rules JComponentRouterRulesCategory object. If they need to be accessible through the router you could use magic calls to get them making the rules object work like a mixin. That gives you flexibility without cluttering the initial router.

johanjanssens replied Dec 25, 2014

Could these methods not be exposed by the rules JComponentRouterRulesCategory object. If they need to be accessible through the router you could use magic calls to get them making the rules object work like a mixin. That gives you flexibility without cluttering the initial router.

This comment has been minimized.

Show comment
Hide comment
@Hackwar

Hackwar Dec 25, 2014

Owner

No, the get"ViewName" methods need to be accessible from inside other rules and thus have to be implemented in the router. If you, as a dev that needs to modify an existing routers, need other methods (for example when you add your own view to a component) you can create your own router which extends from the component router and set it with JRouterSite::setComponentRouter() as a replacement for the original one.

I thought about using magic calls, but I have the feeling that this complicates the implementation again and makes it less intuitive. Joomla devs are not used to that...

Owner

Hackwar replied Dec 25, 2014

No, the get"ViewName" methods need to be accessible from inside other rules and thus have to be implemented in the router. If you, as a dev that needs to modify an existing routers, need other methods (for example when you add your own view to a component) you can create your own router which extends from the component router and set it with JRouterSite::setComponentRouter() as a replacement for the original one.

I thought about using magic calls, but I have the feeling that this complicates the implementation again and makes it less intuitive. Joomla devs are not used to that...

@johanjanssens

This comment has been minimized.

Show comment
Hide comment
@johanjanssens

johanjanssens Dec 16, 2014

@Hackwar Would consider calling this method registerView() instead of register to better declare it's purpose. All other view related methods in the proposed API also have 'View' in their name which makes it easier to see the correlation between the methods.

@Hackwar Would consider calling this method registerView() instead of register to better declare it's purpose. All other view related methods in the proposed API also have 'View' in their name which makes it easier to see the correlation between the methods.

This comment has been minimized.

Show comment
Hide comment
@Hackwar

Hackwar Dec 17, 2014

Owner

Will do.

Owner

Hackwar replied Dec 17, 2014

Will do.

This comment has been minimized.

Show comment
Hide comment
@johanjanssens

johanjanssens replied Dec 25, 2014

Great!

@johanjanssens

This comment has been minimized.

Show comment
Hide comment
@johanjanssens

johanjanssens Dec 16, 2014

@Hackwar see my other comment about the getCategory() method. If you don't add it you also don't need to add the getName() method. Additionally you could ask yourself if this method has use outside of the implementation as proposed in getCategory(). If not it might be better to remove this and implement the logic in getCategory().

@Hackwar see my other comment about the getCategory() method. If you don't add it you also don't need to add the getName() method. Additionally you could ask yourself if this method has use outside of the implementation as proposed in getCategory(). If not it might be better to remove this and implement the logic in getCategory().

This comment has been minimized.

Show comment
Hide comment
@Hackwar

Hackwar Dec 17, 2014

Owner

This method has a use outside of JCategories. It is also used to find the right menu items for the lookup and for other filter tasks that we have. Yes, it is named like our magic getters, but consider it one of the reserved words that can not be used as a name for a view.

Owner

Hackwar replied Dec 17, 2014

This method has a use outside of JCategories. It is also used to find the right menu items for the lookup and for other filter tasks that we have. Yes, it is named like our magic getters, but consider it one of the reserved words that can not be used as a name for a view.

This comment has been minimized.

Show comment
Hide comment
@johanjanssens

johanjanssens Dec 25, 2014

In that case, maybe consider giving the method a protected scope and renaming it to getRouterName() to allow the router to have a getName() method if it needs to ? Otherwise this becomes a need to know that you cannot expose a 'name' property in your routes ?

johanjanssens replied Dec 25, 2014

In that case, maybe consider giving the method a protected scope and renaming it to getRouterName() to allow the router to have a getName() method if it needs to ? Otherwise this becomes a need to know that you cannot expose a 'name' property in your routes ?

@johanjanssens

This comment has been minimized.

Show comment
Hide comment
@johanjanssens

johanjanssens Dec 16, 2014

@Hackwar By making the property public you are allowing it to be changed from the outside. Is that intentional ? Couldn't this result in unwanted side effects ?

@Hackwar By making the property public you are allowing it to be changed from the outside. Is that intentional ? Couldn't this result in unwanted side effects ?

This comment has been minimized.

Show comment
Hide comment
@Hackwar

Hackwar Dec 17, 2014

Owner

That is indeed unintentional. How could that slip through? Thanks, will change.

Owner

Hackwar replied Dec 17, 2014

That is indeed unintentional. How could that slip through? Thanks, will change.

This comment has been minimized.

Show comment
Hide comment
@johanjanssens

johanjanssens replied Dec 25, 2014

Thanks!

@johanjanssens

This comment has been minimized.

Show comment
Hide comment
@johanjanssens

johanjanssens Dec 16, 2014

@Hackwar Bit of a code smell here. You should try to check against and interface and not an implementation. This gives developers more flexibility, Eg would be better if it would be

public function __construct(JComponentRouterAdvancedInterface $router)

@Hackwar Bit of a code smell here. You should try to check against and interface and not an implementation. This gives developers more flexibility, Eg would be better if it would be

public function __construct(JComponentRouterAdvancedInterface $router)

This comment has been minimized.

Show comment
Hide comment
@Hackwar

Hackwar Dec 17, 2014

Owner

I choose the concrete implementation, because the rules are pretty tightly coupled to this class. I also again tried to find a balance between abstraction and over-engineering. My heart is not attached to this, but I'd rather not add another folder to this one simply to stuff the interface in there for the autoloader...

As with the last cases, I'm happy to be overruled by similar opinions.

Owner

Hackwar replied Dec 17, 2014

I choose the concrete implementation, because the rules are pretty tightly coupled to this class. I also again tried to find a balance between abstraction and over-engineering. My heart is not attached to this, but I'd rather not add another folder to this one simply to stuff the interface in there for the autoloader...

As with the last cases, I'm happy to be overruled by similar opinions.

This comment has been minimized.

Show comment
Hide comment
@johanjanssens

johanjanssens Dec 25, 2014

Typo, should have been public function __construct(JComponentRouterInterface $router)Additionally would consider adding this to the JComponentRouterRulesInterface to enforce the constructor.

johanjanssens replied Dec 25, 2014

Typo, should have been public function __construct(JComponentRouterInterface $router)Additionally would consider adding this to the JComponentRouterRulesInterface to enforce the constructor.

This comment has been minimized.

Show comment
Hide comment
@Hackwar

Hackwar Dec 25, 2014

Owner

JComponentRouterInterface does not have the interface that is required by the rules. You definitely need an interface like JComponentRouterAdvanced. Remember that this advanced router is optional. If a component doesn't want it, we are not forcing it upon the devs. So JComponentRouterInterface does not have the features that are required for the advanced component router.

Owner

Hackwar replied Dec 25, 2014

JComponentRouterInterface does not have the interface that is required by the rules. You definitely need an interface like JComponentRouterAdvanced. Remember that this advanced router is optional. If a component doesn't want it, we are not forcing it upon the devs. So JComponentRouterInterface does not have the features that are required for the advanced component router.

This comment has been minimized.

Show comment
Hide comment
@johanjanssens

johanjanssens Jan 13, 2015

@Hackwar I understand, it would still be good to have an interface for this so we don't need to work against an implementation. Can we move the API as defined in the advanced router into an interface ?

johanjanssens replied Jan 13, 2015

@Hackwar I understand, it would still be good to have an interface for this so we don't need to work against an implementation. Can we move the API as defined in the advanced router into an interface ?

@johanjanssens

This comment has been minimized.

Show comment
Hide comment
@johanjanssens

johanjanssens Dec 16, 2014

@Hackwar Do you want to allow this ? By letting any Itemid being passed in a developer can hijack a route. Connecting a component to any random Itemid. Shouldn't that be prevented ?

@Hackwar Do you want to allow this ? By letting any Itemid being passed in a developer can hijack a route. Connecting a component to any random Itemid. Shouldn't that be prevented ?

This comment has been minimized.

Show comment
Hide comment
@Hackwar

Hackwar Dec 17, 2014

Owner

This is necessary for the menu system to work. The menu module hands in "index.php?Itemid=XX" and this is sent through preprocess. If we don't allow Itemids to be set from the outside, we will get an issue here. So I've allowed that.

Owner

Hackwar replied Dec 17, 2014

This is necessary for the menu system to work. The menu module hands in "index.php?Itemid=XX" and this is sent through preprocess. If we don't allow Itemids to be set from the outside, we will get an issue here. So I've allowed that.

This comment has been minimized.

Show comment
Hide comment
@johanjanssens

johanjanssens replied Dec 25, 2014

Oki.

@johanjanssens

This comment has been minimized.

Show comment
Hide comment
@johanjanssens

johanjanssens Dec 16, 2014

@Hackwar As commented before. If the you need dummy methods here you can ask yourself if you really need those methods in the interface at all ?

@Hackwar As commented before. If the you need dummy methods here you can ask yourself if you really need those methods in the interface at all ?

This comment has been minimized.

Show comment
Hide comment
@Hackwar

Hackwar Dec 17, 2014

Owner

My first implementation did not have an interface but instead you could simply hand in callbacks for rules. You also had to manually assign them to parse and build rules. That was before I introduced preprocess and before I expected the rule to be an instantiated object with the router handed in through the constructor. That said, I decided against the ultra-flexible solution, since I think it is more error-prone for third party developers and choose this interface. It requires less methods to register rules and less code to store and execute them and at the same time gives devs a construct to work from.

Owner

Hackwar replied Dec 17, 2014

My first implementation did not have an interface but instead you could simply hand in callbacks for rules. You also had to manually assign them to parse and build rules. That was before I introduced preprocess and before I expected the rule to be an instantiated object with the router handed in through the constructor. That said, I decided against the ultra-flexible solution, since I think it is more error-prone for third party developers and choose this interface. It requires less methods to register rules and less code to store and execute them and at the same time gives devs a construct to work from.

This comment has been minimized.

Show comment
Hide comment
@johanjanssens

johanjanssens Dec 25, 2014

Not following you here. Does that mean you agree in getting it out of the interface or not ?

johanjanssens replied Dec 25, 2014

Not following you here. Does that mean you agree in getting it out of the interface or not ?

This comment has been minimized.

Show comment
Hide comment
@Hackwar

Hackwar Dec 25, 2014

Owner

This means that I think the interface is good the way it is.

Owner

Hackwar replied Dec 25, 2014

This means that I think the interface is good the way it is.

@johanjanssens

This comment has been minimized.

Show comment
Hide comment
@johanjanssens

johanjanssens Dec 16, 2014

@Hackwar As commented before. If the you need dummy methods here you can ask yourself if you really need those methods in the interface at all ?

@Hackwar As commented before. If the you need dummy methods here you can ask yourself if you really need those methods in the interface at all ?

This comment has been minimized.

Show comment
Hide comment
@Hackwar

Hackwar Dec 17, 2014

Owner

See my previous comment. 😄

Owner

Hackwar replied Dec 17, 2014

See my previous comment. 😄

@johanjanssens

This comment has been minimized.

Show comment
Hide comment
@johanjanssens

johanjanssens Dec 16, 2014

@Hackwar Since you are using the result of buildLookup() in this code block I would make buildLookup() return the lookup array. It can still cache it but it makes sense it returns it for use in the code block here.

@Hackwar Since you are using the result of buildLookup() in this code block I would make buildLookup() return the lookup array. It can still cache it but it makes sense it returns it for use in the code block here.

This comment has been minimized.

Show comment
Hide comment
@Hackwar

Hackwar Dec 17, 2014

Owner

This might sound stupid, but I wanted to optimize away a function-call here. I also choose the direct access of the lookup property in the code further down in the hopes that it saves a bit of memory and time instead of copying a pointer each time into a new variable in the methods scope etc. Yes, it might be a bit over-optimized, but I would stick to it.

However I see a bug in there, that the $language is not correctly set if the lookup is already created and the query language parameter is set. Will have to split that into 2 ifs.

Owner

Hackwar replied Dec 17, 2014

This might sound stupid, but I wanted to optimize away a function-call here. I also choose the direct access of the lookup property in the code further down in the hopes that it saves a bit of memory and time instead of copying a pointer each time into a new variable in the methods scope etc. Yes, it might be a bit over-optimized, but I would stick to it.

However I see a bug in there, that the $language is not correctly set if the lookup is already created and the query language parameter is set. Will have to split that into 2 ifs.

This comment has been minimized.

Show comment
Hide comment
@johanjanssens

johanjanssens Dec 25, 2014

I would choose code readability over the few microseconds you might be saving here. Especially since you are already caching.

johanjanssens replied Dec 25, 2014

I would choose code readability over the few microseconds you might be saving here. Especially since you are already caching.

@johanjanssens

This comment has been minimized.

Show comment
Hide comment
@johanjanssens

johanjanssens Dec 16, 2014

@Hackwar Not sure about the proposed function name here. getMenuItemMap() might be a better name to reflect what this method is doing. I would also not make it return void but return the result instead. The result could be filtered by the language passed in through the method parameter.

@Hackwar Not sure about the proposed function name here. getMenuItemMap() might be a better name to reflect what this method is doing. I would also not make it return void but return the result instead. The result could be filtered by the language passed in through the method parameter.

This comment has been minimized.

Show comment
Hide comment
@Hackwar

Hackwar Dec 17, 2014

Owner

Its the name that we used so far in the *HelperRoute classes for this. I simply choose it for the sake of familiarity with those methods.

Owner

Hackwar replied Dec 17, 2014

Its the name that we used so far in the *HelperRoute classes for this. I simply choose it for the sake of familiarity with those methods.

This comment has been minimized.

Show comment
Hide comment
@johanjanssens

johanjanssens Dec 25, 2014

Since you are adding it new here and we are refactoring and removing the HelperRoute classes anyway, I would consider renaming it to benefit a clearer API.

johanjanssens replied Dec 25, 2014

Since you are adding it new here and we are refactoring and removing the HelperRoute classes anyway, I would consider renaming it to benefit a clearer API.

@johanjanssens

This comment has been minimized.

Show comment
Hide comment
@johanjanssens

johanjanssens Dec 16, 2014

@Hackwar Getting the active menu item is a common operation in a router. Could consider adding a getActiveMenuItem() method to return this.

@Hackwar Getting the active menu item is a common operation in a router. Could consider adding a getActiveMenuItem() method to return this.

This comment has been minimized.

Show comment
Hide comment
@Hackwar

Hackwar Dec 17, 2014

Owner

Sorry, but I would call that over-engineered code. The method would simply contain this one line and that is it and I don't see any benefit in that.

Owner

Hackwar replied Dec 17, 2014

Sorry, but I would call that over-engineered code. The method would simply contain this one line and that is it and I don't see any benefit in that.

This comment has been minimized.

Show comment
Hide comment
@johanjanssens

johanjanssens Dec 25, 2014

I don't see why that would be over engineering. This is the L in SOLID, or the Liskov substitution principle and Design by Contract Those are well established programming principles.

By not exposing the getter you are relying on implementation instead of on an interface. Having a getActiveMenuItem() or getActiveMenu() as a getter creates a declarative API and make the code more easily extendable. It also better communicates the intention of the API by having the method as part of it.

I would still reconsider this and try to stick to SOLID as much as possible when creating new API's.

johanjanssens replied Dec 25, 2014

I don't see why that would be over engineering. This is the L in SOLID, or the Liskov substitution principle and Design by Contract Those are well established programming principles.

By not exposing the getter you are relying on implementation instead of on an interface. Having a getActiveMenuItem() or getActiveMenu() as a getter creates a declarative API and make the code more easily extendable. It also better communicates the intention of the API by having the method as part of it.

I would still reconsider this and try to stick to SOLID as much as possible when creating new API's.

@johanjanssens

This comment has been minimized.

Show comment
Hide comment
@johanjanssens

johanjanssens Dec 16, 2014

@Hackwar Getting the default menu item is a common operation in a router. Could consider adding a getDefaultMenuItem() method to return this.

@Hackwar Getting the default menu item is a common operation in a router. Could consider adding a getDefaultMenuItem() method to return this.

This comment has been minimized.

Show comment
Hide comment
@Hackwar

Hackwar Dec 17, 2014

Owner

See my above note.

Owner

Hackwar replied Dec 17, 2014

See my above note.

This comment has been minimized.

Show comment
Hide comment
@johanjanssens

johanjanssens Dec 25, 2014

See my note. Please consider it.

johanjanssens replied Dec 25, 2014

See my note. Please consider it.

@johanjanssens

This comment has been minimized.

Show comment
Hide comment
@johanjanssens

johanjanssens Dec 16, 2014

@Hackwar Does this need to be accessible from object scope ? If not might consider making it private to prevent developers for using it directly ?

@Hackwar Does this need to be accessible from object scope ? If not might consider making it private to prevent developers for using it directly ?

This comment has been minimized.

Show comment
Hide comment
@Hackwar

Hackwar Dec 17, 2014

Owner

In general, rules should not be extended upon. They are self-contained pieces of code. But if someone really wants to, he/she should be able to mess around here. But since its not accessible from the component router, I would say that it is okay the way it is.

Owner

Hackwar replied Dec 17, 2014

In general, rules should not be extended upon. They are self-contained pieces of code. But if someone really wants to, he/she should be able to mess around here. But since its not accessible from the component router, I would say that it is okay the way it is.

This comment has been minimized.

Show comment
Hide comment
@johanjanssens

johanjanssens Dec 25, 2014

Oki, you could still consider making it private for now. If someone needs to access this is could be made protected easily. Having it private gives you less to worry about in case the code needs to be refactored later.

johanjanssens replied Dec 25, 2014

Oki, you could still consider making it private for now. If someone needs to access this is could be made protected easily. Having it private gives you less to worry about in case the code needs to be refactored later.

@johanjanssens

This comment has been minimized.

Show comment
Hide comment
@johanjanssens

johanjanssens Dec 16, 2014

Contributor

@Hackwar Did a first pass of the proposed code and added inline comments. I like the simplicity that is being proposed.

Contributor

johanjanssens commented Dec 16, 2014

@Hackwar Did a first pass of the proposed code and added inline comments. I like the simplicity that is being proposed.

@weeblr-dev

This comment has been minimized.

Show comment
Hide comment
@weeblr-dev

weeblr-dev Dec 17, 2014

Hi Hannes,

How can 3rd party extensions disable/bypass those rules that could be added by components, or even if they make their way int Joomla core pre-4.0? or at least are you storing the original request somewhere so that we can revert any change made by those rules, as discussed in #5264?

"By letting any Itemid being passed in a developer can hijack a route. Connecting a component to any random Itemid. Shouldn't that be prevented ? ": Not random, but manually selected by user - as allowed by most SEF extensions, yes, absolutely.

Cheers

weeblr-dev commented Dec 17, 2014

Hi Hannes,

How can 3rd party extensions disable/bypass those rules that could be added by components, or even if they make their way int Joomla core pre-4.0? or at least are you storing the original request somewhere so that we can revert any change made by those rules, as discussed in #5264?

"By letting any Itemid being passed in a developer can hijack a route. Connecting a component to any random Itemid. Shouldn't that be prevented ? ": Not random, but manually selected by user - as allowed by most SEF extensions, yes, absolutely.

Cheers

@Hackwar

This comment has been minimized.

Show comment
Hide comment
@Hackwar

Hackwar Dec 17, 2014

Member

I guess JComponentRouterAdvanced will get a "removeRule()" method, that takes a rule that you can first find with a "getRules()" method and remove that from the rules to process again. Remember that you will have the buildpreprocess() step, where you can attach yourself already to, to for example add your stuff to the query.

Member

Hackwar commented Dec 17, 2014

I guess JComponentRouterAdvanced will get a "removeRule()" method, that takes a rule that you can first find with a "getRules()" method and remove that from the rules to process again. Remember that you will have the buildpreprocess() step, where you can attach yourself already to, to for example add your stuff to the query.

@weeblr-dev

This comment has been minimized.

Show comment
Hide comment
@weeblr-dev

weeblr-dev Dec 17, 2014

This is not about attaching things to the query. This is about being able to undo things that other rules have attached, or any change they might have done to the original query. In other words not being trapped again in the problems the language filter plugins rules have created (not by themselves, but by being set in stone and unmodifiable)
getRules() and removeRules() sound good, as long as we have an event we can use to be reasonably sure all rules have been attached (so we can also be sure to remove them all). Too bad there isn't an onBeforeRoute...
I guess even if we use onAfterInitialise, we can rely on the plugins ordering so we should be good in most cases.

weeblr-dev commented Dec 17, 2014

This is not about attaching things to the query. This is about being able to undo things that other rules have attached, or any change they might have done to the original query. In other words not being trapped again in the problems the language filter plugins rules have created (not by themselves, but by being set in stone and unmodifiable)
getRules() and removeRules() sound good, as long as we have an event we can use to be reasonably sure all rules have been attached (so we can also be sure to remove them all). Too bad there isn't an onBeforeRoute...
I guess even if we use onAfterInitialise, we can rely on the plugins ordering so we should be good in most cases.

@Hackwar

This comment has been minimized.

Show comment
Hide comment
@Hackwar

Hackwar Dec 17, 2014

Member

For the time being, I would add all rules in the component routers constructors, so when you cycle through all components and then use JRouterSite::getComponentRouter() and do your own stuff by adding/removing rules again in onAfterInitialise, you would be good. Since I would like to have more than one set of component rules, I could also see us having a -1 option that disables all the rule-adding in the constructor and you can do your magic.

Regarding the language filter plugin: I hope that it is a lot saner now when #5140 is merged. I can't really think of anything else that you could want in that regard...

Member

Hackwar commented Dec 17, 2014

For the time being, I would add all rules in the component routers constructors, so when you cycle through all components and then use JRouterSite::getComponentRouter() and do your own stuff by adding/removing rules again in onAfterInitialise, you would be good. Since I would like to have more than one set of component rules, I could also see us having a -1 option that disables all the rule-adding in the constructor and you can do your magic.

Regarding the language filter plugin: I hope that it is a lot saner now when #5140 is merged. I can't really think of anything else that you could want in that regard...

@weeblr-dev

This comment has been minimized.

Show comment
Hide comment
@weeblr-dev

weeblr-dev Dec 18, 2014

Hi Hannes,
Let me clear that up first: I have no problem with the language filter rules, how they were working or whatever. The only issue was that we couldn't bypass them, or even know what the actual original request was and what the language filter rules had done to it. The same would apply to any extension adding rules, whether for parsing or building.
It was a bit more difficult with the language filter because its parse rule is always executed first, regardless of the actual system plugins ordering set by user or otherwise.

As for adding/removing rules on onAfterInitialise, I can request users to be sure my system plugin is the last one in order (and I can check that periodically) and thus be sure to remove all rules that may have been added by other extensions. An onBeforeRoute event would be more secure, but maybe not worth the added perf cost I guess. I'll make sure to do some trials in due time.

weeblr-dev commented Dec 18, 2014

Hi Hannes,
Let me clear that up first: I have no problem with the language filter rules, how they were working or whatever. The only issue was that we couldn't bypass them, or even know what the actual original request was and what the language filter rules had done to it. The same would apply to any extension adding rules, whether for parsing or building.
It was a bit more difficult with the language filter because its parse rule is always executed first, regardless of the actual system plugins ordering set by user or otherwise.

As for adding/removing rules on onAfterInitialise, I can request users to be sure my system plugin is the last one in order (and I can check that periodically) and thus be sure to remove all rules that may have been added by other extensions. An onBeforeRoute event would be more secure, but maybe not worth the added perf cost I guess. I'll make sure to do some trials in due time.

Changing component router to retrieve, since the com_content router i…
…s pretty complex and throws errors in our testing environment
@wilsonge

This comment has been minimized.

Show comment
Hide comment
@wilsonge

wilsonge Dec 24, 2014

Probably need to change the text there then :)

Probably need to change the text there then :)

@johanjanssens

This comment has been minimized.

Show comment
Hide comment
@johanjanssens

johanjanssens Dec 25, 2014

@Hackwar If the method to add a rule if called attachRule(), if would make sense that the method to remove it is called detachRule(), detach being the opposite of attach. Alternatively you could consider renaming attachRule to addRule().

@Hackwar If the method to add a rule if called attachRule(), if would make sense that the method to remove it is called detachRule(), detach being the opposite of attach. Alternatively you could consider renaming attachRule to addRule().

This comment has been minimized.

Show comment
Hide comment
@Hackwar

Hackwar Dec 25, 2014

Owner

What would you prefer? I can do either.

Owner

Hackwar replied Dec 25, 2014

What would you prefer? I can do either.

This comment has been minimized.

Show comment
Hide comment
@johanjanssens

johanjanssens Jan 12, 2015

Attach/Detach is mostly used in the context of an object. Since the rule is an object, attach/detach make most sense here. Does that help ?

johanjanssens replied Jan 12, 2015

Attach/Detach is mostly used in the context of an object. Since the rule is an object, attach/detach make most sense here. Does that help ?

@johanjanssens

This comment has been minimized.

Show comment
Hide comment
@johanjanssens

johanjanssens Dec 25, 2014

Contributor

@weeblr-dev @Hackwar Do we really need to have the ability to remove rules ? Removing rules requires knowledge about the rules being added. Wouldn't it make more sense to allow rules to override others ?

An idea can be to store the original query and do the query modifications into a different array. That way a rule can always use the original query and can also more easily override without needing to have knowledge about other rules.

Contributor

johanjanssens commented Dec 25, 2014

@weeblr-dev @Hackwar Do we really need to have the ability to remove rules ? Removing rules requires knowledge about the rules being added. Wouldn't it make more sense to allow rules to override others ?

An idea can be to store the original query and do the query modifications into a different array. That way a rule can always use the original query and can also more easily override without needing to have knowledge about other rules.

@Hackwar

This comment has been minimized.

Show comment
Hide comment
@Hackwar

Hackwar Dec 25, 2014

Member

That exactly would make it impossible to override rules. Right now, you can attach a rule to the router and clear out the query variables one by one, creating your own rule and in the end, your URL is done and the query variables are empty. Then the next part of the router only sees an empty query and directly returns. If we kept the original query, how would a later rule know that an earlier rule already processed that part?

Yes, removing rules requires you to have quite a knowledge about what has been added before, but the main situation where I see this to be used would be a plugin like sh404sef, which could remove the default behavior of Joomla and replace it with its own. It's not going to be a bunch of GUI options that let you influence that directly.

Member

Hackwar commented Dec 25, 2014

That exactly would make it impossible to override rules. Right now, you can attach a rule to the router and clear out the query variables one by one, creating your own rule and in the end, your URL is done and the query variables are empty. Then the next part of the router only sees an empty query and directly returns. If we kept the original query, how would a later rule know that an earlier rule already processed that part?

Yes, removing rules requires you to have quite a knowledge about what has been added before, but the main situation where I see this to be used would be a plugin like sh404sef, which could remove the default behavior of Joomla and replace it with its own. It's not going to be a bunch of GUI options that let you influence that directly.

@johanjanssens

This comment has been minimized.

Show comment
Hide comment
@johanjanssens

johanjanssens Jan 12, 2015

Attach/Detach is mostly used in the context of an object. Since the rule is an object, attach/detach make most sense here. Does that help ?

johanjanssens commented on 0309e22 Jan 12, 2015

Attach/Detach is mostly used in the context of an object. Since the rule is an object, attach/detach make most sense here. Does that help ?

@johanjanssens

This comment has been minimized.

Show comment
Hide comment
@johanjanssens

johanjanssens Jan 12, 2015

Contributor

@Hackwar About the remove rule comments you made above. I understand, care should be taken though that if no rules are there the URL would still work. Otherwise things could go very wrong, without a working router Joomla would no longer work.

A solution would be to ensure that if no rules are present we fallback to a none-SEF form. Is that already the case ? If no, might be good to include this scenario also in the unit tests.

Contributor

johanjanssens commented Jan 12, 2015

@Hackwar About the remove rule comments you made above. I understand, care should be taken though that if no rules are there the URL would still work. Otherwise things could go very wrong, without a working router Joomla would no longer work.

A solution would be to ensure that if no rules are present we fallback to a none-SEF form. Is that already the case ? If no, might be good to include this scenario also in the unit tests.

@johanjanssens

This comment has been minimized.

Show comment
Hide comment
@johanjanssens

johanjanssens Jan 12, 2015

@Hackwar I would consider using JComponentRouterView classname. Think we should avoid using class names that are not properly camelcased.

@Hackwar I would consider using JComponentRouterView classname. Think we should avoid using class names that are not properly camelcased.

@johanjanssens

This comment has been minimized.

Show comment
Hide comment
@johanjanssens

johanjanssens Jan 12, 2015

@hannes Good work. Feedback as requested :

  • If you have special setters it's best to have getters too and make the class variables protected or even private. Your implementation now allows for the class variables to be set directly which can have unwanted side-effects.
  • I would consider adding an isNestable() method to be able to check if the views is nestable or not.
  • What is the difference between setName() and setViewName() ? These methods are a bit confusing to me, is there a better way we can name them ?

johanjanssens commented on 68d9424 Jan 12, 2015

@hannes Good work. Feedback as requested :

  • If you have special setters it's best to have getters too and make the class variables protected or even private. Your implementation now allows for the class variables to be set directly which can have unwanted side-effects.
  • I would consider adding an isNestable() method to be able to check if the views is nestable or not.
  • What is the difference between setName() and setViewName() ? These methods are a bit confusing to me, is there a better way we can name them ?
@johanjanssens

This comment has been minimized.

Show comment
Hide comment
@johanjanssens

johanjanssens Jan 12, 2015

@Hackwar I would consider changing this to getCategoryIdyou are getting the category identifiers, not it's key. They key would be the name of the identifier, aka 'id'.

@Hackwar I would consider changing this to getCategoryIdyou are getting the category identifiers, not it's key. They key would be the name of the identifier, aka 'id'.

@Hackwar

This comment has been minimized.

Show comment
Hide comment
@Hackwar

Hackwar Jan 12, 2015

Member

@johanjanssens I specifically chose JComponentRouterViewconfiguration since I did not want to use JComponentRouterView for this. I can see us wanting to have a JComponentRouterView class for different purposes.

Regarding the setName/setViewName: Please see the latest commits to this PR. Those have since been removed.

Member

Hackwar commented Jan 12, 2015

@johanjanssens I specifically chose JComponentRouterViewconfiguration since I did not want to use JComponentRouterView for this. I can see us wanting to have a JComponentRouterView class for different purposes.

Regarding the setName/setViewName: Please see the latest commits to this PR. Those have since been removed.

@johanjanssens

This comment has been minimized.

Show comment
Hide comment
@johanjanssens

johanjanssens Jan 13, 2015

Contributor

@Hackwar About setName/setViewName() I missed those changes. Looking much better indeed API wise.

About JComponentRouterViewconfiguration vs JComponentRouterView. I understand, still not 100% happy with JComponentRouterViewconfiguration, any alternatives ? JComponentRouterConfig maybe, or JComponentRouterContext ?

Contributor

johanjanssens commented Jan 13, 2015

@Hackwar About setName/setViewName() I missed those changes. Looking much better indeed API wise.

About JComponentRouterViewconfiguration vs JComponentRouterView. I understand, still not 100% happy with JComponentRouterViewconfiguration, any alternatives ? JComponentRouterConfig maybe, or JComponentRouterContext ?

@johanjanssens

This comment has been minimized.

Show comment
Hide comment
@johanjanssens

johanjanssens Jan 13, 2015

@Hackwar Oki, fair comments. Additional feedback :

  • I still don't feel category handling should be in the this router. It would make more sense to me that this router is part of the categories component. It could be an abstract router one can extend from. By the chosen implementation you are tightly coupling categorie handling with routing.
  • I would consider renaming JComponentRouterAdvanced to JComponentRouterView. Advanced isn't a very concrete name. This router deals specifically with component routing based on registering views. Would make sense the name reflect this, this also matches with the JComponentRouterViewconfigurationObject.

johanjanssens commented on 56e680e Jan 13, 2015

@Hackwar Oki, fair comments. Additional feedback :

  • I still don't feel category handling should be in the this router. It would make more sense to me that this router is part of the categories component. It could be an abstract router one can extend from. By the chosen implementation you are tightly coupling categorie handling with routing.
  • I would consider renaming JComponentRouterAdvanced to JComponentRouterView. Advanced isn't a very concrete name. This router deals specifically with component routing based on registering views. Would make sense the name reflect this, this also matches with the JComponentRouterViewconfigurationObject.

Hackwar and others added some commits Jan 21, 2015

Implementing JComponentRouterAdvanced class. Fixes #5444
Call-time pass-by-reference has been removed

Removing JError, using Exception instead

protecting $name and renaming register() to registerView()

Adding removeRule, getRules and renamed $id to $key in register method

Making method names consistent

Implementing JComponentRouterViewconfiguration for configuration of views in JComponentRouterAdvanced

Codestyle, smaller improvements, unittests for all component router classes except for JComponentRouterAdvanced

Removing ability to have one view with different names and implementing unittests for JComponentRouterAdvanced

Adding get<View>Slug() and get<View>Key() methods to JComponentRouterAdvanced

Updating unittest

Small fixes

Adding back in platform check

Adding back in platform check

Adding back in platform check

Adding back in platform check

Implementing feedback so far

Adding "covers" notation for unittests
Merge branch '3.5-dev' of https://github.com/joomla/joomla-cms into c…
…omponentrulesitemid

Conflicts:
	tests/unit/suites/libraries/cms/component/router/JComponentRouterViewTest.php
Merge pull request #6990 from mbabker/35testFix
Fix unit test failure in JComponentRouterViewTest
@Hackwar

This comment has been minimized.

Show comment
Hide comment
@Hackwar

Hackwar May 22, 2015

Member

I've updated this PR to work with the changed JComponentRouterView class. Please test.

Member

Hackwar commented May 22, 2015

I've updated this PR to work with the changed JComponentRouterView class. Please test.

@dgrammatiko

This comment has been minimized.

Show comment
Hide comment
@dgrammatiko

dgrammatiko May 22, 2015

Contributor

@Hackwar what shall we look for the output urls? just B/C or something more/else?

Contributor

dgrammatiko commented May 22, 2015

@Hackwar what shall we look for the output urls? just B/C or something more/else?

@Hackwar

This comment has been minimized.

Show comment
Hide comment
@Hackwar

Hackwar May 22, 2015

Member

simply B/C. This should not change the URLs.

Member

Hackwar commented May 22, 2015

simply B/C. This should not change the URLs.

@Hackwar

This comment has been minimized.

Show comment
Hide comment
@Hackwar

Hackwar Aug 2, 2015

Member

I've combined the changes from this and all other routing related PRs into a new PR: #7615 Please review and comment in the new PR. I'm closing this one, so that we can focuse on the new PR.

Member

Hackwar commented Aug 2, 2015

I've combined the changes from this and all other routing related PRs into a new PR: #7615 Please review and comment in the new PR. I'm closing this one, so that we can focuse on the new PR.

@Hackwar Hackwar closed this Aug 2, 2015

@Hackwar Hackwar deleted the Hackwar:componentrulesitemid branch Jan 6, 2016

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