Documentation Required PR-staging ?

Pending

User tests: Successful: Unsuccessful:

avatar csthomas
csthomas
3 Jan 2018

Summary of Changes

When there is no menu item for category/categories/article view in com_content and option Remove IDs is enabled then a link like:

  • /index.php/en/component/content/article/article-en-gb?catid=8 returns error 404

In this PR I rewrote NomenuRules class to build and parse links:

  • index.php/en/component/content/article/category-en-gb/article-en-gb (option Remove IDs is enabled)
  • index.php/en/component/content/article/3-category-en-gb/2-article-en-gb

Testing Instructions

Disable all menu items for com_content categories/category/article views.
Go to frontend index.php/component/content/categories and check links to category and article items.

Expected result

Links work, no 404 when option Remove Ids is enabled/disabled.

Actual result

When option Remove Ids is enabled then links to articles return error 404.

Votes

# of Users Experiencing Issue
1/1
Average Importance Score
5.00

avatar csthomas csthomas - open - 3 Jan 2018
avatar csthomas csthomas - change - 3 Jan 2018
Status New Pending
avatar joomla-cms-bot joomla-cms-bot - change - 3 Jan 2018
Category Libraries Unit Tests
avatar jsubri
jsubri - comment - 3 Jan 2018

Quite large PR, congrats. I've tested with a copy of a customer site on my PC. At a fist glance the links do not have the IDs anymore which is good, and 404 are gone.

I've a draw back for Links displayed via Modules . In my case the links are displayed via the Joomla Custom Modules in position Right using the popular 3rd party extension https://www.regularlabs.com/extensions/articlesanywhere . I can reproduce without the 3rd party plug-in using the "Modules: Articles - Newsflash" once le links are clicked they are loaded in the "main" position. At this point this is B/C the Modules in Right position do not load anymore using Menu Assignment "Only on the pages selected" as we lost the menu "context".

avatar csthomas
csthomas - comment - 4 Jan 2018

Quite large PR, congrats. I've tested with a copy of a customer site on my PC. At a fist glance the links do not have the IDs anymore which is good, and 404 are gone.

So it is success test.

I've a draw back for Links displayed via Modules . In my case the links are displayed via the Joomla Custom Modules in position Right using the popular 3rd party extension https://www.regularlabs.com/extensions/articlesanywhere . I can reproduce without the 3rd party plug-in using the "Modules: Articles - Newsflash" once le links are clicked they are loaded in the "main" position. At this point this is B/C the Modules in Right position do not load anymore using Menu Assignment "Only on the pages selected" as we lost the menu "context".

In general, this is unrelated to this PR.
If you have links start with index.php/component/ or /component/ then it means that there is no menu item for such pages. If there is no menu item then modules won't work as you expected.

I understand that you complain, that before you have (invalid - from my perspective but) working link like '/menu-category-a/category-alias-b/articlealias' but now you have /component/content/article/category-alias-b/articlealias.

If there is no menu item then only modules with setting "On all pages" (and IIRC On all except selected) will be displayed.
To fix this issue you should create menu item for missing category (category-alias-b).

Ex, if you see link to article like /component/content/article/alias-category/alias-article this means that you should create a menu item for category "Alias Category".
If you do not want to create a massive items per each top category you can create menu item for categories view only.

avatar jsubri
jsubri - comment - 4 Jan 2018

I understand that you complain, that before you have (invalid - from my perspective but) working link

Correct that is the reason I mentioned B/C

To fix this issue you should create menu item for missing category (category-alias-b).

You are correct again and worth mentioning the menu item need to be Published otherwise it won't work. In my specific case if this PR is accepted, I'll end-up creating 3-5 extra menu and editing 20+ modules to have the same user experience. On the positive note, SEO and sitemap will be enhanced.

It breaks my site and I can deal with that, so can break others.

I'll vote for this PR subject to super-duper assessment.

Updated formatting.

avatar csthomas
csthomas - comment - 4 Jan 2018

To be clear, mentioned issue about B/C is related to already merged PR #19099, not to this one.

I'll end-up creating 3-5 extra menu

Yes, this should be done earlier in you website but earlier joomla allows to missing menu items and put there (in link) menu items from other contents. The problem was hidden.

Ex. if you had 2 different categories (dogs and cats) on the same top level and you missed to add menu item for dogs then when you are on article about cats and you have a module with links to dogs, then links to dogs (in an old way) looks like /menu-for-cats/2-category-dogs/21-article-about-dogs
in a new way it will be /component/content/article/2-category-dogs/21-article-about-dogs.

The new way does not borrow the menu item from previous page, which created a lot of duplicate contents in Google.

and editing 20+ modules to have the same user experience.

After you add a few menu items then you can assign the old modules to them. That all.

avatar jsubri
jsubri - comment - 4 Jan 2018

I'll update the site accordingly.
Thank you

avatar Quy
Quy - comment - 4 Jan 2018

@jsubri Please mark your test result here: https://issues.joomla.org/tracker/joomla-cms/19280

avatar jsubri jsubri - test_item - 4 Jan 2018 - Tested successfully
avatar jsubri
jsubri - comment - 4 Jan 2018

I have tested this item successfully on 0fb789d

Modern router with SEF is far better with this PR, issue with languagefilter is a separated PR
As stated above we do not have the IDs anymore and 404 are gone.
Up to other testers to provide feedback and for the production team to decide if the side effects I mentioned are bug fixes.


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

avatar csthomas csthomas - change - 9 Jan 2018
The description was changed
avatar csthomas csthomas - edited - 9 Jan 2018
avatar Hackwar
Hackwar - comment - 9 Jan 2018

Hmm, I don't have a good feeling with this. From my perspective, this is not backwards compatible. it changes existing URLs from released versions of Joomla, so if we honour our dev guidelines and our commitment to backwards compatibility, we can not change this in this way.

A way to make this backwards compatible would be to add a nomenu2 rule and a switch in the components where you can switch between the different behaviors, like we have with modern and legacy routing.

Besides that, I would also ask you to change your unittests. The fact that you had to change the unittests in this way, shows that this is a break in B/C. The tests in this PR do not test the same as they did before and prior tests would fail. You are also using sample data from the database, which I would consider to be bad practice, and instead have the test-data be part of the test-suite.

avatar csthomas
csthomas - comment - 10 Jan 2018

We have at least six routing rules for SEF:

  1. Stable - with menu item
  2. Stable - without menu item
  3. Modern - with menu item (Remove ids NO)
  4. Modern - with menu item (Remove ids YES)
  5. Modern - without menu item (Remove ids NO)
  6. Modern - without menu item (Remove ids YES)

This PR breaks B/C (error 404) for rule 5 (only one example below), which has been using rarely.
The released versions of Joomla 3.8 has a habit to use the other menu item to generate a link to the page without own menu item.

I wrote this PR to fix rule 6, which is completely broken.

Examples of 5 and 6 rule before and after PR:

We can start at index.php/en/component/content/categories.
Remeber to disable all menu items for categories/category/article views.

Before PR (Remove IDs is OFF - rule 5)

Build and parse:

  • OK - index.php/en/component/content/category/8-category-en-gb
  • OK - index.php/en/component/content/article/1-article-en-gb?catid=8
  • OK - index.php/en/component/content/category/1190-subcategory-en-gb
  • OK - index.php/en/component/content/article/4006-article-in-subcategory-en?catid=1190

Before PR (Remove IDs is ON - rule 6)

Build ok and the parser results:

  • Error 404 - index.php/en/component/content/category/category-en-gb
  • Error 404 - index.php/en/component/content/article/article-en-gb?catid=8
  • Error 404 - index.php/en/component/content/category/subcategory-en-gb
  • Error 404 - index.php/en/component/content/article/article-in-subcategory-en?catid=1190

After PR (Remove IDs is OFF - rule 5)

Parse old links:

  • Works - as - before - - index.php/en/component/content/category/8-category-en-gb - (the same link)
  • Works - as - before - - index.php/en/component/content/article/1-article-en-gb?catid=8
  • Break B/C - Error 404 - index.php/en/component/content/category/1190-subcategory-en-gb
  • Works - as - before - - index.php/en/component/content/article/4006-article-in-subcategory-en?catid=1190

Build and parse new links:

  • The same - index.php/en/component/content/category/8-category-en-gb
  • New Link - index.php/en/component/content/article/8-category-en-gb/1-article-en-gb
  • New link - index.php/en/component/content/category/8-category-en-gb/1190-subcategory-en-gb
  • New link - index.php/en/component/content/article/8-category-en-gb/1190-subcategory-en-gb/4006-article-in-subcategory-en

After PR (Remove IDs is ON - rule 6)

Parse old links, which does not work before:

  • OK +fixed - index.php/en/component/content/category/category-en-gb
  • Error 404 - index.php/en/component/content/article/article-en-gb?catid=8
  • Error 404 - index.php/en/component/content/category/subcategory-en-gb
  • Error 404 - index.php/en/component/content/article/article-in-subcategory-en?catid=1190

Build and parse new links:

  • The same - index.php/en/component/content/category/category-en-gb
  • New link - index.php/en/component/content/article/category-en-gb/article-en-gb
  • New link - `index.php/en/component/content/category/category-en-gb/subcategory-en-gb
  • New link - index.php/en/component/content/article/category-en-gb/subcategory-en-gb/article-in-subcategory-en
avatar Hackwar
Hackwar - comment - 10 Jan 2018

I understand that. However, a break in B/C is a break in B/C. The broken behavior in rule 6 is indeed something we need to fix and I'm currently considering if we can switch off removing the ID for that router rule. That would be extremely dirty in my book... I don't know the right solution for that right now.

The other change in behavior however is not really acceptable. Either we stick to our B/C claim or we don't and it would be extremely hypocritical if a change like this now would be handled differently than the router modernization efforts before this.

avatar csthomas
csthomas - comment - 10 Jan 2018

For modern routing I'm not convinced to be B/C at this issue.
There is a real problem for the rule 6.

IMHO we can use an exception to fix the real problem,
but I am only a contributor, I will not decide.

I only put my solution there. I'm waiting for better solutions.
The unit tests can be changed later as you wish.

If we do not fix it at the beginning, the problem of rule 6 will grow.

Please do not leave people with choice:

  • use old stable routing and be "safe"
  • or use modern routing (with removed Ids) and live with error 404 as expected behaviour till J4.
avatar csthomas csthomas - change - 14 Jan 2018
Labels Added: PR-staging ?
avatar csthomas
csthomas - comment - 15 Jan 2018

A way to make this backwards compatible would be to add a nomenu2 rule

OK

and a switch in the components where you can switch between the different behaviors, like we have with modern and legacy routing.

IMO there will be too many switches but If others agree I can do it.
I prefer to create nomenu2 and force it to joomla core components.

avatar joomdonation
joomdonation - comment - 22 Jan 2018

I have tested this item successfully on 614dda6

This PR solves 404 issue as described. It solves the issue #19430, too, so I think it needs to be merged for 3.8.4


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

avatar joomdonation joomdonation - test_item - 22 Jan 2018 - Tested successfully
avatar franz-wohlkoenig
franz-wohlkoenig - comment - 22 Jan 2018

@jsubri can you please retest?

avatar joomdonation
joomdonation - comment - 22 Jan 2018

My test is for single language website only.

avatar jsubri
jsubri - comment - 22 Jan 2018

I have tested this item successfully on 614dda6

This PR solves #19430

IMHO, due to #19099 and #19280, 3.8.4 shall probably advertised as a bit more than a simple patch for the ones using the Modern Router.


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/19280.
avatar jsubri jsubri - test_item - 22 Jan 2018 - Tested successfully
avatar franz-wohlkoenig franz-wohlkoenig - change - 22 Jan 2018
Status Pending Ready to Commit
avatar franz-wohlkoenig
franz-wohlkoenig - comment - 22 Jan 2018

Ready to Commit after two successful tests.

avatar infograf768
infograf768 - comment - 22 Jan 2018

Has someone tested this with multilingual?

avatar jsubri
jsubri - comment - 22 Jan 2018

My initial test was on multilingual, today was English only (Sample site). Given the potential impact with such PR, addl testers can’ hurt as we had recent changes in the languagefilter.

avatar csthomas
csthomas - comment - 22 Jan 2018

This PR works in the same way in a single or multilingual environment. This is independent.

avatar infograf768
infograf768 - comment - 25 Jan 2018

@mbabker
As this PR is an important change and has some B/C issue, I suggest you look at it carefully.
Thanks.

avatar csthomas
csthomas - comment - 5 Feb 2018

At now IMO this PR is not needed. I have plan to open a new PR on J4.

avatar csthomas csthomas - change - 5 Feb 2018
Status Ready to Commit Closed
Closed_Date 0000-00-00 00:00:00 2018-02-05 22:34:35
Closed_By csthomas
Labels Added: ?
avatar csthomas csthomas - close - 5 Feb 2018
avatar frandevelop
frandevelop - comment - 3 Apr 2018

The problem seems to also affect the Virtuemart categories, in this case in version 3.8.6 it fails with Remove Ids true or false, can it be fixed?

avatar csthomas
csthomas - comment - 3 Apr 2018

This PR is not B/C. It breaks rule 5 at "After PR (Remove IDs is OFF - rule 5)" from #19280 (comment)

@Hackwar suggested creating a new NomenuRules2 class and leaving the current intact, and then creating a switch in the core components. Then administrators can switch between the old (current) or the new (this PR).
I am not delighted with this solution.

NomenuRules does not work for all rules, but probably B/C is more important for people.

Maybe should I add an exception (until J4.0) to parse every invalid URL like index.php/en/component/content/category/1190-subcategory-en-gb? What about the duplicate URLs?

For example the correct URL should be like index.php/en/component/content/category/8-category-en-gb/1190-subcategory-en-gb

avatar frandevelop
frandevelop - comment - 4 Apr 2018

Sorry I'm not a contributor or expert, but if this problem linking a Joomla 3.8.6 menu with a virtuemart category, and this forum is the only one that has seen the problem, I would like to get the details of how to configure it? otherwise I will go back to the previous version 3.8.3 where the problem did not happen. Reviewing the comment that you indicate, I do not understand what I have to do.

Thank you

avatar csthomas
csthomas - comment - 4 Apr 2018

OK, the problem is complicated and difficult to repair from the technical side.

For quick fix, you should create a menu item for each (parent/category/or directly) for your link.
I'm not familiar with the virtuemart.

If your links contain a menu item, the problem will not affect you.

avatar frandevelop
frandevelop - comment - 4 Apr 2018

Hi,

I can not get it to work, I explain a little more detail

menu

this menu link to http://site.com/index.php?option=com_virtuemart&view=category&virtuemart_category_id=2&virtuemart_manufacturer_id=0

menu2

I have tried to put the direct url as you indicate instead of using the component, the problem persists,
when I open the URL directly, it does not work either, the error is the same, it does not find the category. In version 3.8.6 it works correctly.

integration

Maybe I do not understand something well, excuse my English

avatar csthomas
csthomas - comment - 4 Apr 2018

It seems that you issue is not related to this PR if it works in 3.8.3.

Please ask for help on joomla forum or directly ask a question on virtuemart forum.

avatar frandevelop
frandevelop - comment - 4 Apr 2018

The version that works is 3.8.3, and the 3.8.6 produces the error.

It produces a 404 error indicating that it does not found category, and since it is the same version they comment, I thought the problem would be the same.

Regards

avatar alex7r
alex7r - comment - 7 Apr 2018

I did not dig deep into testing, but I had an issue described in GitHub issue that was a root of this PR with J version 3.8.2, updated to 3.8.6 - still the same issue.
image
with the modern router.

User case to think about:
I want my homepage to contain featured articles.
And I want my articles to have urls like: /first-article and /second-article.

p.s.
I understand each statement that was provided in the issue and this PR. However, if those who claim that this is "new direction" and "this is how it should be" are cooking the future of Joomla - seems it's the time to think about going out, as a ship is going down.
Seriously, use "cool" stuff for Major version update, and leave minor with 1 line in content.xml file which will handle the task. It was there and was successfully running in production in 3.5 and I guess even earlier and was mentioned previously in GitHub issues.

p.p.s.
IMHO, if you (developers who in charge of this new feature) do want to introduce the new feature and want it to be welcomed - at least reconfigure default installation to work like a charm with your new feature and make sure it (demo) maintain the old behavior.

avatar csthomas
csthomas - comment - 7 Apr 2018
  1. This patch (this PR) is not included in Joomla3, nor J4.
  2. This PR breaks B/C and I closed it because I suspect that maintainers won't merge it.
  3. If something works in 3.8.2 and stops in 3.8.6 then please check merged commits (PRs) between 3.8.2 and 3.8.6
  4. This PR is closed, lots of people (except 10 participants) have not seen your new comments, I suggest to open a new issue and describe a problem there.
avatar laoneo
laoneo - comment - 9 Apr 2018

Why did you close this one? As I was integrating the new router in DPCalendar I was confronting with the same problem when there is no menu item available.

I had to do some dirty hacks to get rid of the 404 error in DPCalendar. There is an alternative approach which contains the id in #20117. But from a quick glance this here is the logical way. I mean how can a fix for an invalid url which leads to an invalid 404 be a BC break? I'm reopening it, as this bug needs to be fixed in J3.

avatar laoneo laoneo - change - 9 Apr 2018
Status Closed New
Closed_Date 2018-02-05 22:34:35
Closed_By csthomas
Labels Removed: ?
avatar laoneo laoneo - change - 9 Apr 2018
Status New Pending
avatar laoneo laoneo - reopen - 9 Apr 2018
avatar laoneo
laoneo - comment - 9 Apr 2018

I have tested this item successfully on 614dda6


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

avatar laoneo laoneo - test_item - 9 Apr 2018 - Tested successfully
avatar franz-wohlkoenig franz-wohlkoenig - change - 9 Apr 2018
Status Pending Ready to Commit
avatar franz-wohlkoenig
franz-wohlkoenig - comment - 9 Apr 2018

Ready to Commit after 3 successful tests.

avatar csthomas
csthomas - comment - 9 Apr 2018

:)

This PR has a one weakness, it breaks B/C in only one case, but we can add a workaround for J3.

This comment is about parsing an an old links if Remove IDs is DISABLED. for modern routing.

Subject:
Break B/C - Error 404 - index.php/en/component/content/category/1190-subcategory-en-gb

To test issue please create a tree of categories:

Animal -> Dog -> Dachshund

Before PR go to:
index.php/en/component/content/category/10-dachshund

After PR the above link won't work, Joomla will build a new one, the correct will be:
index.php/en/component/content/category/3-animal/4-dog/10-dachshund

On your test, you can ignore language prefix.

To support B/C, we can scan all categories and find the correct ID, (ex: for 10-dachshund).
IMO it should be done only for J3 and in J4 the hack should be removed.

avatar laoneo
laoneo - comment - 9 Apr 2018

For me it is not a BC break when such an obvious bug is fixed. In the release announcement we need to add then a special note.

avatar csthomas
csthomas - comment - 16 May 2018

Can we push it forward until not many people use it?

avatar laoneo
laoneo - comment - 17 May 2018

Maintainers feel a bit hesitant to ship it with a patch release. So I changed the base branch to 3.9.

@mbabker your turn here :)

avatar laoneo laoneo - change - 17 May 2018
Labels Added: Documentation Required ?
avatar csthomas
csthomas - comment - 17 May 2018

Thanks @laoneo,

3.9 is great.

avatar mbabker
mbabker - comment - 17 May 2018

Bug or not it still creates a B/C break as pointed out. And that's the reason I've been hesitant on hitting the merge button, this isn't "just" fixing some bug, it has user facing impact because it changes URI structure (even if the outcome is what is desired) and inherently changes what routes can be parsed and routed correctly.

avatar csthomas
csthomas - comment - 17 May 2018

We can try to add solutions like:

  1. I'm not a fun of this - add changes to a new class NomenuRules2 in order to not disturb 3rd party extensions. But it will be still a problem for core extensions.

  2. To eliminate error 404, add a workaround to parse old addresses by default, which gets 404 and redirect them to new URLs - it may not be an easy task.

avatar mbabker
mbabker - comment - 17 May 2018

Personally I don't like either one of the options. It just seems like it's asking for trouble. But so does blatantly merging something that everyone acknowledges is a B/C break.

avatar csthomas
csthomas - comment - 12 Jun 2018

Is there any decision from maintainers about what to do with this PR?

Should I move it to 4.0-dev branch or close it and we will wait for a better solution?

avatar franz-wohlkoenig franz-wohlkoenig - change - 13 Jun 2018
Status Ready to Commit Information Required
avatar franz-wohlkoenig
franz-wohlkoenig - comment - 18 Jun 2018

@laoneo can you please comment above Comment by @csthomas?

avatar franz-wohlkoenig franz-wohlkoenig - change - 23 Jun 2018
Status Information Required Discussion
avatar franz-wohlkoenig franz-wohlkoenig - change - 23 Jun 2018
Status Discussion Pending
avatar csthomas
csthomas - comment - 25 Jun 2018

Because I am not aware of the decision what to do next with this PR, this week I will try to move this PR to 4.0.

avatar csthomas
csthomas - comment - 4 Jul 2018

Closing in favour of PR #20979

avatar csthomas csthomas - change - 4 Jul 2018
Status Pending Closed
Closed_Date 0000-00-00 00:00:00 2018-07-04 22:23:19
Closed_By csthomas
Labels Removed: ?
avatar csthomas csthomas - close - 4 Jul 2018

Add a Comment

Login with GitHub to post a comment