Incorrect logic in form and validate functions for adding menu links via UI
pwolanin - November 3, 2009 - 18:13
| Project: | Drupal |
| Version: | 7.x-dev |
| Component: | menu.module |
| Category: | bug report |
| Priority: | critical |
| Assigned: | Unassigned |
| Status: | needs work |
Description
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)
#1
This PHP function may be useful for breaking the query string up http://www.php.net/manual/en/function.parse-str.php
#2
Here's a start. We now have drupal_parse_url to use.
#3
Patch looks good, but it seems we need tests here. :-/
#4
Patch is not complete in terms of the problems, it's just a start.
#5
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':
<?phpif (isset($item['options']['query'])) {
$path .= '?' . $item['options']['query'];
}
?>
#6
@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).
#7
How about this...
#8
+++ 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?
#9
Edited... Wrong patch.
#10
Here's an update to use drupal_http_build_query()...
Needs works b/c there are still no tests.