[5.2] SEF/Router: Only merge default menu item when same component

[Hackwar]

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

1. Switch the default menu item to a single article view and select a random article.
2. Edit plugins/system/schedulerunner/src/Extension/ScheduleRunner.php and add var_dump($id);die; to line 206
3. Go to System -> Scheduled Tasks -> Options -> WebCron and enable Web Cron.
4. Save the options and later copy the link from that same place to run the webcron via ajax call.
5. 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:

• [X] No documentation changes for docs.joomla.org needed

• [ ] Pull Request link for manual.joomla.org:

• [X] No documentation changes for manual.joomla.org needed

[alikon]

should'nt this be backported to 4.4 ? as per #43154

[uGE70]

I have tested this item :white_check_mark: successfully on 773a13fb200ff310815339df5c9d95eb855098e8

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

[Hackwar]

should'nt this be backported to 4.4 ? as per #43154

I'm unsure how this should be handled and asked the cms maintainer group about this. waiting for their feedback.

[SniperSister]

I have tested this item :white_check_mark: successfully on 8accc62b5515e7a52c38e53090d42e2f8a52a38c

---

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

[Quy]

RTC

---

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

[brianteeman]

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

[robbiejackson]

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

[Hackwar]

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? ;-)

[Hackwar]

@uGE70, @SniperSister, @robbiejackson could you test this again so that we hopefully can merge this into 5.2? Thank you!

[brianteeman]

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

[uGE70]

I have tested this item :white_check_mark: successfully on 359bbde9bde568b5fb059420b5e0eedba9d8630b

---

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

[robbiejackson]

I have tested this item :white_check_mark: successfully on 359bbde9bde568b5fb059420b5e0eedba9d8630b

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

[Hackwar]

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

[Hackwar]

Thanks for the tests!

[zero-24]

I have tested this item :white_check_mark: successfully on bfa035f60d3361419303f7bac34019f4612f5959

---

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

[SniperSister]

I have tested this item :white_check_mark: successfully on bfa035f60d3361419303f7bac34019f4612f5959

---

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