[#19945] - Proper selection of menu item, for URLs that do not specify a layout, but menu items both with and without layout exist
- Closed
- 20 Mar 2022
- Medium
- Build: staging
- # 19945
- Diff
- ggppdk:patch-59
- Success hound No violations found. Woof! Details
- Success continuous-integration/drone/pr the build was successful Details
- Pending JTracker/HumanTestResults Human Test Results: 1 Successful 0 Failed. Details
- Success continuous-integration/travis-ci/pr The Travis CI build passed Details
- Failure continuous-integration/appveyor/pr AppVeyor build failed Details
User tests: Successful: Unsuccessful:
More complete alternative to Pull Request #19516
Summary of Changes
During building menu item lookup table
when a menu item that has a layout is encountered, then 2 entries are added to the lookup table:
- [$view . $layout] ....... (an exact match for URLs that specify layout variable
- [$view] .......................... (a "fallback entry" in the case of URL without layout variable)
but the above causes later added (to the lookup table) menu items for the specific $view,
that do not specify a 'layout' to be ignored, as the "fallback entry" is not overwritten
PR #19516, suggests to remove the fallback entry, but then ... we have no fallback entry when needed
- this PR will add a distinct fallback 3rd entry indexed via ... [$view . ':default'] to be tried last
Order of checking the menu item lookup table
- [$view . $layout] ....... Same as before this PR, created when a menu item with layout is encountered
- [$view] ......................... Change, this entry is now added ONLY by menu items without 'layout'
- [$view . ':default'] ..... New, entry is added the first time that a menu item with layout is encountered
NOTE:
The lookup code is exactly to what it was before, just the exact same code is moved inside a loop that iterates (in order) the above 3 cases ! Please see that is identical here:
https://github.com/ggppdk/joomla-cms/blob/30f86c8133dd226d7a8da908ba4baf6d2a3f403e/libraries/src/Component/Router/Rules/MenuRules.php#L131-L149
Testing Instructions
Expected result
Actual result
Documentation Changes Required
None
| Status | New | ⇒ | Pending |
joomla-cms-bot
- | Category | ⇒ | Libraries |
| Title |
|
||||||
| Title |
|
||||||
| Labels |
Added:
?
|
||
| Category | Libraries | ⇒ | Libraries Unit Tests |
| Labels |
Added:
?
|
||
| Category | Libraries Unit Tests | ⇒ | Libraries |
| Labels |
Removed:
?
|
||
It would be good to add one unit test to protect this functionality. I did a code review and it looks OK.
Yes indeed, 1 more test unit would be nice,
when i get a clear mind again will try to add it, too late night now
if you can suggest a test please do as i am not sure what the new test should be
Returning to the original issue from #19943 and check below:
- Create a menu item A for com_content with category list (layout=default)
- Create a menu item B for com_contnet with category blog (layout=blog)
Before PR (single language) for category links we get the B item.
After PR we get the A item.
I understand that this PR was designed to do that, but J3 is probably not ready for that.
I have tested this item
Tested on a multilingual and a normal page.
echo JRoute::_('index.php?option=com_content&view=category&id=8&layout=default');
echo JRoute::_('index.php?option=com_content&view=category&id=8');
echo JRoute::_('index.php?option=com_content&view=category&id=8&layout=blog');
before patch:
/joomla/index.php/blog
/joomla/index.php/blog
/joomla/index.php/blog
after patch:
/joomla-39/index.php/list
/joomla-39/index.php/list
/joomla-39/index.php/blog
This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/19945.
FPerisa
-
Thanks anyone spending time on this PR
No interest in spending more on it by me
| Status | Pending | ⇒ | Closed |
| Closed_Date | 0000-00-00 00:00:00 | ⇒ | 2018-07-16 05:36:52 |
| Closed_By | ⇒ | ggppdk |
| Status | Closed | ⇒ | New |
| Closed_Date | 2018-07-16 05:36:52 | ⇒ | |
| Closed_By | ggppdk | ⇒ |
| Status | New | ⇒ | Pending |
set label ´Needs New Owner´.
| Status | Pending | ⇒ | Closed |
| Closed_Date | 0000-00-00 00:00:00 | ⇒ | 2022-03-20 11:07:45 |
| Closed_By | ⇒ | laoneo | |
| Labels |
Added:
Ready to take over
?
Removed: ? |
||
It would be good to add one unit test to protect this functionality. I did a code review and it looks OK.