User tests: Successful: Unsuccessful:
Pull Request for Issue #34527.
This should fix occurrence of unwanted queries in Menu links with SEF URLs enabled.
Unset filter_tag query in SiteRouter's SEF route builder.
Create menu item that filters articles by tag. If SEF is enabled, URLs currently retain an unwanted filter_tag
query.
Menu items that link to tag filtered category blogs are polluted with filter_tag
query with SEF URLs enabled.
Menu items that link to tag filtered results shouldn't have redundant filter_tag
query.
None
Status | New | ⇒ | Pending |
Category | ⇒ | Libraries |
You should not do it in SiteRouter,
it need be done in component router level, somehow
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
?
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.
Labels |
Added:
?
?
|
Category | Libraries | ⇒ | Administration Libraries |
Category | Libraries Administration | ⇒ | Libraries |
Labels |
Added:
?
Removed: ? |
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?
Labels |
Added:
?
Removed: ? |
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.
Hi guys,
tested with success. Please, Will this file be added to the 4.0 stable release ?
Title |
|
This is not the right solution to the problem. Please don't add this. It breaks more than it fixes.
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?
Labels |
Added:
?
Removed: ? ? |
Hi guys,
Is there any chance to see this fix added in 4.1 version ?
Hi guys,
sorry, but, What about this fix and 4.1 version release ? Will it be added ?
What is planned for this issue ?
Labels |
Added:
?
Removed: ? |
Hi guys,
Please, Is there any merge plan here to the next upcoming ?
Hi @brianteeman
thanks for reply. Yes, it seems like @ditsuke is still waiting more details from @Hackwar...
This pull request has automatically rebased to 4.2-dev.
This pull requests has been automatically converted to the PSR-12 coding standard.
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...
@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 ?
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
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.
Status | Pending | ⇒ | Closed |
Closed_Date | 0000-00-00 00:00:00 | ⇒ | 2023-03-22 02:47:20 |
Closed_By | ⇒ | wilsonge | |
Labels |
Added:
?
Removed: ? |
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
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.
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.