[4.0] Clean filter_tag query from SEF URLs

[ditsuke]

Pull Request for Issue #34527.

This should fix occurrence of unwanted queries in Menu links with SEF URLs enabled.

Summary of Changes

Unset filter_tag query in SiteRouter's SEF route builder.

Testing Instructions

Create menu item that filters articles by tag. If SEF is enabled, URLs currently retain an unwanted filter_tag query.

Actual result BEFORE applying this Pull Request

Menu items that link to tag filtered category blogs are polluted with filter_tag query with SEF URLs enabled.

Expected result AFTER applying this Pull Request

Menu items that link to tag filtered results shouldn't have redundant filter_tag query.

Documentation Changes Required

None

[RickR2H]

I have tested this item :white_check_mark: successfully on 3ac41dedf391c137ec5a1cc88e86f918f0a869ad

Patch removed the unwanted query.

---

_{This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/34652.}

[Fedik]

You should not do it in SiteRouter, it need be done in component router level, somehow

[richard67]

@Hackwar Could you have a look on this PR here and the linked issue #34527 ? Is it really an issue, and is this here the right way to fix it?

[ditsuke]

You should not do it in SiteRouter, it need be done in component router level, somehow

Would it be more appropriate if I implement it in Router\Rules\NomenuRules' build rule or a new RulesInterface implementation in com_content ?

[Hackwar]

This is definitely not the right place to remove this. This would be something that would have to be implemented by the component router. This would have to probably be fixed in the StandardRules.php. But please don't simply remove the filter_tag, but cycle through the menu items query elements and remove them one by one.

[ditsuke]

@Hackwar I've implemented changes in the class you suggested. Can you have a look again?

[wilsonge]

This just doesn't look right to me. Query parameters are used in various places even in core (e.g. the layout param, tpl param, I'd also guess (but not 100%) that the pagination on tables uses it too). And we can't predict what 3rd parties are going to do. What problem are you trying to solve here?

[ditsuke]

This just doesn't look right to me. Query parameters are used in various places even in core (e.g. the layout param, tpl param, I'd also guess (but not 100%) that the pagination on tables uses it too). And we can't predict what 3rd parties are going to do. What problem are you trying to solve here?

@wilsonge I think it's undesirable to have redundant queries in SEF URLs when the corresponding menu entry already has them. The linked issue had an instance of the filter_tag queries being retaining when the SEF route already accounted for it. My initial change only stripped the filter_tag queries (in the SiteRouter) but on suggestion of Hackwar I implemented stripping of all queries also found identical in the Menu entry in StandardRules. If this is breaking something, I suppose we need to place the cleanup at the step where other routines are already supposed to have dealt with the queries in their own ways and as this to ensure the final URL reflected in the front end is clean (it will retain queries the menu entry doesn't have).

As for the layout param, if you look at cleanupQuery() the behavior is consistent with how it was being unset before.

[joomleb]

Hi guys, tested with success. Please, Will this file be added to the 4.0 stable release ?

[Hackwar]

This is not the right solution to the problem. Please don't add this. It breaks more than it fixes.

[ditsuke]

This is not the right solution to the problem. Please don't add this. It breaks more than it fixes.

@Hackwar I would really like to get this issue fixed. If this approach breaks things, can you please point me in the right direction to solve this?

[joomleb]

This is not the right solution to the problem. Please don't add this. It breaks more than it fixes.

Hi @Hackwar I suppose you are referring to:

This is definitely not the right place to remove this. This would be something that would have to be implemented by the component router. This would have to probably be fixed in the StandardRules.php. But please don't simply remove the filter_tag, but cycle through the menu items query elements and remove them one by one.

Please, What breaks did you find?

[joomleb]

Hi guys, Is there any chance to see this fix added in 4.1 version ?

[joomleb]

Hi guys, sorry, but, What about this fix and 4.1 version release ? Will it be added ? What is planned for this issue ?

[joomleb]

Hi guys, Please, Is there any merge plan here to the next upcoming ?

[brianteeman]

Presumably not unless the comment from @Hackwar is resolved

[joomleb]

Hi @brianteeman thanks for reply. Yes, it seems like @ditsuke is still waiting more details from @Hackwar...

[HLeithner]

This pull request has automatically rebased to 4.2-dev.

[joomla-bot]

This pull requests has been automatically converted to the PSR-12 coding standard.

[level420]

So @Hackwar did not answer since about a year now. I'm also waiting for this fix, but nothing happens. How to proceed with this issue/PR?

[joomleb]

This is not the right solution to the problem. Please don't add this. It breaks more than it fixes.

@Hackwar I would really like to get this issue fixed. If this approach breaks things, can you please point me in the right direction to solve this?

We stop here, @ditsuke comment on 16 Aug 2021

So @Hackwar did not answer since about a year now. I'm also waiting for this fix, but nothing happens. How to proceed with this issue/PR?

@level420 A bug discussed over and over by many people and literally "abandoned". Since August 2005 it is the first time that I happen to see such a thing in Joomla...

[joomleb]

@ditsuke @chmst @HLeithner @rdeutz I suppose it is time to change PR-4.2-dev label to PR-4.3-dev here.

Please, Does anyone have any news on this ticket ?

[HLeithner]

I suppose it is time to change PR-4.2-dev label to PR-4.3-dev here.

Unlikely it goes to 4.3 nor 4.4, when rebased I would change it to 5.0, but that has to be clarified first

[wilsonge]

I've reopened the original issue and am closing this PR. I'm sorry but forcibly removing all query params without a match in the router at the library level is a recipe to breaking things in 3rd party components - many of which use query params for things such as pagination.

[HLeithner]

@wilsonge if I read the code correctly only query parameter which are set in the menu will be removed and only if the value is equal. At least what I have seen in the cleanupQuery() function

What did I missed?

[wilsonge]

https://github.com/joomla/joomla-cms/pull/34652/files#diff-10c2ab3e362cc27a7f0e2b18ee0b59c909041a35db8c9c00abe617e8cd982061R279

Maybe I'm loosing it but that keeps the 3 mentioned ones and unsets everything else? Hence filter_tag isn't explicitly listed despite that's the problem it solves

[HLeithner]

Nope, look at https://github.com/joomla/joomla-cms/pull/34652/files#diff-10c2ab3e362cc27a7f0e2b18ee0b59c909041a35db8c9c00abe617e8cd982061R281

it only unset get entries which are not in the menu item explicit set and has the same value as the get parameter provided to the router.

[joomleb]

Hi guys, this should be solved by the #40225 fixing, Right?

[ditsuke]

Hi guys, this should be solved by the #40225 fixing, Right?

Should be. On a cursory glance it looks the same as this one except it doesn't generalise for parameters beyond filter_tag.