Routing: Legacy Router Rule for com_users

[Hackwar]

This implements the current routers behavior of com_users as a legacyrouter. This is meant for those that want to keep the old behavior at all costs. Later, parameters will be introduced to switch between the legacy behavior and new features.

This router is copied verbatim from the current router to make sure that the same behavior is kept. No changes should be done to it until Joomla 4.0, when this is supposed to be deleted. The component routers right now are so fragile, that a slow transition will be next to impossible. To prevent any breaks in backwards compatibility, this legacy router is introduced, so that people having problems with the new router can switch back.

How to test

1. Check your site before applying the patch and see the URLs of com_users.
2. Apply the patch.
3. See the URLs of com_users to stay the same.

Dependencies

This PR requires #5446 to be accepted by the project before it can be applied.

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]

@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 ?

[johanjanssens]

@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.

[johanjanssens]

@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().

[johanjanssens]

@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 ?

[johanjanssens]

@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)

[johanjanssens]

@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 ?

[johanjanssens]

@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 ?

[johanjanssens]

@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 ?

[johanjanssens]

@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.

[johanjanssens]

@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.

[johanjanssens]

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

[johanjanssens]

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

[johanjanssens]

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

[wilsonge]

Probably need to change the text there then :)

[johanjanssens]

@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().

[johanjanssens]

@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]

@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'.

[johanjanssens]

@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]

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.