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:

  1. Create new menu item with a path of: node#fragment?test=test
  2. 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.

CommentFileSizeAuthor
#28 682784_menu_item_query.patch811 bytesIsland Usurper
#21 682784_1.patch2.33 KBnaxoc
#19 682784.patch2.4 KBnaxoc
#16 menu_edit_bug03.patch977 bytesandrewfn
#14 menu_edit_bug02.patch992 bytesandrewfn
menu_edit_bug01.patch884 bytesandrewfn
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

andrewfn’s picture

andrewfn CreditAttribution: andrewfn commented
Status: Active » Needs review

requesting review

Status: Needs review » Needs work

The last submitted patch, menu_edit_bug01.patch, failed testing.

andrewfn’s picture

andrewfn CreditAttribution: andrewfn commented

According to other reports, the test failure is due to "general test-bot badness today"
Resubmitting

Status: Needs work » Needs review

Re-test of menu_edit_bug01.patch from comment @comment was requested by andrewfn.

Anonymous’s picture

Anonymous (not verified) CreditAttribution: Anonymous commented

I'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. :(

fabsor’s picture

fabsor CreditAttribution: fabsor commented

Fix seems fine to me. Nothing much to say about it really =)

andrewfn’s picture

andrewfn CreditAttribution: andrewfn commented

Yes, 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.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented

Could these be set to empty strings rather than unset()?

Also this needs a test.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented
Priority: Critical » Normal

And 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.

andrewfn’s picture

andrewfn CreditAttribution: andrewfn commented

Before 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().

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented

This is a validation function very far outside the critical path, I think the empty string would be more readable.

andrewfn’s picture

andrewfn CreditAttribution: andrewfn commented

Maybe 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.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented

Ahh 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.

andrewfn’s picture

andrewfn CreditAttribution: andrewfn commented
FileSize
menu_edit_bug02.patch992 bytes

Thanks 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

mr.baileys’s picture

mr.baileys
Primary language Dutch
Location 🇧🇪 (Ghent)
CreditAttribution: mr.baileys commented

While waiting for the testbot to return with the verdict:

+++ modules/menu/menu.admin.inc	31 Jan 2010 03:30:33 -0000
@@ -348,9 +348,16 @@ function menu_edit_item_validate($form, 
+      // use unset() rather than setting to empty string  to avoid redundant serialized data being stored

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.

andrewfn’s picture

andrewfn CreditAttribution: andrewfn commented
FileSize
menu_edit_bug03.patch977 bytes

Good call mr.baileys.
Patch re-rolled with capital at start, period at end and wrapped to be <80 characters.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented
Status: Needs review » Reviewed & tested by the community

Looks great.

Dries’s picture

Dries CreditAttribution: Dries commented
Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs tests

Committed to CVS HEAD (thanks!) but it feels like we need to write a test for this.

naxoc’s picture

naxoc CreditAttribution: naxoc commented
Status: Needs work » Needs review
FileSize
682784.patch2.4 KB

I 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 display node?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.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented

Test looks fine apart from one code style issue:

+   * Tests that one can add a menu item with a query string and a fragment.
+   * And tests that the query and fragment can be removed again.

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.

naxoc’s picture

naxoc CreditAttribution: naxoc commented
FileSize
682784_1.patch2.33 KB

I like that wording better too, so fixed the comment.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented
Status: Needs review » Reviewed & tested by the community

Great.

Dries’s picture

Dries CreditAttribution: Dries commented

Great thanks a lot naxoc. Committed to CVS HEAD.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented
Status: Reviewed & tested by the community » Fixed
lelutin’s picture

lelutin CreditAttribution: lelutin commented
Issue tags: -Needs tests

Removing tag "Needs tests" since the last patch added a test and the issue is in "fixed" status

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.

Island Usurper’s picture

Island Usurper CreditAttribution: Island Usurper commented
Version: 7.x-dev » 6.x-dev
Status: Closed (fixed) » Patch (to be ported)

Can we get this fixed in Drupal 6 as well?

Island Usurper’s picture

Island Usurper CreditAttribution: Island Usurper commented
Status: Patch (to be ported) » Needs review
FileSize
682784_menu_item_query.patch811 bytes

It seems we're not backporting tests, so here is the patch from #16 rerolled for 6.x.

mr.baileys’s picture

mr.baileys
Primary language Dutch
Location 🇧🇪 (Ghent)
CreditAttribution: mr.baileys commented

Marked #538548: Menu option editing as duplicate.

mr.baileys’s picture

mr.baileys
Primary language Dutch
Location 🇧🇪 (Ghent)
CreditAttribution: mr.baileys commented
Status: Needs review » Reviewed & tested by the community

Reviewed, applied and tested. Same approach as the D7 fix that was committed earlier. RTBC.

Gábor Hojtsy’s picture

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy commented
Status: Reviewed & tested by the community » Fixed

Thanks, committed.

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.