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: New URL routing for Joomla #8256

Closed
wants to merge 113 commits into
base: staging
from

Conversation

Projects
None yet
@Hackwar
Member

Hackwar commented Nov 3, 2015

This is an updated PR from #7615

This PR implements a new routing system for Joomla, based on what has been proposed here: http://www.joomlager.de/crowdfunding

This is a combined PR of the former changes from #5446, #5501, #5502. #5503, #5509, #5599 and #5604.

Please test and comment. For further informations, read the other 7 PRs.

@johanjanssens

This comment has been minimized.

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

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.

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.

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.

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

Owner

Hackwar replied Dec 17, 2014

Will do.

This comment has been minimized.

johanjanssens replied Dec 25, 2014

Great!

@johanjanssens

This comment has been minimized.

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

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.

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.

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

Owner

Hackwar replied Dec 17, 2014

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

This comment has been minimized.

johanjanssens replied Dec 25, 2014

Thanks!

@johanjanssens

This comment has been minimized.

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

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.

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.

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.

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.

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

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.

johanjanssens replied Dec 25, 2014

Oki.

@johanjanssens

This comment has been minimized.

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

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.

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.

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.

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

Owner

Hackwar replied Dec 17, 2014

See my previous comment. 😄

@johanjanssens

This comment has been minimized.

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

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.

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.

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

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.

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.

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

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.

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.

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

Owner

Hackwar replied Dec 17, 2014

See my above note.

This comment has been minimized.

johanjanssens replied Dec 25, 2014

See my note. Please consider it.

@johanjanssens

This comment has been minimized.

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

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.

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.

@joomla-cms-bot

This comment has been minimized.

joomla-cms-bot commented Jan 7, 2016

This PR has received new commits.

CC: @anibalsanchez, @Gitjk


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

@joomla-cms-bot

This comment has been minimized.

joomla-cms-bot commented Jan 7, 2016

This PR has received new commits.

CC: @anibalsanchez, @Gitjk


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

@joomla-cms-bot

This comment has been minimized.

joomla-cms-bot commented Jan 7, 2016

This PR has received new commits.

CC: @anibalsanchez, @Gitjk


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

@joomla-cms-bot

This comment has been minimized.

joomla-cms-bot commented Jan 7, 2016

This PR has received new commits.

CC: @anibalsanchez, @Gitjk


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

@joomla-cms-bot

This comment has been minimized.

joomla-cms-bot commented Jan 7, 2016

This PR has received new commits.

CC: @anibalsanchez, @Gitjk


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

@Hackwar

This comment has been minimized.

Member

Hackwar commented Jan 8, 2016

Hi folks, I cleaned up the code further, fixed the unittests and fixed the codestyle. Please give this another round of tests.

To test, please apply these changes and enable the new routing in the global configuration. Then check the URLs of the site. The best would be, if you could test this on a copy of a real site. The sampledata of Joomla has already been tested and works. When you test these changes, the URLs should stay the same in most cases. The only time when the URL will be changed by this is when the menu item links to a category and the content item is in a child-child-category of the linked category.

@Gitjk

This comment has been minimized.

Gitjk commented Jan 8, 2016

Hi again,
Just had a short (10 Minute) look. Used latest staging Joomla and VirtueMart 3.0.12, both bilingual. After adding #8256 with the patch-tester, I didn't see any difference in urls with 'Use new URL routing' enabled/disabled. Seems to work fine at the first glance. I might have another look on Sunday.

@infograf768

This comment has been minimized.

Member

infograf768 commented Jan 9, 2016

@test
Looks like working fine here on my multilingual test site, sef on.
Did not test the child-child situation yet.

@infograf768

This comment has been minimized.

Member

infograf768 commented Jan 9, 2016

Please alpha order new lang strings.

@@ -168,6 +168,8 @@ COM_CONFIG_FIELD_PROXY_USERNAME_DESC="The username used to access the Proxy serv
COM_CONFIG_FIELD_PROXY_USERNAME_LABEL="Proxy Username"
COM_CONFIG_FIELD_SECRET_DESC="This is an auto-generated, unique alphanumeric code for every Joomla installation. It is used for security functions."
COM_CONFIG_FIELD_SECRET_LABEL="Secret"
COM_CONFIG_FIELD_SEF_ADVANCED_LABEL="Use new URL routing"
COM_CONFIG_FIELD_SEF_ADVANCED_DESC="This uses the new URL routing. This will change your URLs!"

This comment has been minimized.

@brianteeman

brianteeman Jan 9, 2016

Contributor

will or may?

This comment has been minimized.

@Hackwar

Hackwar Jan 9, 2016

Member

You are right, it "may" change your URLs.

@anibalsanchez

This comment has been minimized.

Contributor

anibalsanchez commented Jan 9, 2016

Tested on a news site, upgraded to Joomla 3.5, installed the new router, crawled Urls with Screaming Frog and compared results. No differences found in resulting Urls.


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

@wilsonge

This comment has been minimized.

Contributor

wilsonge commented Jan 28, 2016

@Hackwar can you please update this PR to staging (conflicts atm). Got some more people to test on this

@joomla-cms-bot

This comment has been minimized.

joomla-cms-bot commented Jan 29, 2016

This PR has received new commits.

CC: @anibalsanchez, @Gitjk


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

1 similar comment
@joomla-cms-bot

This comment has been minimized.

joomla-cms-bot commented Jan 29, 2016

This PR has received new commits.

CC: @anibalsanchez, @Gitjk


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

@Hackwar

This comment has been minimized.

Member

Hackwar commented Jan 29, 2016

I've updated the branch. Please notice that I've moved the setting for advanced routing from the global configuration to the component configuration for each specific component.

wilsonge added a commit that referenced this pull request Feb 20, 2016

@wilsonge wilsonge closed this Feb 20, 2016

@wilsonge

This comment has been minimized.

Contributor

wilsonge commented Feb 20, 2016

Merged into 3.6.x branch :)

@wilsonge wilsonge modified the milestones: Joomla 3.7.0, Joomla 3.6.0 May 11, 2016

@810

This comment has been minimized.

Contributor

810 commented Oct 5, 2016

UsersHelperRoute::getLoginRoute(); is set to deprecated, what's the new call, or are the deprecated tags wrong on the j3.6/7 branch, because on the 4.0 branch its still there

@wilsonge

This comment has been minimized.

Contributor

wilsonge commented Oct 5, 2016

4.0 branch is currently based on staging not the 3.7 branch :)

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