Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[4.0] Fix error 404 in modern routing for links without own menu item (NomenuRules) #20979

Closed
wants to merge 2 commits into from

Conversation

csthomas
Copy link
Contributor

@csthomas csthomas commented Jul 4, 2018

Summary of Changes

This is a copy of #19280 without unit tests.

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.

@fancyFranci
Copy link
Contributor

I have tested this item ✅ successfully on 7ba153e

Links are working with and without IDs, even on a multilanguage site.


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

@infograf768
Copy link
Member

Multilingual

###NOTE:
I am testing here after correcting the info-block and any file related in order to add the category and parent category language to ContentHelperRoute::getCategoryRoute() as these are missing.

patchcontentcategory.diff.zip

Wanted to make a PR with this but as it is still broken, I will wait...

Results
Setting all types of com_content article or category display (separately or together).
Making sure category and parent category (with links) are set to display in the info-block.

Clicking on the category or parent category link in the infoblock gives correct results depending on the language of the category and its parent category.

If any is set to ALL, the urls are real bad.

I can get:
/en/featured-en-gb?view=category&id=19 Bad
or, if I have a menu item for the article
/en/othercatarticle?view=category&id=18 Bad

If all set to the same Content Language, it will use my hidden All Categories Menu item (alias: allcategories-en-gb) as base (correct result in 3.x with modern router).
/en/allcategories-en-gb/category-all Good.

Briefly said, when anything set to language ALL, sef links in 4.0 are badly broken, even after this patch.

I also tested with a custom menu item to use instead of blog (With xml and all).
I can create the menu item, it will display fine, except for the info-block category and parent category where the layout is appended &layout=cassiopeamyblog for example...

@Hackwar Any idea?

@csthomas
Copy link
Contributor Author

I can get:
/en/featured-en-gb?view=category&id=19 Bad
or, if I have a menu item for the article
/en/othercatarticle?view=category&id=18 Bad

This is another issue, more related to reversed PR #19099 and issue at #19537.

@infograf768
Copy link
Member

Solved here the issue with info-block and categories set to ALL languages.
Change has to be done in route.php
Will make PR for info-block as it is a separate issue than this PR

@infograf768
Copy link
Member

See #21916 concerning multilang
For com_content, as long as one has a List All Categories menu item, even hidden, no more 404 afaik.

@brianteeman
Copy link
Contributor

Requiring a "hidden menu item" is not a real solution

@csthomas
Copy link
Contributor Author

We have 2 issues, one is fixed by your PR another by this PR. Please do not mix it.
This PR is only for URL when there is no any menu item assigned.

@csthomas
Copy link
Contributor Author

csthomas commented Sep 3, 2018

@infograf768 Can you say that you have tested this PR?

@infograf768
Copy link
Member

let me retest in present 4.0-dev + #22022

@infograf768
Copy link
Member

@csthomas
Multilingual:
Settings
If we have absolutely no menu item to com_content stuff, i.e. if we have for example a home displaying a contact or a com_search menu item.

Then using as url index.php/component/content/categories just gives me a Category not found

Now, if I use the module articles_category, it will display articles and I can click on their title link or their category link.

After your PR, I get links like
/en/component/content/category/category-en-gb?Itemid=112

or
/en/component/content/article/category-en-gb/article-en-gb?Itemid=112

with always the same Itemid=112 , which is the itemid of the Home page whatever the menu item displayed (a contact, a search, whatever) before clicking on the links in the articlescategory module.

I never get the type of links you posted above
en/component/content/article/category-en-gb/article-en-gb

before your patch I do indeed get 404s.

Not sure I can set the test as successful.

@csthomas
Copy link
Contributor Author

csthomas commented Sep 7, 2018

Probably J4 start to add an Itemid everywhere, but this is not job for this PR to fix it. IMO this Itemid is redundant. @Hackwar do you want to add something?

@csthomas
Copy link
Contributor Author

csthomas commented Sep 7, 2018

@infograf768 Your test has been successful. J4 adds Itemid before and after the patch.

@infograf768
Copy link
Member

Any consideration about B/C as we had with #19280 ?
@wilsonge @mbabker

@laoneo
Copy link
Member

laoneo commented Sep 12, 2018

This here is a fix for a pretty obvious bug. As it is now ported to 4 it should be fine to be accepted as BC breaks can be tolerated.

@wilsonge
Copy link
Contributor

I want the unit tests ported as well please @rdeutz can you please work with @csthomas on that. Having stable routing unit tests is going to be super important for j4 so if we do need to make any more changes in the future we can understand the impact clearly

@csthomas
Copy link
Contributor Author

It may be hard to start as I have not found any tests on joomla routing.
https://github.com/joomla/test-unit/ ??
Please contact me at glip.

@Hackwar
Copy link
Member

Hackwar commented Sep 13, 2018

Idk where the tests are now, but I wrote full tests for all routing code...

@csthomas
Copy link
Contributor Author

@mbabker
Copy link
Contributor

mbabker commented Sep 13, 2018

Try https://github.com/joomla/test-integration (anyone got a map to actually find any of the tests anymore?)

@csthomas
Copy link
Contributor Author

Thanks, I found.

I see that I can use database in tests but I should not as @Hackwar mentioned in older PR.

@ghost ghost added the J4 Issue label Apr 5, 2019
@ghost ghost removed the J4 Issue label Apr 13, 2019
@roland-d
Copy link
Contributor

@wilsonge Should this be followed-up on?

@roland-d
Copy link
Contributor

roland-d commented Aug 1, 2020

Can anybody pick this up or should we close this PR?

@chmst
Copy link
Contributor

chmst commented Feb 13, 2021

I had the same issue as described in #32248.
The link is: ... /index.php/component/contact/contact/uncategorised/bb?Itemid=101
SEO friendly urls is ON
index.php/component/contact/contact/uncategorised/bb?Itemid=101

before patch: 404, after patch - the contat form is displayed with this url


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

@richard67
Copy link
Member

Closing in favour of #32695 .

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet