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.3] Routing: enforcing segment comparison for categories #39803

Closed
wants to merge 2 commits into from

Conversation

Hackwar
Copy link
Member

@Hackwar Hackwar commented Feb 6, 2023

Summary of Changes

The content component routers have a situation where they behave unintentional. When a category has a subcategory and a content item (for example an article) with the same ID and the "remove IDs" option is disabled, the router will choose the subcategory over the content item, regardless of the alias. This is because the router compares the current segment it processes against the available childcategories and only if that fails, it switches over to checking against the articles.

Up till now, the check only compared the leading ID of the segment and not the complete segment with ID AND alias. This PR changes that and tightens the comparison. This would fix the above mentioned issue and prevent broken URLs from working. That last part however is also the biggest issue. If people actually rely on broken URLs (for example pasting them into a field which actually isn't long enough for the pasted content.) this would break those URLs.

Unfortunately an automatic fallback is not possible. If we have this new behavior and then fall back on the old behavior when no match was made, we would end up at exactly the same point we are now. I also would strongly vote against adding another option for this, because I don't think we can convoy the function of such an option to our users. It would lead to some cargo cult behavior, I fear.

Since this is a potential backwards compatibility issue, I'm asking for a vote from the production department on this PR.

Testing Instructions

  1. Create a category (henceforth named A) with another category as subcategory (henceforth named B).
  2. Create articles in the category A until you have one with the same ID as the subcategory.
  3. Create a menu item to the category A
  4. Try to navigate to category B

Actual result BEFORE applying this Pull Request

The article with the same ID is displayed.

Expected result AFTER applying this Pull Request

The correct subcategory is displayed.

Link to documentations

Please select:

  • Documentation link for docs.joomla.org:

  • No documentation changes for docs.joomla.org needed

  • Pull Request link for manual.joomla.org:

  • No documentation changes for manual.joomla.org needed

@HLeithner
Copy link
Member

As this is a even worse b/c break I'm against it.

@wilsonge
Copy link
Contributor

wilsonge commented Feb 6, 2023

So I went back through the old router and in this case we used to check the alias of article and category as the distinguishing factor

https://github.com/joomla/joomla-cms/blob/3.10-dev/components/com_content/helpers/legacyrouter.php#L352
https://github.com/joomla/joomla-cms/blob/3.10-dev/components/com_content/helpers/legacyrouter.php#L370

So I think actually PR is probably correct because it restores the previous behaviour.

@Hackwar
Copy link
Member Author

Hackwar commented Feb 7, 2023

This PR was discussed in the Production Team meeting on 2023-02-07 and the decision was to close this PR, as the initial issue is very rare and can be fixed on a per-site-basis and the issue of lacking precision of the router is fixed by switching off the IDs in the components configuration. So the fix for this problem is to disable IDs in the URLs, as is done by default on new installations. At the same time, the fall out from simply doing this change in a release seems far to big to do it otherwise.

@Hackwar Hackwar closed this Feb 7, 2023
@Hackwar Hackwar deleted the 4.3-routing-alias-check branch February 7, 2023 18:34
@brianteeman
Copy link
Contributor

Will this remedy etc be documented?

@Hackwar
Copy link
Member Author

Hackwar commented Feb 7, 2023

Feel free to document it somewhere. I will not invest more time into this.

@wilsonge
Copy link
Contributor

This is ridiculous. Any article with the same id as a category can’t be routed with legacy routing and this is the same behaviour as legacy routing in j3.

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

5 participants