This logic is rather broken:

http://api.drupal.org/api/function/menu_edit_item_validate/7

why do we look for an alias before testing if the link is external? And if it is external, we should still split of the query and fragment to they are handled properly. Also, we are trying to get all querystrings to be stored as arrays not strings - and right now they may be wither, so the above function is possibly wrong for not converting it to an array, and this function is broken:

since it will not handle a link with a query string that is an array (which we now expect)

http://api.drupal.org/api/function/menu_edit_item/7

CommentFileSizeAuthor
#10 menu_item_val.patch2.43 KBbcn
#9 menu_admin.patch2.16 KBbcn
#7 menu_item_val.patch2.47 KBbcn
#2 menu_admin.patch2.16 KBbcn

Comments

pwolanin’s picture

pwolanin commented

This PHP function may be useful for breaking the query string up http://www.php.net/manual/en/function.parse-str.php

bcn’s picture

bcn commented
Status: Active » Needs review
StatusFileSize
new menu_admin.patch2.16 KB

Here's a start. We now have drupal_parse_url to use.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented

Patch looks good, but it seems we need tests here. :-/

pwolanin’s picture

pwolanin commented

Patch is not complete in terms of the problems, it's just a start.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented

1) Why should we store 'query' and 'fragment' in 'options' for external links? External links are external links. url() handles them properly, and in an edge-case scenario, a hook_menu_link_alter() implementation may want to store a query or fragment to extend/override the values in the external link (handled by url()). For external links, we're missing only the else condition, which should set ['options']['external'] => TRUE;

2) Hence, we only need to parse the link in case it's not external. When it's not external, then we need 'query' and 'fragment' splitted up. drupal_parse_url() already does this.

3) Only for internal links we want to search for a system path.

4) menu_edit_item() needs to be updated to implode() 'query':

  if (isset($item['options']['query'])) {
    $path .= '?' . $item['options']['query'];
  }
pwolanin’s picture

pwolanin commented

@sun - if we set options['external'] here url() will not check for bad protocols

Also, your arguments about altering an extrernal link suggest exactly that we need to store the query and fragment in the options array - however, I'm willing to leave that part of the flow as-is (i.e. don't parse external links).

bcn’s picture

bcn commented
StatusFileSize
new menu_item_val.patch2.47 KB

How about this...

sun’s picture

sun
Primary language German
Location Karlsruhe
commented 
+++ modules/menu/menu.admin.inc	7 Nov 2009 07:44:00 -0000
@@ -266,7 +266,7 @@
   if (isset($item['options']['query'])) {
-    $path .= '?' . $item['options']['query'];
+    $path .= '?' . http_build_query($item['options']['query']);
   }

This should use drupal_http_build_query().

I'm on crack. Are you, too?

bcn’s picture

bcn commented
Status: Needs review » Needs work
StatusFileSize
new menu_admin.patch2.16 KB

Edited... Wrong patch.

bcn’s picture

bcn commented
StatusFileSize
new menu_item_val.patch2.43 KB

Here's an update to use drupal_http_build_query()...

Needs works b/c there are still no tests.

sun.core’s picture

sun.core commented
Priority: Critical » Normal

@pwolanin: Any update on this? Not sure what you meant earlier by "incomplete"...

This doesn't seem to touch any critical functionality.

Status: Needs work » Closed (outdated)

Automatically closed because Drupal 7 security and bugfix support has ended as of 5 January 2025. If the issue verifiably applies to later versions, please reopen with details and update the version.