User tests: Successful: Unsuccessful:
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 404In 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
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.
Links work, no 404 when option Remove Ids
is enabled/disabled.
When option Remove Ids
is enabled then links to articles return error 404.
Status | New | ⇒ | Pending |
Category | ⇒ | Libraries Unit Tests |
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.
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.
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.
I'll update the site accordingly.
Thank you
@jsubri Please mark your test result here: https://issues.joomla.org/tracker/joomla-cms/19280
I have tested this item
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.
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.
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.
We can start at index.php/en/component/content/categories
.
Remeber to disable all menu items for categories/category/article views.
Build and parse:
index.php/en/component/content/category/8-category-en-gb
index.php/en/component/content/article/1-article-en-gb?catid=8
index.php/en/component/content/category/1190-subcategory-en-gb
index.php/en/component/content/article/4006-article-in-subcategory-en?catid=1190
Build ok and the parser results:
index.php/en/component/content/category/category-en-gb
index.php/en/component/content/article/article-en-gb?catid=8
index.php/en/component/content/category/subcategory-en-gb
index.php/en/component/content/article/article-in-subcategory-en?catid=1190
Parse old links:
index.php/en/component/content/category/8-category-en-gb
- (the same link)index.php/en/component/content/article/1-article-en-gb?catid=8
index.php/en/component/content/category/1190-subcategory-en-gb
index.php/en/component/content/article/4006-article-in-subcategory-en?catid=1190
Build and parse new links:
index.php/en/component/content/category/8-category-en-gb
index.php/en/component/content/article/8-category-en-gb/1-article-en-gb
index.php/en/component/content/category/8-category-en-gb/1190-subcategory-en-gb
index.php/en/component/content/article/8-category-en-gb/1190-subcategory-en-gb/4006-article-in-subcategory-en
Parse old links, which does not work before:
index.php/en/component/content/category/category-en-gb
index.php/en/component/content/article/article-en-gb?catid=8
index.php/en/component/content/category/subcategory-en-gb
index.php/en/component/content/article/article-in-subcategory-en?catid=1190
Build and parse new links:
index.php/en/component/content/category/category-en-gb
index.php/en/component/content/article/category-en-gb/article-en-gb
index.php/en/component/content/article/category-en-gb/subcategory-en-gb/article-in-subcategory-en
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.
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:
Labels |
Added:
?
?
|
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.
I have tested this item
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
My test is for single language website only.
I have tested this item
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.
Status | Pending | ⇒ | Ready to Commit |
Ready to Commit after two successful tests.
Has someone tested this with multilingual?
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.
This PR works in the same way in a single or multilingual environment. This is independent.
At now IMO this PR is not needed. I have plan to open a new PR on J4.
Status | Ready to Commit | ⇒ | Closed |
Closed_Date | 0000-00-00 00:00:00 | ⇒ | 2018-02-05 22:34:35 |
Closed_By | ⇒ | csthomas | |
Labels |
Added:
?
|
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?
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
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
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.
Hi,
I can not get it to work, I explain a little more detail
this menu link to http://site.com/index.php?option=com_virtuemart&view=category&virtuemart_category_id=2&virtuemart_manufacturer_id=0
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.
Maybe I do not understand something well, excuse my English
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.
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
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.
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.
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.
Status | Closed | ⇒ | New |
Closed_Date | 2018-02-05 22:34:35 | ⇒ | |
Closed_By | csthomas | ⇒ | |
Labels |
Removed:
?
|
Status | New | ⇒ | Pending |
I have tested this item
Status | Pending | ⇒ | Ready to Commit |
Ready to Commit after 3 successful tests.
:)
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.
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.
Can we push it forward until not many people use it?
Labels |
Added:
?
?
|
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.
We can try to add solutions like:
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.
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.
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.
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?
Status | Ready to Commit | ⇒ | Information Required |
Status | Information Required | ⇒ | Discussion |
Status | Discussion | ⇒ | Pending |
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.
Status | Pending | ⇒ | Closed |
Closed_Date | 0000-00-00 00:00:00 | ⇒ | 2018-07-04 22:23:19 |
Closed_By | ⇒ | csthomas | |
Labels |
Removed:
?
|
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".