Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix for issue #15498 #15499

Merged
merged 1 commit into from Apr 24, 2017
Merged

Fix for issue #15498 #15499

merged 1 commit into from Apr 24, 2017

Conversation

wilsonge
Copy link
Contributor

@wilsonge wilsonge commented Apr 24, 2017

Pull Request for Issue #15498 .

Summary of Changes

It's bad practice to typehint against a stdClass instance. Instead it's better to check we don't get null back from JMenu::getActive or JMenu::getItem

Testing Instructions

Install staging with sample testing data. Go to the featured articles page. Before patch the home page has a url like JROOT/index.php/19-sample-data-articles/joomla/24-joomla. After the patch it should be JROOT/index.php

Documentation Changes Required

None

@@ -91,7 +91,7 @@ public function build(&$query, &$segments)
}

// Are we dealing with an article or category that is attached to a menu item?
if (($menuItem instanceof stdClass)
if ($menuItem !== null
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add:
&& isset($query['view']) below changed line?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No - that's not related to this bug - it needs to go in a separate Pull Request

@infograf768
Copy link
Member

I have tested this item ✅ successfully on 87ba5af


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

2 similar comments
@Kubik-Rubik
Copy link
Member

I have tested this item ✅ successfully on 87ba5af


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

@dgrammatiko
Copy link
Contributor

I have tested this item ✅ successfully on 87ba5af


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

@wilsonge
Copy link
Contributor Author

RTC.


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

@joomla-cms-bot joomla-cms-bot added the RTC This Pull Request is Ready To Commit label Apr 24, 2017
@wilsonge wilsonge added this to the Joomla 3.7.0 milestone Apr 24, 2017
@rdeutz rdeutz merged commit f7eb141 into joomla:staging Apr 24, 2017
@joomla-cms-bot joomla-cms-bot added PR-staging and removed RTC This Pull Request is Ready To Commit labels Apr 24, 2017
@wilsonge wilsonge deleted the fix-content-router branch April 24, 2017 10:24
rdeutz pushed a commit to joomlajenkins/joomla-cms that referenced this pull request May 1, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants