? Pending

User tests: Successful: Unsuccessful:

avatar ditsuke
ditsuke
29 Jun 2021

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

avatar ditsuke ditsuke - open - 29 Jun 2021
avatar ditsuke ditsuke - change - 29 Jun 2021
Status New Pending
avatar joomla-cms-bot joomla-cms-bot - change - 29 Jun 2021
Category Libraries
avatar ditsuke ditsuke - change - 29 Jun 2021
The description was changed
avatar ditsuke ditsuke - edited - 29 Jun 2021
avatar RickR2H RickR2H - test_item - 29 Jun 2021 - Tested successfully
avatar RickR2H
RickR2H - comment - 29 Jun 2021

I have tested this item successfully on 3ac41de

Patch removed the unwanted query.


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

avatar Fedik
Fedik - comment - 30 Jun 2021

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

avatar richard67
richard67 - comment - 30 Jun 2021

@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?

avatar ditsuke
ditsuke - comment - 30 Jun 2021

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 ?

avatar Hackwar
Hackwar - comment - 30 Jun 2021

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.

avatar ditsuke ditsuke - change - 30 Jun 2021
Labels Added: ? ?
avatar joomla-cms-bot joomla-cms-bot - change - 30 Jun 2021
Category Libraries Administration Libraries
avatar joomla-cms-bot joomla-cms-bot - change - 30 Jun 2021
Category Libraries Administration Libraries
avatar ditsuke ditsuke - change - 30 Jun 2021
Labels Added: ?
Removed: ?
avatar ditsuke
ditsuke - comment - 30 Jun 2021

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

avatar wilsonge
wilsonge - comment - 30 Jun 2021

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?

avatar ditsuke ditsuke - change - 1 Jul 2021
Labels Added: ?
Removed: ?
avatar ditsuke
ditsuke - comment - 1 Jul 2021

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.

avatar joomleb
joomleb - comment - 15 Aug 2021

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

avatar ditsuke ditsuke - change - 15 Aug 2021
Title
Clean filter_tag query from SEF URLs
[4.0] Clean filter_tag query from SEF URLs
avatar ditsuke ditsuke - edited - 15 Aug 2021
avatar Hackwar
Hackwar - comment - 15 Aug 2021

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

avatar ditsuke
ditsuke - comment - 16 Aug 2021

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?

avatar joomleb
joomleb - comment - 16 Aug 2021

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?

avatar ditsuke ditsuke - change - 20 Aug 2021
Labels Added: ?
Removed: ? ?
avatar joomleb
joomleb - comment - 1 Oct 2021

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

avatar joomleb
joomleb - comment - 8 Jan 2022

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

avatar chmst chmst - change - 31 Jan 2022
Labels Added: ?
Removed: ?
avatar joomleb
joomleb - comment - 11 May 2022

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

avatar brianteeman
brianteeman - comment - 11 May 2022

Presumably not unless the comment from @Hackwar is resolved

avatar joomleb
joomleb - comment - 13 May 2022

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

avatar HLeithner
HLeithner - comment - 27 Jun 2022

This pull request has automatically rebased to 4.2-dev.

avatar joomla-bot
joomla-bot - comment - 27 Jun 2022

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

avatar level420
level420 - comment - 26 Aug 2022

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?

avatar joomleb
joomleb - comment - 20 Sep 2022

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...

avatar joomleb
joomleb - comment - 17 Mar 2023

@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 ?

avatar HLeithner
HLeithner - comment - 21 Mar 2023

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

avatar wilsonge
wilsonge - comment - 22 Mar 2023

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.

avatar wilsonge wilsonge - close - 22 Mar 2023
avatar wilsonge wilsonge - change - 22 Mar 2023
Status Pending Closed
Closed_Date 0000-00-00 00:00:00 2023-03-22 02:47:20
Closed_By wilsonge
Labels Added: ?
Removed: ?
avatar HLeithner
HLeithner - comment - 22 Mar 2023

@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?

avatar wilsonge
wilsonge - comment - 28 Mar 2023

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

avatar HLeithner
HLeithner - comment - 29 Mar 2023

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.

avatar joomleb
joomleb - comment - 23 May 2023

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

avatar ditsuke
ditsuke - comment - 23 May 2023

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.

Add a Comment

Login with GitHub to post a comment