There is a "Hotel California" effect for menu items that have query strings or fragments on the end of the path - you can add them to a menu item's path but once they are there you can never delete them.
To reproduce problem:
- Create new menu item with a path of:
node#fragment?test=test
- Now try to change the path back to
node
--you get the picture, there is no way to get rid of the#fragment?test=test
!
Looking at the code in menu.admin.inc, the reason is obvious. There is a missing else
in the if
construct for processing the path. So if the new path contains a query string or a fragment, then it is saved in the item data structure, but if it does not, then the menu item is left exactly how it was before.
if (isset($parsed_link['query'])) {
$item['options']['query'] = $parsed_link['query'];
}
if (isset($parsed_link['fragment'])) {
$item['options']['fragment'] = $parsed_link['fragment'];
}
This is very easy to fix, simply by adding an else
clause to each if
so that the query or fragment may be deleted:
if (isset($parsed_link['query'])) {
$item['options']['query'] = $parsed_link['query'];
}
else {
unset($item['options']['query']);
}
if (isset($parsed_link['fragment'])) {
$item['options']['fragment'] = $parsed_link['fragment'];
}
else {
unset($item['options']['fragment']);
}
I have attached a patch against HEAD to fix the bug.
Comment | File | Size | Author |
---|---|---|---|
#28 | 682784_menu_item_query.patch | 811 bytes | Island Usurper |
#21 | 682784_1.patch | 2.33 KB | naxoc |
#19 | 682784.patch | 2.4 KB | naxoc |
#16 | menu_edit_bug03.patch | 977 bytes | andrewfn |
#14 | menu_edit_bug02.patch | 992 bytes | andrewfn |
Comments
Comment #1
andrewfn CreditAttribution: andrewfn commentedrequesting review
Comment #3
andrewfn CreditAttribution: andrewfn commentedAccording to other reports, the test failure is due to "general test-bot badness today"
Resubmitting
Comment #5
Anonymous (not verified) CreditAttribution: Anonymous commentedI'm seeing this in Drupal 6 as well. Has this been a long standing issue? I can't seem to find any issues about this for Drupal 6.
We have lots of links with query strings in our primary menu, but there is no way to remove that query string without deleting the menu item and creating a new one. :(
Comment #6
fabsor CreditAttribution: fabsor commentedFix seems fine to me. Nothing much to say about it really =)
Comment #7
andrewfn CreditAttribution: andrewfn commentedYes, the bug is in D6 as well (which is actually where I discovered it), but the system is that it has to be fixed in D7 first, and then the patch can be backported.
Comment #8
catchCould these be set to empty strings rather than unset()?
Also this needs a test.
Comment #9
catchAnd while it's an annoying bug, it isn't critical - you could delete the menu item and recreate it, and nothing gets horribly broken by this except for the iink not changing.
Comment #10
andrewfn CreditAttribution: andrewfn commentedBefore I wrote the patch, I did some research about the relative merits of unset() and setting the string to empty. There is a small performance advantage in using unset() when reading the array because one less de-reference is required. Since the performance of menus is critical to the speed of Drupal, I decided it was worth doing a clean unset().
Comment #11
catchThis is a validation function very far outside the critical path, I think the empty string would be more readable.
Comment #12
andrewfn CreditAttribution: andrewfn commentedMaybe I didn't explain properly. Although the code in this patch will not be run frequently, the data structure it is modifying is one of the key Drupal arrays: the menu, which is accessed very frequently.
The choice is between removing unused entries, or leaving them there as empty strings. Both will work fine, but every time a page is built, the menu array is referenced and so it is best not to clutter it up with null entries.
It is a bit like having a book with pages marked "This page is intentionally left blank". There is no harm done, but it slows down reading a bit.
Comment #13
catchAhh because options is serialized in the db. That makes perfect sense then, but please add a one line comment to explain it's to avoid redundant serialized data being stored since I won't be the only person who forgets that's the case when reading this code.
Comment #14
andrewfn CreditAttribution: andrewfn commentedThanks for your suggestion.
I have added the comment as you requested and re-rolled the patch against head.
It reads:
// use unset() rather than setting to empty string to avoid redundant serialized data being stored
Comment #15
mr.baileysWhile waiting for the testbot to return with the verdict:
Should start with a capital, end with a period and wrap at 80 characters. There also seems to be a double-space in the middle.
Powered by Dreditor.
Comment #16
andrewfn CreditAttribution: andrewfn commentedGood call mr.baileys.
Patch re-rolled with capital at start, period at end and wrapped to be <80 characters.
Comment #17
catchLooks great.
Comment #18
Dries CreditAttribution: Dries commentedCommitted to CVS HEAD (thanks!) but it feels like we need to write a test for this.
Comment #19
naxoc CreditAttribution: naxoc commentedI wrote a test for this. It checks that you can add a menu item with a query and a fragment + that you can in fact get rid of query and fragment again.
I ran into a small issue testing this:
When a menu item is created with a query and/or fragment, the edit form will display for example
node?Array
when you revisit the page. It should displaynode?arg1=value1
. By revisit I mean:1. Create the item and hit save.
2. Click the edit-link of the newly created item.
I put a fix in the patch. Not sure if i should make a seperate isssue, but it is only a one line fix.
Comment #20
catchTest looks fine apart from one code style issue:
This should be on one line otherwise api.drupal.org complains. Maybe "Add and remove a menu link with a query string and fragment."?
Fix is related to the test, so we should probably just fix it here.
Comment #21
naxoc CreditAttribution: naxoc commentedI like that wording better too, so fixed the comment.
Comment #22
catchGreat.
Comment #23
Dries CreditAttribution: Dries commentedGreat thanks a lot naxoc. Committed to CVS HEAD.
Comment #24
catchComment #25
lelutin CreditAttribution: lelutin commentedRemoving tag "Needs tests" since the last patch added a test and the issue is in "fixed" status
Comment #27
Island Usurper CreditAttribution: Island Usurper commentedCan we get this fixed in Drupal 6 as well?
Comment #28
Island Usurper CreditAttribution: Island Usurper commentedIt seems we're not backporting tests, so here is the patch from #16 rerolled for 6.x.
Comment #29
mr.baileysMarked #538548: Menu option editing as duplicate.
Comment #30
mr.baileysReviewed, applied and tested. Same approach as the D7 fix that was committed earlier. RTBC.
Comment #31
Gábor HojtsyThanks, committed.