http://joomlacode.org/gf/project/joomla/tracker/?action=TrackerItemEdit&tracker_item_id=33428&start=0

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)

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.

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.

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.

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?

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

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.

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

Why not preprocess()?

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.

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

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

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

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

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

You can also restart the build on Travis directly.

Sent from my iPhone

On Mar 7, 2014, at 3:37 PM, Marko D notifications@github.com wrote:

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

—
Reply to this email directly or view it on GitHub.

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.

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

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

Done. So lets get some testers. :-)

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

fixed the fatal error

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

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&view=article&id=8:french&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

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.

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?

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

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.

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.

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.

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.

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

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

@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

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

Can this be closed in favour if #3383

#3383 merged, closing.