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] Rewriting com_tags router #39114

Merged
merged 12 commits into from
Nov 16, 2022
Merged

[4.3] Rewriting com_tags router #39114

merged 12 commits into from
Nov 16, 2022

Conversation

Hackwar
Copy link
Member

@Hackwar Hackwar commented Oct 31, 2022

Pull Request for Issues #39105, #38996, #35382, #13598, #37444, #37492.

Summary of Changes

This PR rewrites the router of com_tags. Above you can see a whole list of issues which this should fix. Since these are very different issues, please don't close the issues right away without first testing this if it really fixes the issue.

This router improves a lot of things, but it also still is not perfect. While this better matches existing menu items, removes unnecessary URL parameters, allows for multiple tags per URL and fixes bugs in pagination and a few other areas, this still does not properly support language and multilanguage setups. It also fails to properly support nested tags.

The routes of the new router should be pretty much identical to the existing ones, except for where the current URLs aren't actually working. Right now the router can generate and parse URLs with several tags in one URL, where the IDs of the tags are handed over as an array in the non-SEF URL and a SEFed URL will have the aliases of those tags concatenated as segments of the URL.

Stuff not solved by this PR

The multilanguage issues are serious and I would love to solve them, but those fixes would build upon these changes here and these changes then again are already complex enough to test, so I'd like to first merge this here and then fix the language issues in another PR, especially since that PR would require a lot of changes to the models of the component.

The nested tags issue in the end means a new feature. Right now you can set a tag to be a child of another tag. However, URLs to a tag only contain the alias, not the complete path of the tag. This would be problematic when 2 child tags of different parents have the same alias. There is also no visual representation of this structure. My proposal would be to introduce an option to switch between the ability to select more than one tag per URL by using /tags/[tag1]/[tag2]/... or selecting a single, nested tag by using /tags/[tag1-level1]/[tag1-level2]/...

Another new feature could be to add a plus sign to tags in order to further filter a list of items down by adding yet another tag to that filter. Right now several tags in a menu item don't mean that only items with all tags associated to these items are shown, but all items which have at least one of those tags are displayed.

Testing Instructions

Pagination issue (fixes #35382 & #39105)

  1. Install testing sampledata
  2. Unpublish all tag related menu items (search for "tag" in the "all menu items" backend view)
  3. Create a tag which you then assign to at least 21 articles
  4. In the frontend, lookup one of the articles and click on the new tag
  5. In the single tag view for that tag, click on the second page.
  6. Without this change, this should result in an error. With this change, this should be fine.

Additional URL parameter (fixes #38996)

Please go to #38996 and follow the instructions there to rebuild the issue. After applying this change, this should be fixed.

Use the right menu item for tags (fixes #13598)

  1. Install testing sampledata
  2. Create two tags and create a menu item for one of the two with a single tag view
  3. Assign the new tags to an article
  4. Due to the "All Tags" menu item, after applying the change, the tag with the menu item should point to that menu item and the other should point to the all-tags menu item

Numeric tags don't work (fixes #37444)

  1. Install testing sampledata
  2. Create a tag with a numeric title
  3. Assign it to an article
  4. Try to click on that tag in the article in the frontend. Without this change, you don't get a valid URL, with this change, the right page is displayed.

Additional URL parameter v2 (#37492)

I can't reproduce this with this change applied. Can someone check if this is valid?

Improvements with these changes

  1. You can now have URLs with multiple tags. Such a non-SEFed URL would look like this: index.php?option=com_tags&view=tag&id[0]=1:alias-1&id[1]=2:alias-2...
  2. SEFed URLs with multiple tags would look like this: /[menu-item]/[tag-1]/[tag-2]/...

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

I don't know right now if we need documentation changes...

@Hackwar Hackwar marked this pull request as ready for review November 2, 2022 10:18
@richard67
Copy link
Member

Setting the release blocker label as inherited from issue #39105 .

components/com_tags/src/Helper/RouteHelper.php Outdated Show resolved Hide resolved
This was referenced Nov 3, 2022
@jamfx
Copy link

jamfx commented Nov 3, 2022

I have tested this item ✅ successfully on 8f23f4f

Followed the detailed instructions. Works as described. No errors found.


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

@Hackwar Hackwar requested a review from bembelimen November 8, 2022 13:59
@Hackwar Hackwar mentioned this pull request Nov 9, 2022
@chmst
Copy link
Contributor

chmst commented Nov 15, 2022

Test on a fresh installation (4.3 dev) with testing sample data and additional own data, not multilingual on localhost (sef urls on but no rewrite rule).

Could not replicate before applying the patch (successful per se)
Pagination issue (fixes #35382 & #39105)
Numeric tags don't work (fixes #37444)

Could not replicate: Additional URL parameter v2 (#37492)

Successful
Additional URL parameter (fixes #38996)
Use the right menu item for tags (fixes #13598)

Looks good for me but recommend testing with multilingual sites. No documentation change needed.


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

@chmst
Copy link
Contributor

chmst commented Nov 15, 2022

I have tested this item ✅ successfully on a3cd27e


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

@richard67
Copy link
Member

I've restored @jamfx 's test result since all changes after it were not really functional.

@richard67
Copy link
Member

RTC


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

@joomla-cms-bot joomla-cms-bot added the RTC This Pull Request is Ready To Commit label Nov 15, 2022
@obuisard obuisard added this to the Joomla! 4.3.0 milestone Nov 16, 2022
@obuisard obuisard merged commit 821cc42 into joomla:4.3-dev Nov 16, 2022
@joomla-cms-bot joomla-cms-bot removed the RTC This Pull Request is Ready To Commit label Nov 16, 2022
@obuisard
Copy link
Contributor

Thank you Hannes @Hackwar for this much needed PR!

@ch2856
Copy link

ch2856 commented Apr 20, 2023

J4.3.0
The new router adds ?Itemid=104 to all general tags (no menu)


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

@joeforjoomla
Copy link
Contributor

Confirmed. The new router has broken routing adding ?Itemid=xxx to all links despite the menu item created.

@NicolasDerumigny
Copy link
Contributor

I am unsure whether I should open an issue for it or just a comment, but when:

  • SEF Url is enable
  • a menu entry exists for "list all tags (with root as parent)", setting for example the alias tags for index.php?option=com_tags&view=tags&parent_id=1

Then tag URL are not rewritten from mysite.com/component/tags/tag/name_of_tag to mysite.com/tags/name_of_tag, though the latter is also valid URL and behave as expected.

Is this the correct behavior?

@Fedik
Copy link
Member

Fedik commented May 10, 2023

We have a PR that for fixing broken behavior of Tag router #40474. Please test, and report your result there. Thanks.

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