Skip to content

Undefined index with non-component menu item#25796

Merged
HLeithner merged 4 commits intojoomla:stagingfrom
SharkyKZ:j3/undefinedIndex
Aug 23, 2019
Merged

Undefined index with non-component menu item#25796
HLeithner merged 4 commits intojoomla:stagingfrom
SharkyKZ:j3/undefinedIndex

Conversation

@SharkyKZ
Copy link
Contributor

@SharkyKZ SharkyKZ commented Aug 8, 2019

Pull Request for Issue #25029.

Summary of Changes

Fixes undefined index notice when accessing certain pages with non-component menu item ID.

Testing Instructions

Create External URL or other non-component menu item.
Remember its ID.
Enter, article, article category, newsfeeds, newsfeed category or tags page via non-SEF URL, e.g. index.php?option=com_content&view=article&id=1.
Append menu item ID of external URL menu item, e.g. index.php?option=com_content&view=article&id=1&Itemid=488.

Expected result

No notices.

Actual result

Notice: Undefined index: option in components\com_content\views\article\view.html.php on line 245

Documentation Changes Required

No.

@ghost
Copy link

ghost commented Aug 9, 2019

I have tested this item ✅ successfully on 1c52418


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

@richard67
Copy link
Member

@dioubernardo Could you test if this solves your issue #25029 ? Just apply the changes made with this PR and see if it helps. You should do this not on your production site but on a copy (files and db) you can use for testing. When ok, please mark your test result in the issue tracker here https://issues.joomla.org/tracker/joomla-cms/25796 by using the "Test this" button, then selecting the result and submitt. It would be good for us to know if this PR here solves your specific issue.

@richard67
Copy link
Member

@SharkyKZ I started with testing, com_content article and category blog are ok. But I just ask myself why should anyone append a completely unrelated menu item id to the non-sef url of anything?

@ghost
Copy link

ghost commented Aug 9, 2019

@richard67 its enough to test one of the Menutypes as @SharkyKZ wrote "or".

@richard67
Copy link
Member

@franz-wohlkoenig No. He changed code for different components, and (theoretically) with every change could happen a typo, so as a good JBS member I should test all changes 👅

@ghost
Copy link

ghost commented Aug 9, 2019

I have tested this item 🔴 unsuccessfully on 1c52418

Had only tested as written in Testing Instructions.


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

@richard67
Copy link
Member

@franz-wohlkoenig And he forgot to mention com_contact in his list.

@richard67
Copy link
Member

@franz-wohlkoenig Why unsuccessfully? You found a mistake? If no, please change to "not tested". If yes, please describe the mistake.

@ghost
Copy link

ghost commented Aug 9, 2019

I have not tested this item.


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

@richard67
Copy link
Member

I have tested this item ✅ successfully on 1c52418

Tested contact, content, newsfeeds components (each the single item and the category views) and tags view of tags component.

I still don't understand what it is good for to append an unrelated menu item ID, and the breadcrumps having the e.g. URL menu item at beginning of the path seems strange to me, but better that than a PHP notice shown at frontend.


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

@richard67
Copy link
Member

Hint for testers: Easiest way is to use sample testing data, it countains already all menu item and content types needed.

Copy the links from backend menu item edit view of the particular menu item (contact categroy, single contact, content category, single article, news feeds, ... ) to a frontend page. You can use several broswer tabs, one for each of menu item.

Then append to every link the "&Itemid=xxx". I used 448 for xxx, that is the menu item id of the URL menu item type for the link to the admin page on a new clean installation of staging + testing sample data.

Goto the modified URL and you see the PHP notice.

Now apply the patch e.g. with patchtester and reload every browser tab.

You will see no notice and the right view for the particular menu item (contact categroy, single contact, ... ).

@Quy
Copy link
Contributor

Quy commented Aug 9, 2019

why should anyone append a completely unrelated menu item id to the non-sef url of anything?

#22973 (comment)

@richard67
Copy link
Member

I see. Now we just need 1 more tester. Seems to be hard to get some in those days.

@alikon
Copy link
Contributor

alikon commented Aug 11, 2019

I have tested this item ✅ successfully on 1c52418


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

@ghost
Copy link

ghost commented Aug 11, 2019

Status "Ready To Commit".

@joomla-cms-bot joomla-cms-bot added the RTC This Pull Request is Ready To Commit label Aug 11, 2019
@HLeithner
Copy link
Member

Thank you for killing 15 bugs!

@HLeithner HLeithner merged commit a74cf2d into joomla:staging Aug 23, 2019
@joomla-cms-bot joomla-cms-bot removed the RTC This Pull Request is Ready To Commit label Aug 23, 2019
@HLeithner HLeithner added this to the Joomla! 3.9.12 milestone Aug 23, 2019
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.

7 participants