Skip to content

make() method for component router interface #3244

Closed
wants to merge 12 commits into from

8 participants

@Hackwar
Joomla! member
Hackwar commented Mar 6, 2014

This PR adds a make() method to the component router interface. This method is meant to clean up URLs and do a kind of pre-processing on them and is thus executed on each URL every time, regardless of SEF being turned on or off.

The specific task for this method is to provide a means to add the Itemid to a URL in the router. Since the component router is normally only loaded when SEF is switched on and then it is only meant to actually SEF the URL, this process needs to go into a second method, which is only aimed at cleaning up the URL and provide a way to add this Itemid.

Of course, we don't have to only add the Itemid here. We might add or remove other parts of the query, which ultimately makes processing the query in the actual build() method easier. Think of missing slug or a timestamp or renaming limitstart to start, etc.

@dongilbert

Personally, I would make JComponentRouter into an abstract base class that implements the JComponentRouterInterface, so we can keep the code duplication to a minimum. It would also make it easier to abstract common logic up the inheritance tree later on, if we so choose.

Additionally, I know this guideline isn't in the CMS coding style yet, but I'm working towards adding the "PHP interfaces must be suffixed with Interface, which is inline with other interfaces within the CMS. (JTableInterface, JObservableInterface, etc)

@dongilbert

Also, I'm not sure on the wording of the method. make($query) doesn't convey, at least to me, what it's supposed to be doing.

@Hackwar
Joomla! member
Hackwar commented Mar 6, 2014

when I implemented the router interface, it was exactly the other way around. We currently have JModel and JController as interfaces, which is why I used JComponentRouter as name for the interface.
I also plan on an abstract class to do some preliminary work in the area of abstracting the code. That class can have the find-the-right-itemid code in it from all extensions and replace the code in all the *HelperRoute classes. As I showed several times already, there are also ways how we could make the whole router thing drop dead easy for the developers and awesomely flexible for the users, but considering that 3.3 is right around the corner, that is something that I can't finish in time.
My proposal would be a class JComponentRouterExample, which contains that code and is deprecated directly, with the clear comment that this class is not meant to be used anywhere else, since it will be superseeded in 3.4, for example. Not a nice solution, but since no one looked at this code for the last 3 years but me, I can't help it when a perfect implementation is not possible right away.

Regarding the wording of the method: I talked with @wilsonge about this and we played around with a few names, like check(), preprocess() and validate() and make() seemed to be the best proposal so far. If you have a better one, feel free to provide it. I'll change my code accordingly.

@dongilbert

The current direction is for newly added interfaces to be suffixed with Interface. Those who opposed that suffix previously (hence the reason for JController and JModel) have either left the project, or came around to the realization that it's the right thing to do (as evidenced in the Framework).

Please change this to JComponentRouterInterface and move the duplicated code into an abstract parent class that implements make($query) instead of duplicating code across all implementations.

@dongilbert

As for another method name, I'd really need to understand the exact purpose to help come up with one. It seems like it's meant to fill in missing bits in the $query array. Is that the case?

In terms of a router, make($query) implies that it's going to make an SEF url out of the given parameters, which this is obviously not doing.

If it is the case that it's meant to fill in the blanks, what if you called it fill($query) instead?

@Bakual
Bakual commented Mar 6, 2014

Maybe complete($query) or amend($query)? Just using Google to find a word for the one I would use in german 😄

@Hackwar
Joomla! member
Hackwar commented Mar 6, 2014

Ok, I'll add the suffix, no problem.

The make() method is supposed to preprocess the query. That could mean that it adds the Itemid, but it could also change a query parameter, like the start <=> limitstart thing that we have. Simply said, it should do all the things that have to be done to a URL to be complete when SEF is turned off.
The problem is, that stuff like adding the Itemid is currently done in the *HelperRoute classes, where it does not belong. It belongs inside the router, since the router needs to find out the right Itemid by himself. Otherwise you get wrong URLs like right now, where we can add a link to an article1 in an article2, save that, then move article1 under a different Itemid and have wrong URLs in the article2. If we did not pre-compute the Itemid and instead create it when the URL in the content is parsed, the router needs to have a way to do that. The component router could do that, but the component router only provides build and parse, which either expect a SEFed URL or create one. Neither is usefull when SEF URLs are switched off, especially since the component routers aren't even loaded when SEF URLs are off. make() is supposed to fill that void. And it is supposed to check the existing query parameters and make sure that they conform to the format that the SEF methods expect.

@dongilbert

That's quite a bit of functionality it can do there. :) What about just calling it process($query)?

@Bakual
Bakual commented Mar 6, 2014

Why not preprocess()?

@Hackwar
Joomla! member
Hackwar commented Mar 6, 2014

the thinking was, that process() or preprocess() is equally meaningless... But I guess preprocess would be the right way to go. Will rename it to that.

As much as I'd like to directly propose a class that implements the method and removes all the duplicate code in the routers, I will need a bit more time and I can't guarantee that I will make it before 3.3 is released. So before we are stuck in b/c hell here, I'd rather have this duplicate code in than to have no chance of vital and easy improvements in the whole 3.x series at all.

@Hackwar Hackwar Changing make() to preprocess()
Renaming JComponentRouter to JComponentRouterInterface
15efb94
@Hackwar
Joomla! member
Hackwar commented Mar 6, 2014

Changed make() to preprocess() and JComponentRouter to JComponentRouterInterface

@wilsonge
Joomla! member
wilsonge commented Mar 6, 2014

I did say preprocess() instead of make() :P just saying :p :p :p

In all seriousness though suffixing Interface kinda makes sense. Agreed we don't wanna be stuck with B/C issues so this should go in now. Although if you can get the abstract class up I guess it would be pretty awesome :)

@Hackwar
Joomla! member
Hackwar commented Mar 7, 2014

The failing Travis is not because of this change. Something else is wrong there.

@wilsonge
Joomla! member
wilsonge commented Mar 7, 2014

Yeah it was one of the security fixes in 3.2.3 that broke it. If you merge in master it should pass. It got fixed last night

@chivitli
chivitli commented Mar 7, 2014

Or just make a "dummy" commit like I did with my PR to pass the Travis (I added a newline and then removed it)

@dongilbert
@Hackwar
Joomla! member
Hackwar commented Mar 9, 2014

On the chance that I broke the PR, I implemented a class JComponentRouterBasic, which does the Itemid stuff that I talked about earlier. This deprecates a bit of stuff, it also does a lot of stuff faster than our current implementation, but it is still far from perfect. It's basically a quick shot from the holster.

@Hackwar
Joomla! member
Hackwar commented Mar 9, 2014

I've also added the call to preprocess() to all legacy functions.

@mbabker
Joomla! member
mbabker commented Mar 9, 2014

Aside from syntax errors (I sent a PR to you for those), everything seems fine.

@Hackwar
Joomla! member
Hackwar commented Mar 9, 2014

Done. So lets get some testers. :-)

@chivitli

So far so good, but I do have one issue to report. I have a homepage set in a blog view. In one of the articles displayed on the homepage, I added a link to a category, which resulted in:
Fatal error: Access to undeclared static property: JComponentRouterBasic::$lang_lookup in C:\wamp\www\joomla3.3\libraries\cms\component\router\basic.php on line 52

Generated link looks like "index.php?option=com_content&view=category&id=2&language=9"

Removing &language=9 fixes the issue.

I couldn't test fully multilingual sites due to another bug (http://joomlacode.org/gf/project/joomla/tracker/?action=TrackerItemEdit&tracker_item_id=33443&start=0)

In any case, this is the most welcome addition, I am looking forward to have item ids removed from the link 👍

@Hackwar
Joomla! member
Hackwar commented Mar 10, 2014

fixed the fatal error

@chivitli

Great, that one is fixed. I have another one

Notice: Undefined variable: language in C:\wamp\www\joomla3.3\libraries\cms\component\router\basic.php on line 55

It happens after creating an article to which I assign a language

@chivitli

I added manually menus for multilingual test directly in the DB, so here are few more issues.

I created one article with english language, and one with french. I directly linked from english one to french one in two ways. One link using JCE, and one by clicking on the "article" button.

The one created wth JCe leads to a 404 page beccause it goes to the english structure. Internally it looks like:

index.php?option=com_content&amp;view=article&amp;id=8:french&amp;catid=9:blog.

The one created with Article buttons generates the same notice like above (cause of unset at a wrong place). It also leads incorrectly to /en/ structure, but it doesn't get 404 because to the link is appended query ?lang=french, which shou;dn't be visible.

Similar for language switcher module, it has query appended, but it leads correctly to /fr/ structure

@Hackwar
Joomla! member
Hackwar commented Mar 10, 2014

I should state that this does NOT remove the Itemids from the URLs or actually changes the URLs in any way. It is simply an attempt to simplify the code and reduce duplicate code.

@chivitli

Notice is gone, the other issues (404 and lang in query) stayed.

I think you misunderstood me, I didn't mean it removes item id's from links that you see like site/1-article, but removing &Itemid=xxx from the generated link so that you can move articles around safely.

But now that you mention it, with new router, how much more work would we need to offer advanced sef option which removes them id's from the visible link as well?

@Hackwar
Joomla! member
Hackwar commented Mar 10, 2014

There is lots of stuff that we can do. The question is: How much time do we have and will the PLT accept the changes? My last info was, that 3.3 is right around the corner and in that case we wont be able to get this stable enough for release. That is why I tried to do this incrementally. Now Don asked for some further improvements, but the JComponentRouterBasic addition already introduces lots of issues...

@chivitli

Considering the level of improvement and how long the 3 is gonna stay around, I think extra few days to stabilise it won't hurt anyone :) I am willing to test as much as needed.

@chivitli

One more thing, the 404 when I add a link to french article with JCE is not because of this PR. The same happens even when I revert it and repeat the steps, so that bug exists nevertheless.

@chivitli chivitli commented on an outdated diff Mar 14, 2014
libraries/cms/component/router/basic.php
+ $query = $db->getQuery(true)
+ ->select('a.sef AS sef')
+ ->select('a.lang_code AS lang_code')
+ ->from('#__languages AS a');
+ $db->setQuery($query);
+
+ $langs = $db->loadObjectList();
+ foreach ($langs as $lang)
+ {
+ $this->lang_lookup[$lang->lang_code] = $lang->sef;
+ }
+
+ $component = JComponentHelper::getComponent('com_' . strtolower($this->getName()));
+
+ $attributes = array('component_id', 'language');
+ $values = array($component->id, '*');
@chivitli
chivitli added a note Mar 14, 2014

This does not seem to be correct, as regardless of the language variable passed, only menu items set to 'all languages' will be returned. I think that all items need to be fetched, otherwise correct Itemid won't be found (which was what happened in my test).

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

I was debuging language issue, and some sweat later I can at least give some info what's happening and why my fr articles end up with english path.

The problem is that before the component router is called uri is already processed. By the time you pass $uri to JRouterSite::buildSefRoute, it will already have the path variable set, so the part of the component router which adds $query['lang'] is completely ignored. Any time raw url does not have '&lang=xx' set in it, and site is multilanguage, url with the default language path will be generated regardless of what the component router does, because PlgSystemLanguageFilter::buildRule() will be called before. This is the current behaviour without this patch as well, so for example at the moment JCE produces incorrect links as it doesn't append 'lang' properly to raw urls.

The solution is either to change the logic of how system works, or to instead of 'language=xx-XX' append 'lang=xx' (note, the short code, not the full code) to raw url's when you are adding a link. JCE will stay buggy in any case and they need to fix the issue on their side, as the core Article button works as it should and appends language strings.

There is one more issue, I added inline comment about it.

@Hackwar
Joomla! member
Hackwar commented Mar 27, 2014

I reverted the last commit partially in order to get this into 3.3, since I don't have the time to do this properly now. 😦 Please test and merge. 😉

@chivitli

Ah, a pitty, I think we were close :)

@Hackwar
Joomla! member
Hackwar commented Mar 27, 2014

Unfortunately no. There is lots of stuff to fix before this correctly works.

@infograf768
Joomla! member

@test
Unhappily, many urls are changed in multilingual when they are created by Using the Article button or when displayed for example through the Most Read Content module (or any Tag module):
Before:
http://localhost:8888/testwindows/trunkgitnew/fr/instructions-multilangue/160-activer-le-plugin-syst%C3%A8me-filtre-de-langue.html
After:
http://localhost:8888/testwindows/33dev/fr/home/11-cat%C3%A9gories-en-fran%C3%A7ais/joomla-multilangue/160-activer-le-plugin-syst%C3%A8me-filtre-de-langue.html

@mbabker
Joomla! member
mbabker commented Mar 28, 2014

Please see #3383 which takes care of just adding the method to the interface.

@brianteeman
Joomla! member

Can this be closed in favour if #3383

@mbabker
Joomla! member
mbabker commented Mar 31, 2014

#3383 merged, closing.

@mbabker mbabker closed this Mar 31, 2014
@Hackwar Hackwar deleted the Hackwar:router_interface branch Jan 6, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.