[#43164] - [5.2] SEF/Router: Only merge default menu item when same component
- Fixed in Code Base
- 24 Jul 2024
- Medium
- Build: 5.2-dev
- # 43164
- Diff
- Hackwar:5.2-router-default
- Pending continuous-integration/drone/pr Build is pending Details
User tests: Successful: Unsuccessful:
This fixes issues #43154, #35463 and #39149
Summary of Changes
The router right now merges the query elements of the current menu item into the query it parsed so far. If no menu item was found, it merges in the values of the default menu item. This is quite a problem when the default menu item contains query elements which are not overwritten by the URL itself. One example would be our own schedule runner plugin with a default menu item linking to an article. This PR only merges in the menu items query when the menu item and the parsed option are identical. Most likely this doesn't solve the issue entirely, since you could have a component which still could fail in this constellation, but at least it wouldn't fail anymore for all the components out there which currently have the query from an article default menu item merged into their request.
Testing Instructions
- Switch the default menu item to a single article view and select a random article.
- Edit plugins/system/schedulerunner/src/Extension/ScheduleRunner.php and add
var_dump($id);die;to line 206 - Go to System -> Scheduled Tasks -> Options -> WebCron and enable Web Cron.
- Save the options and later copy the link from that same place to run the webcron via ajax call.
- Open the link in a new tab.
Actual result BEFORE applying this Pull Request
The var_dump() returns a non-zero ID, even though there is no ID in the URL.
Expected result AFTER applying this Pull Request
The var_dump() correctly only returns a 0.
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
| Status | New | ⇒ | Pending |
joomla-cms-bot
- | Category | ⇒ | Libraries |
I have tested this item ✅ successfully on 773a13f
Thanks a lot for your patch. I've tested it and the task id is returning zero now.
This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/43164.
| Labels |
Added:
PR-5.2-dev
|
||
I have tested this item ✅ successfully on 8accc62
This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/43164.
| Status | Pending | ⇒ | Ready to Commit |
RTC
This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/43164.
| Labels |
Added:
RTC
|
||
| Title |
|
||||||
| Labels |
Added:
Feature
|
||
| Category | Libraries | ⇒ | Libraries JavaScript Unit Tests |
| Labels |
Added:
Unit/System Tests
|
||
Surely having to change an existing test that passes without this PR in order for it to pass with this PR is a red flag that something is wrong with this PR
Good to get this bug fixed! It shouldn't merge in the parameters of the home menuitem.
However, the code is now not setting the active menuitem. This is a problem as extension developers do use this.
In Joomla 3.x the active menuitem wasn't set in the case of the home menuitem being used as default.
Then in Joomla 4 it changed to being set.
So in my opinion we shouldn't be now changing it back to the case where it's not set.
If the balance of opinion is that it shouldn't be set then this side-effect needs to be clearly highlighted in the documentation of the pull request.
See https://docs.joomla.org/Menu_and_Menuitems_API_Guide#Active_Menu_Item and
https://manual.joomla.org/docs/general-concepts/menus-menuitems#active-menu-item
| Category | Libraries JavaScript Unit Tests | ⇒ | Libraries |
Thank you @robbiejackson. Your comment made me check this again and do a debugging session of several hours and after several checks I indeed agree that we should still set the active menu item. I changed the code accordingly. Good catch. Our code actually states that getActive() might return null when no menu item is set, but who reads docblocks? ;-)
@uGE70, @SniperSister, @robbiejackson could you test this again so that we hopefully can merge this into 5.2? Thank you!
Surely having to change an existing test that passes without this PR in order for it to pass with this PR is a red flag that something is wrong with this PR
whoever could have predicted that you dont edit a test to fix a problem
I have tested this item ✅ successfully on 359bbde
This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/43164.
I have tested this item ✅ successfully on 359bbde
Thanks for changing to keep the active menuitem :-)
I'll update the joomla manual documentation to confirm that the menuitem is now always set.
This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/43164.
@robbiejackson this is actually something which I would keep vague for now. The getActive() method states that it can also return null and I'm currently still considering it to be more correct to not set the menu item in this case. However that would be a b/c break and thus has to wait until at least 6.0.
Thanks for the tests!
| Labels |
Removed:
Unit/System Tests
|
||
I have tested this item ✅ successfully on bfa035f
This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/43164.
I have tested this item ✅ successfully on bfa035f
This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/43164.
rdeutz
- | Status | Ready to Commit | ⇒ | Fixed in Code Base |
| Closed_Date | 0000-00-00 00:00:00 | ⇒ | 2024-07-24 16:15:08 |
| Closed_By | ⇒ | rdeutz |
should'nt this be backported to 4.4 ? as per #43154