Fix com_content legacy router trying to use menu items of other components by ggppdk · Pull Request #17271 · joomla/joomla-cms

ggppdk · 2017-07-26T12:04:58Z

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:

Enable error reporting to maximum
2 .Unpublish ALL com_content menu items
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,

Display a Joomla latest articles module in your home page (make sure you have some items in it)
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

…nents

ggppdk · 2017-07-26T12:08:14Z

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

csthomas · 2017-10-07T18:36:15Z

components/com_content/helpers/legacyrouter.php

-		// Check again
-		if ($menuItemGiven && isset($menuItem) && $menuItem->component != 'com_content')
+		// Make sure that the menu item selected above actually points to com_content component
+		if (!empty($menuItem) && $menuItem->component != 'com_content')


Can you change above line to if ($menuItem !== null && $menuItem->component != 'com_content')?

csthomas · 2017-10-07T19:15:27Z

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

csthomas · 2017-10-11T07:21:46Z

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?

ghost · 2017-10-26T11:42:11Z

@Quy Code review?

infograf768 · 2017-11-16T07:35:26Z

@ggppdk
Is https://forum.joomla.org/viewtopic.php?f=710&t=955997&p=3502464#p3498744 related?

csthomas · 2017-11-16T07:40:15Z

Is https://forum.joomla.org/viewtopic.php?f=710&t=955997&p=3502464#p3498744 related?

@infograf768 It was fixed in #18564

ggppdk · 2017-11-16T07:54:28Z

@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

ggppdk · 2017-11-16T07:56:46Z

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

ggppdk · 2017-11-16T08:05:11Z

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

csthomas · 2017-11-16T10:04:48Z

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.

ggppdk · 2018-03-27T19:57:39Z

#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

Chacapamac · 2018-05-26T16:53:41Z

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

Quy · 2018-05-26T18:13:11Z

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

Chacapamac · 2018-05-26T19:00:05Z

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

#18271 or #18757 from ggppdk?

Quy · 2018-05-26T19:03:58Z

PR #17271 that you are on.

Chacapamac · 2018-05-26T20:56:27Z

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

Quy · 2018-05-28T16:32:52Z

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

ggppdk · 2018-07-16T05:35:34Z

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

Fix com_content legacy router trying to use menu items of other compo…

d4a077d

…nents

joomla-cms-bot added the PR-staging label Jul 26, 2017

csthomas reviewed Oct 7, 2017

View reviewed changes

Make check of menuItem found / loaded more strict

6f792c4

ggppdk mentioned this pull request Mar 27, 2018

Add additional check that view exists before proceeding #18757

Merged

infograf768 referenced this pull request May 26, 2018

Revert my fix and use one by ggppdk

64f5ac9

ggppdk closed this Jul 16, 2018

Uh oh!

Conversation

ggppdk commented Jul 26, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary of Changes

Testing Instructions

Expected result

Actual result

Documentation Changes Required

Uh oh!

ggppdk commented Jul 26, 2017

Uh oh!

csthomas Oct 7, 2017

Choose a reason for hiding this comment

Uh oh!

csthomas commented Oct 7, 2017

Uh oh!

csthomas commented Oct 11, 2017

Uh oh!

ghost commented Oct 26, 2017

Uh oh!

infograf768 commented Nov 16, 2017

Uh oh!

csthomas commented Nov 16, 2017

Uh oh!

ggppdk commented Nov 16, 2017

Uh oh!

ggppdk commented Nov 16, 2017

Uh oh!

ggppdk commented Nov 16, 2017

Uh oh!

csthomas commented Nov 16, 2017

Uh oh!

ggppdk commented Mar 27, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Chacapamac commented May 26, 2018

Uh oh!

Quy commented May 26, 2018

Uh oh!

Chacapamac commented May 26, 2018

Uh oh!

Quy commented May 26, 2018

Uh oh!

Chacapamac commented May 26, 2018

Uh oh!

Quy commented May 28, 2018

Uh oh!

ggppdk commented Jul 16, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

ggppdk commented Jul 26, 2017 •

edited

Loading

ggppdk commented Mar 27, 2018 •

edited

Loading