-
-
Notifications
You must be signed in to change notification settings - Fork 3.8k
[5.4] Tightened regex searching for menu items to prevent duplicate edit link icons #46567 #46569
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
[5.4] Tightened regex searching for menu items to prevent duplicate edit link icons #46567 #46569
Conversation
|
@henrikdalgaard - sadly, I have not been able to reproduce the BEFORE condition - I set a side menu Main Menu - GB with 15+ items some with sub-menu and another Test Menu with itself 15 items and neither show double Pencil Checkbox ;( (I had to go into Global Configuration and update the Frontend Editing field/option to Modules & Menu (you may wish to add that to the Testing Instructions)) ( I wonder what other/additional condition is causing/creating the double icon ? |
|
I have debugged it further. To provoke the bug, I have menu items 31, 32, 33, 34, 35 and then 3 in the same menu. The regex '/(<li.?\bitem-' . $menuItemid . '.?>)/' becomes '/(<li.?\bitem-3.?>)/' which will match Screenshot showing my texteditor matching item 34 with the regex for item 3 With the fixed regex, only menu item 3 is matched when searching for menu item 3. Here is the $moduleHtml before the links are added. Menu-without-editing-links.html and $moduleHtml after the links are added I notice that before adding the links, the entire menu is in a 3000 character wide line. |
|
I have tested this item ✅ successfully on 4c5993d (For other testers, I did have to edit the database to recreate the situation needed (my IDs were in the 130's and I had to update an entry that wasn't a duplicate to 13 to make the condition appear) To make sure that this worked with more complex menu/sub-menu structures, I also created deeper structures than your example: and 3 levels deep Happy Holidays! This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/46569. |
|
@henrikdalgaard There is no need to update the PR to changes in the base branch as long as there are no conflicts shown on GitHub. Now with your branch update you have invalidated the human test count, so I have to manually add back @exlemor 's successful test. This causes additional work for me. |
|
I have tested this item ✅ successfully on 5103a73 Created nested menu items. Problem seen before applying the PR. This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/46569. |
|
RTC This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/46569. |
|
✅ Final test before merge with JBT
|
|
Thank you @henrikdalgaard for your contribution. Thank you @brianteeman, @exlemor and @ThomasFinnern for testing. |


Pull Request for Issue #46566
Summary of Changes
The regex search and replace for menu items finds too many matches as the regex is not limited to one li tag. This results in extra edit links getting added. See screen shots.
An extra space is added to after the inserted edit link as it looks bad if flush against the menu item.
Tabs replaced with spaces in the same line.
Testing Instructions
Enable frontend editing on a page with a menu with lots of items.
Example buggy html output with fixed html
mod-menu1-buggy.html
mod-menu1-fixed.html
Actual result BEFORE applying this Pull Request
Notice the extra edit buttons to the left of two of the menu items.

Expected result AFTER applying this Pull Request
Link to documentations
Please select:
Documentation link for docs.joomla.org:
No documentation changes for docs.joomla.org needed
Pull Request link for manual.joomla.org:
No documentation changes for manual.joomla.org needed