If the link is the same as active menu item then use it #18260

csthomas · 2017-10-06T22:05:39Z

Pull Request for Issue #17719 and #16774

Summary of Changes

If link Itemid is equal to active menu and link query is equal to active query then use it, do not try to find a better match.
Add unit tests to prove that.

Testing Instructions

Use tests from #17719 or #16774 or this below.

Turn off Search Engine Friendly URLs in Global Configuration
Create menu item (Style 1) to an article, save, then save as copy (Style 2), publish and save and close.
Go to front-page and click on the first one (Style 1), check both URLs, it is OK.
Go to second (Style 2), and check link at (Style 2), the Itemid in URL point to (Style 1) instead to itself (Style 2).

Expected result

Active second menu item (Style 2) should have URL to (Style 2)

Actual result

Active second menu item (Style 2) use first menu item URL (Style 1)

Documentation Changes Required

No

csthomas · 2017-10-06T22:13:55Z

@arrowthemes @weeblr

This small patch should fix your issues and it has chance to be merged in J3.8.x than my previous one #17746 that won't be merged in J3.8

To test you have to replace only one file libraries/src/Component/Router/Rules/MenuRules.php.

esedic · 2017-10-07T09:27:34Z

I have tested this item ✅ successfully on 94fae0c

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

ggppdk · 2017-10-08T04:18:23Z

libraries/src/Component/Router/Rules/MenuRules.php

+				// If the same view has two different menu items and one of them is active then use the active one
+				return;
+			}
+		}


This does not seem to be enough, it will fail in cases of URLs that have id variable, because
$activeQuery['id'] is id
$query['id'] is id:alias

so the equality comparison will fail, but its should have succeed, right ?

infograf768 · 2017-10-08T07:17:56Z

Not sure this works.
Test with 3 menu items displaying same article:

When clicking a second time on the same menu item, the itemid changes, except for the last one.
This screencapture displays the url(s) obtained at the bottom of the window.

infograf768 · 2017-10-08T08:10:09Z

@csthomas
Note: I also tested #17746 with the same menu items and when clicking a second time on the same menu item, it changes from the menu item itemid to the Home page menu item itemid.

ggppdk · 2017-10-08T10:06:31Z

@infograf768
it must be because of:
#18260 (comment)

csthomas · 2017-10-08T12:50:02Z

Thanks @ggppdk @infograf768

PR #17746 after the last commit is broken for this test.

I added a new commit. I tested an article with menu styles with success.
Please test again.

infograf768 · 2017-10-08T14:16:43Z

At first sight, it was working OK, i.e. I got unique itemids for each of the menu items, even when clicking again on the same menu... but when clicking on the category link in the Article Info Category link, I get
[08-Oct-2017 16:12:19 Europe/Berlin] PHP Warning: explode() expects parameter 2 to be string, array given in /Applications/MAMP/htdocs/testnew/trunkgitnew/libraries/src/Component/Router/Rules/MenuRules.php on line 83

csthomas · 2017-10-08T20:28:00Z

The problem was filter_key that is an array in category view.
As filter_key is not a part of routing then I use now only option, view, key (usual it is id) and layout.

infograf768 · 2017-10-09T08:53:34Z

@csthomas
Your last changes solve the category display warning, but we still have an error.

Test:
Create a menu item of type Create an article
Create a menu item of type Login form with option set to redirect to the Create an article menu item.
Login via this login form. url is here http://localhost:8888/testnew/trunkgitnew/index.php?option=com_users&view=login&Itemid=135&lang=en

Login is done OK, but I get a notice:
[09-Oct-2017 10:46:22 Europe/Berlin] PHP Notice: Undefined index: a_id in /Applications/MAMP/htdocs/testnew/trunkgitnew/libraries/src/Component/Router/Rules/MenuRules.php on line 103

The Notice is the same in multilang whether the redirect is set to a Create an article tagged to the same language as the login form or to another language.

infograf768 · 2017-10-09T09:06:21Z

In fact clicking directly on the Create article menu item also sends the Notice.
URL is:
http://localhost:8888/testnew/trunkgitnew/index.php?option=com_content&view=form&layout=edit&a_id=0&Itemid=134&lang=en

infograf768 · 2017-10-09T09:22:19Z

TBH, I am a bit concerned about B/C here.
I have only tested stuff concerning com_content, but are'nt we going to find other issues in other extensions, including 3rd party?

csthomas · 2017-10-09T13:37:18Z

There is still an issue with B/C now.

infograf768 · 2017-10-09T14:49:34Z

Last commit solved the Notice for Create Article menu item.

csthomas · 2017-10-09T15:26:30Z

As I can not use $view->key because it is only in newer routing I back to simpler way and only compare strings, if in query has array then this PR change nothing.

csthomas · 2017-10-09T18:33:50Z

~~Current version does not work for category view because category view has query parameter filter_tag that is not used in build lookup. I can add a workaround for that by adding code in loop~~

~~if (strpos($k, 'filter_') === 0) continue; that will ignore every filter parameters.~~

infograf768 · 2017-10-10T08:52:54Z

@csthomas

If someone care about category view then such improvement could be add later.

What is the exact issue you have with category view?

infograf768 · 2017-10-10T09:01:20Z

What I remarked (and I do not see how to solve that aspect except by a new parameter), is that

a link to article 1 in an article will always display the article1 menu item with highest id
same for a link to a category in the article info when one has 2 single category menu items.
Example a category blog has itemid 117 and category list or blog is itemid 136, then 136 will display.

csthomas · 2017-10-10T10:28:26Z

Joomla routing is not a simple path.

Version for monolingual (If you do not use language parameter):

if you want to route to article then you look up for the lowest menu item id that match criteria.
Version for multilingual (If appended parameter lang=...):
if you want to route to article then you look up for the highest menu item id that match criteria.

This is very weird but probably there ware some B/C issue.

The code is at https://github.com/joomla/joomla-cms/blob/staging/libraries/src/Component/Router/Rules/MenuRules.php#L206 - the trick one

[Deleted]
...

csthomas · 2017-10-10T12:47:46Z

same for a link to a category in the article info when one has 2 single category menu items.
Example a category blog has itemid 117 and category list or blog is itemid 136, then 136 will display.

[Updated]

It would be good to add new column/field "Routing Priority" to each menu item. This way we can manage which menu item (category menu) should be used for routing

After that you can set higher priority for menu item 117.
Then only first category menu will be used for articles routing.

csthomas · 2017-10-10T13:20:52Z

Going back to this PR,

If someone care about category view then such improvement could be add later.

What is the exact issue you have with category view?

I'm surprised that it works for category menus.
Ok, then It is completed. Please test.

infograf768 · 2017-10-12T08:05:55Z

I have tested this item ✅ successfully on 8740d71

No more Notices or warnings.
Modules assignment work fine.

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

ghost · 2017-10-12T09:10:33Z

@esedic can you please retest?

esedic · 2017-10-12T12:19:00Z

I have tested this item ✅ successfully on 8740d71

@franz-wohlkoenig, done

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

ghost · 2017-10-12T13:04:59Z

RTC after two successful tests.

If the link is the same as active menu item then use it

94fae0c

joomla-cms-bot added PR-staging Unit/System Tests labels Oct 6, 2017

ggppdk reviewed Oct 8, 2017

View reviewed changes

Add a better check for query params with alias like id:alias

3347526

Use router->view->key to compare menu items

30749ae

Check language too

3a5506e

Move buildLookup upper to add default value for view->key

6e1e3d9

Back to simpler way

8740d71

joomla-cms-bot added the RTC This Pull Request is Ready To Commit label Oct 12, 2017

infograf768 added this to the Joomla 3.8.2 milestone Oct 12, 2017

ggppdk mentioned this pull request Oct 15, 2017

Always add rel canonical in article view (? rare issue of alternative URLs found by search engines) #18341

Closed

eSilverStrike mentioned this pull request Oct 15, 2017

Canonical Links does not use Preferred url #18318

Closed

mbabker merged commit 074c618 into joomla:staging Oct 15, 2017

joomla-cms-bot removed the RTC This Pull Request is Ready To Commit label Oct 15, 2017

csthomas deleted the use_active_itemid_if_match branch October 15, 2017 18:16

csthomas mentioned this pull request Mar 19, 2018

Proper selection of menu item, for URLs that do not specify a layout, but menu items both with and without layout exist #19945

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

If the link is the same as active menu item then use it #18260

If the link is the same as active menu item then use it #18260

csthomas commented Oct 6, 2017 •

edited by joomla-cms-bot

Loading

csthomas commented Oct 6, 2017

esedic commented Oct 7, 2017

ggppdk Oct 8, 2017

infograf768 commented Oct 8, 2017

infograf768 commented Oct 8, 2017

ggppdk commented Oct 8, 2017

csthomas commented Oct 8, 2017 •

edited

Loading

infograf768 commented Oct 8, 2017

csthomas commented Oct 8, 2017

infograf768 commented Oct 9, 2017

infograf768 commented Oct 9, 2017

infograf768 commented Oct 9, 2017

csthomas commented Oct 9, 2017

infograf768 commented Oct 9, 2017

csthomas commented Oct 9, 2017

csthomas commented Oct 9, 2017 •

edited

Loading

infograf768 commented Oct 10, 2017

infograf768 commented Oct 10, 2017

csthomas commented Oct 10, 2017 •

edited

Loading

csthomas commented Oct 10, 2017 •

edited

Loading

csthomas commented Oct 10, 2017

infograf768 commented Oct 12, 2017

ghost commented Oct 12, 2017

esedic commented Oct 12, 2017 •

edited

Loading

ghost commented Oct 12, 2017

If the link is the same as active menu item then use it #18260

If the link is the same as active menu item then use it #18260

Conversation

csthomas commented Oct 6, 2017 • edited by joomla-cms-bot Loading

Summary of Changes

Testing Instructions

Expected result

Actual result

Documentation Changes Required

csthomas commented Oct 6, 2017

esedic commented Oct 7, 2017

ggppdk Oct 8, 2017

Choose a reason for hiding this comment

infograf768 commented Oct 8, 2017

infograf768 commented Oct 8, 2017

ggppdk commented Oct 8, 2017

csthomas commented Oct 8, 2017 • edited Loading

infograf768 commented Oct 8, 2017

csthomas commented Oct 8, 2017

infograf768 commented Oct 9, 2017

infograf768 commented Oct 9, 2017

infograf768 commented Oct 9, 2017

csthomas commented Oct 9, 2017

infograf768 commented Oct 9, 2017

csthomas commented Oct 9, 2017

csthomas commented Oct 9, 2017 • edited Loading

infograf768 commented Oct 10, 2017

infograf768 commented Oct 10, 2017

csthomas commented Oct 10, 2017 • edited Loading

csthomas commented Oct 10, 2017 • edited Loading

csthomas commented Oct 10, 2017

infograf768 commented Oct 12, 2017

ghost commented Oct 12, 2017

esedic commented Oct 12, 2017 • edited Loading

ghost commented Oct 12, 2017

csthomas commented Oct 6, 2017 •

edited by joomla-cms-bot

Loading

csthomas commented Oct 8, 2017 •

edited

Loading

csthomas commented Oct 9, 2017 •

edited

Loading

csthomas commented Oct 10, 2017 •

edited

Loading

csthomas commented Oct 10, 2017 •

edited

Loading

esedic commented Oct 12, 2017 •

edited

Loading