Success

User tests: Successful: Unsuccessful:

avatar wilsonge wilsonge - open - 1 Apr 2014
avatar Bakual
Bakual - comment - 1 Apr 2014

I can't reproduce an issue with my own extension using the "legacy" routing.
Can you explain a bit more what the issue is?

avatar wilsonge
wilsonge - comment - 1 Apr 2014

Not exactly :p so basically we had an issue where accidentally the core extensions were not not using the new routing classes and used legacy mode. In urls where there was :id:-:alias: the URL gained a second semicolon so for example:

using new interface - 9:kategorie-de-de
using legacy class - 9:kategorie:de-de

And of course then the urls failed. Not all actually went to a 404. Just to the wrong page

Does that help?

avatar Bakual
Bakual - comment - 1 Apr 2014

using new interface - 9:kategorie-de-de
using legacy class - 9:kategorie:de-de

Correct SEF URL would imho be 9-kategorie-de-de. The colon should be replaced with a hyphen when SEF is on. Because the colon is actually a reserved character in an URL and the alias doesn't allow a colon in it.

We do that a bit funny in Joomla for some reasons. In non-SEF URLs the "slug" (id-alias combination) is created using a colon (9:kategorie-de-de), but in SEF URLs it will be replaced with a hyphen (9-kategorie-de-de).
When parsing the URL, that doesn't matter much for many components as they just force that part to int to the get the id from the slug. They don't care about the alias at all. :smile:

It would still help to know how to reproduce that behavior.
I understand it correct that this behavior is related to the new routing class?
In the tracker you said it affects 3rd party extensions, but now you say core?

avatar mbabker
mbabker - comment - 1 Apr 2014

The only way I know to do it:

  • Revert this commit
  • Install fresh with no sample data and add a language with all multilingual features
  • Once installed, navigate to front-end
  • Switch to an alternate language, then click the category link for the article on the frontpage

For me with an en-GB/de-DE install, it 404'd on http://jcms/index.php/de/home-de-de/9-kategorie-de-de. The internal handling replaced the first two dashes with colons which caused the com_content router to route incorrectly (it was routing to the article view and checking for article ID 9 with catid 9, in reality it should be routing to category ID 9).

avatar wilsonge
wilsonge - comment - 1 Apr 2014

OK so when we had the PR that changed the routing interface name we forgot to change the name here to the interface at first. https://github.com/joomla/joomla-cms/blob/3.3-dev/libraries/cms/router/site.php#L720

This effectively lead to all core components for a short amount of time using the legacy URL's (what all 3PD's would be using from 3.3) rather than the new routing. So when we found the bug it was in core because of this bug (since fixed) - but as all 3PD's won't be using the new class it should still affect them.

avatar wilsonge
wilsonge - comment - 1 Apr 2014

lol posting at the same time as @mbabker - just do what he says xD

avatar wilsonge wilsonge - change - 1 Apr 2014
Title
Fix issue in legacy routing
[#33547] Fix issue in legacy routing
avatar Bakual
Bakual - comment - 1 Apr 2014

but as all 3PD's won't be using the new class it should still affect them.

I'm confused :confused: Is it already fixed or not and does it affect 3rd parties or not?

I thought the legacy routing works the same as it did before the introduction of this new router class.
The code you want to remove in this PR is also present in current 3.2.x. Thus I'd say removing that would break something for sure for extensions using legacy routing (which are currently all extensions).

avatar mbabker
mbabker - comment - 1 Apr 2014

It won't break 3PD. There's actually a risk of breaking in 3PD code right
now because aliases can be double encoded. This PR is needed to fix that
condition.

On Tuesday, April 1, 2014, Thomas Hunziker notifications@github.com wrote:

but as all 3PD's won't be using the new class it should still affect them.

I'm confused [image: :confused:] Is it already fixed or not and does it
affect 3rd parties or not?

I thought the legacy routing works the same as it did before the
introduction of this new router class.
The code you want to remove in this PR is also present in current 3.2.x.
Thus I'd say removing that would break something for sure for extensions
using legacy routing (which are currently all extensions).

Reply to this email directly or view it on GitHub#3399 (comment)
.

avatar Bakual
Bakual - comment - 1 Apr 2014

I can confirm the issue with my own extension when I dump $segments in my ParseRoute function.
1-my-own-alias becomes 1:my:own-alias instead of 1:my-own-alias.
It doesn't generate an error for me because I'm not doing anything with the alias, but it's clearly wrong.

PR fixes that and I couldn't see any sideeffect. So :+1: from me.

avatar mbabker mbabker - reference | 3e7f741 - 1 Apr 14
avatar mbabker mbabker - merge - 1 Apr 2014
avatar mbabker mbabker - close - 1 Apr 2014
avatar mbabker mbabker - change - 1 Apr 2014
Status New Closed
Closed_Date 0000-00-00 00:00:00 2014-04-01 23:40:39
avatar mbabker mbabker - close - 1 Apr 2014
avatar wilsonge wilsonge - head_ref_deleted - 2 Apr 2014
avatar Bakual Bakual - reference | 42a606f - 12 May 14

Add a Comment

Login with GitHub to post a comment