Skip to content

Fix PHP warning if an old type menus exist#17323

Closed
chivitli wants to merge 3 commits intojoomla:stagingfrom
chivitli:patch-15
Closed

Fix PHP warning if an old type menus exist#17323
chivitli wants to merge 3 commits intojoomla:stagingfrom
chivitli:patch-15

Conversation

@chivitli
Copy link
Copy Markdown
Contributor

@chivitli chivitli commented Jul 28, 2017

I just noticed that error log on one of my sites quickly grew to over 100 MB. It was always the same error:

Trying to get property of non-object in /*******/public_html/libraries/cms/component/router/rules/menu.php on line 179.

I was not the only one with this problem, here is a related post on the forum: https://forum.joomla.org/viewtopic.php?t=949743

It would happen for example if when migrating from an old version you had a menu of type "Content Section", and it wouldn't matter that the menu isn't displayed anywhere on the new version. It may happen under some other circumstances as well. The change I made should keep the condition of the if statement unchanged, except that it won't throw an error if the variable is uninitialized.

Pull Request for Issue # .

Summary of Changes

Testing Instructions

Expected result

Actual result

Documentation Changes Required

I just noticed that error log on one of my sites quickly grew to over 100 MB. It was always the same error:

_Trying to get property of non-object in /*******/public_html/libraries/cms/component/router/rules/menu.php on line 179._

I was not the only one with this problem, here is a related post on the forum: https://forum.joomla.org/viewtopic.php?t=949743

It would happen for example if when migrating from an old version you had a menu of type "Content Section", and it wouldn't matter that the menu isn't displayed anywhere on the new version. It may happen under some other circumstances as well. The change I made should keep the condition of the if statement unchanged, except that it won't throw an error if the variable is uninitialized.
@ghost
Copy link
Copy Markdown

ghost commented Nov 1, 2017

Status is set on "Needs Review".

@csthomas
Copy link
Copy Markdown
Contributor

csthomas commented Nov 2, 2017

IMO the fix should look like

diff --git a/libraries/src/Component/Router/Rules/MenuRules.php b/libraries/src/Component/Router/Rules/MenuRules.php
index 34bd9b1e58..973d01095c 100644
--- a/libraries/src/Component/Router/Rules/MenuRules.php
+++ b/libraries/src/Component/Router/Rules/MenuRules.php
@@ -202,7 +202,7 @@ class MenuRules implements RulesInterface
 
                        foreach ($items as $item)
                        {
-                               if (isset($item->query) && isset($item->query['view']))
+                               if (isset($item->query['view'], $views[$item->query['view']]))
                                {
                                        $view = $item->query['view'];

because such view does not exist and can not be added to menu lookup at all.

@johannesveje
Copy link
Copy Markdown

Any updates on how long it will be till this 8 chars, 3½ month old fix will be implemented ? I hope the time isn't proportional to the size of the commit for other problems.

@brianteeman
Copy link
Copy Markdown
Contributor

Every pull request needs at least two human tests to confirm the issue and fix

@johannesveje
Copy link
Copy Markdown

johannesveje commented Nov 13, 2017

So right now all that is missing is one human confirming the fix? The issue have been reported multiple times: #18123 and #18043 . The last one also have a confirmed fix, which might even work better than this one.

@csthomas
Copy link
Copy Markdown
Contributor

I have created another solution for the same problem. Welcome to test #18564.
You can test it in your own way.

If no PR will be tested by two testers, none of them will be merged.

@johannesveje
Copy link
Copy Markdown

Yay one more solution \o/

@csthomas
Copy link
Copy Markdown
Contributor

@johannesveje If you want any changes in joomla you should test some of them at https://issues.joomla.org/tracker/joomla-cms/

@johannesveje
Copy link
Copy Markdown

I'll create a solution myself when I get the time 😊

@Quy
Copy link
Copy Markdown
Contributor

Quy commented Nov 15, 2017

@csthomas Can this be closed?

@csthomas
Copy link
Copy Markdown
Contributor

I think so or after #18564 will be merged.

@joomla-cms-bot
Copy link
Copy Markdown

Set to "closed" on behalf of @franz-wohlkoenig by The JTracker Application at issues.joomla.org/joomla-cms/17323

@ghost
Copy link
Copy Markdown

ghost commented Nov 15, 2017

closed as stated above.


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

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.

6 participants