Joomla! Issue Tracker - CMS

Download
Launch
Feeling Lucky

[#17271] - Fix com_content legacy router trying to use menu items of other components

? Pending

User tests: Successful: Unsuccessful:

avatar ggppdk
ggppdk
26 Jul 2017

Pull Request for Issues #15823

com_content router will only check if menu item is of 'com_content' type only if "menuItemGiven" flag is set, but this is not enough

The result is that current menu item can be tried without caring if it is com_content menu item

Summary of Changes

Always check if selected menu item is

Testing Instructions

Code review

Also an example for testing is shown here:

  1. Enable error reporting to maximum
    2 .Unpublish ALL com_content menu items
  2. Create a menu item M1 of some other component but
  • but select a menu item that DOES NOT have ID variable,
  • but it has a view name either 'article' or 'category' so that you get the undefined variable notice,
  1. Display a Joomla latest articles module in your home page (make sure you have some items in it)
  2. Enable SEF urls and view the menu item M1 in frontend

Result is shown here
https://cloud.githubusercontent.com/assets/5092940/17293281/0f36a64a-57f8-11e6-8ae2-3012187cb7ea.png

Expected result

The current menu item since it belongs to other component should not be examined by the com_content router

Actual result

The com_content router tries to use the menu item

Documentation Changes Required

avatar joomla-cms-bot joomla-cms-bot - change - 26 Jul 2017
Category ⇒ Front End com_content
avatar ggppdk ggppdk - open - 26 Jul 2017
avatar ggppdk ggppdk - change - 26 Jul 2017
Status New ⇒ Pending
avatar ggppdk
ggppdk - comment - 26 Jul 2017

Please note that the picture is old (it shows same notice in different PHP file), because this is the 3rd PR to made, the other had conflicts after the legacy router being moved, and we closed them

avatar ggppdk ggppdk - change - 7 Oct 2017
The description was changed
avatar ggppdk ggppdk - edited - 7 Oct 2017
avatar ggppdk ggppdk - change - 7 Oct 2017
Labels Added: ?
avatar csthomas csthomas - test_item - 7 Oct 2017 - Tested successfully
avatar csthomas
csthomas - comment - 7 Oct 2017

I have tested this item ✅ successfully on 6f792c4

Itemid from Active Menu Item also should be removed from query when it is not from com_content.

This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/17271.

avatar csthomas
csthomas - comment - 11 Oct 2017

This need one more test.
After #18271 has been merged, you won't see PHP Notice any more.
I suggest code review.

@Quy Could you test this?

avatar franz-wohlkoenig
franz-wohlkoenig - comment - 26 Oct 2017

@Quy Code review?

avatar ggppdk
ggppdk - comment - 16 Nov 2017

@infograf768

This PR is not related to the particular case that you linked

it seems to me that @csthomas PR mentioned above is fixing that particular issue mentioned in the forum

avatar ggppdk
ggppdk - comment - 16 Nov 2017

But if this PR is merged

maybe the issue in the forum will be fixed too

  • because it can be that the menu item being tried is non-com_content menu item
avatar ggppdk
ggppdk - comment - 16 Nov 2017

Thinking of it more,
#18564 fixes the warnings of
https://forum.joomla.org/viewtopic.php?f=710&t=955997&p=3502464#p3498744

but the original reason of it
must be that a non-com_content menu is being examined

avatar csthomas
csthomas - comment - 16 Nov 2017

Because of

joomla-cms/libraries/src/Component/Router/Rules/MenuRules.php

Lines 167 to 173 in 54eb5d8

// If not found, return language specific home link
$default = $this->router->menu->getDefault($language);
if (!empty($default->id))
{
$query['Itemid'] = $default->id;
}

for now $query['Itemid'] should be always set up.

But if we comment above lines and create a new component with article view, then URL routing for com_content article view will generate a wrong link, if we are in page with active menu item for the new component on article view.

It is illogical to unset $query['Itemid'] at line 80, only if it was set outside ($menuItemGiven=true) but do not unset $query['Itemid'] if it comes from active menu item.

joomla-cms/components/com_content/helpers/legacyrouter.php

Lines 65 to 81 in 54eb5d8

if (empty($query['Itemid']))
{
$menuItem = $this->router->menu->getActive();
$menuItemGiven = false;
}
else
{
$menuItem = $this->router->menu->getItem($query['Itemid']);
$menuItemGiven = true;
}
// Check again
if ($menuItemGiven && isset($menuItem) && $menuItem->component != 'com_content')
{
$menuItemGiven = false;
unset($query['Itemid']);
}

BTW. It would be nice to replace all occurrences of $menuItemGiven by $menuItem !== null.

avatar ggppdk
ggppdk - comment - 27 Mar 2018

#18271

The PR above removed the notice when comparing to menu items
that have the same name for the view but they do not have an id
which is a positive thing

and then there is this PR

#18757

to clean the notice of non set view variable in the menu item when comparing
a com_content menu item
with
a menu item which does not specify a view,
e.g. a menu item of type 'Alias'

  • If the routing code of com_conent router,

would just not try to match a menu item that is of a different component

then all these notices would not need to be cleaned (assuming you then make comparison in correct order, aka first the view and then the id)

so Instead of addressing a route issue we suppress the notices so that we can compare with menu items that are clearly not a match because are of wrong component

and if you ever succeed to make a match (since we ignored that they are menu items of different component) then the URL when clicked will be broken

avatar Chacapamac
Chacapamac - comment - 26 May 2018

I’m not to sure how to participate but I was invited to do it here.
With Joomla 3.8.8, I experienced an error Undefined index: view in —> components/com_content/helpers/legacyrouter.php on line 95 when going from my front page to a sub menu with an article. This sub menu is under a top menu item type: Separator.

I test a fix from “impressionestudio” on #15823 that work perfectly by inserting this line of code && isset($menuItem->query['view']) here if ($menuItem !== null && isset($menuItem->query['view']) && $menuItem->query['view'] == $query['view'] in the legacyrouter.php .

No more errors. I was wondering if this is the right fix ? — Sorry, I’m learning how Github work, be patient with me...

avatar Quy
Quy - comment - 26 May 2018

@Chacapamac please test this pull request to see if it will fix your issue.

avatar Chacapamac
Chacapamac - comment - 26 May 2018

From Quy > @Chacapamac please test this pull request to see if it will fix your issue.

#18271 or #18757 from ggppdk?

avatar Quy
Quy - comment - 26 May 2018

PR #17271 that you are on.

avatar Chacapamac
Chacapamac - comment - 26 May 2018

I guess I go in “Files Changed”
As indicated, I replace the line 77 with:
if ($menuItemGiven && isset($menuItem) && $menuItem->component != 'com_content') // Make sure that the menu item selected above actually points to com_content component if ($menuItem !== null && $menuItem->component != 'com_content')

Passing from front page to inside page give me again the Error:
Undefined index: view in —> components/com_content/helpers/legacyrouter.php on line 95

Again, adding && isset($menuItem->query['view']) after line 94, fix the error

avatar Quy
Quy - comment - 28 May 2018

@Chacapamac Please test #18757 and mark your test result at the issue traker: https://issues.joomla.org/tracker/joomla-cms/18757

avatar ggppdk
ggppdk - comment - 16 Jul 2018

If i remember correctly, (without retesting) i think it was resolved elsewhere, closing

avatar ggppdk ggppdk - change - 16 Jul 2018
Status Pending ⇒ Closed
Closed_Date 0000-00-00 00:00:00 ⇒ 2018-07-16 05:35:38
Closed_By ⇒ ggppdk
avatar ggppdk ggppdk - close - 16 Jul 2018

Add a Comment

Login with GitHub to post a comment