The part that's funny is that chx went to extra special lengths to make sure the menu system could support this, and then it was never fully implemented. It does use %menu_tail, but its input is then ignored.

This patch implements menu_tail_load which never got implemented (there's a menu_tail_to_arg() which I don't think really does anything. Maybe chx can weigh in there).

search_get_keys() was full of cruft and somewhat painful to follow logic. It is now a total breeze. In fact, it's not clear to me that it even needs to be a separate function but it is nice to have it separated and out of the way.

CommentFileSizeAuthor
#6 search-hardcodes-key-position.patch3.94 KBmerlinofchaos
search-hardcodes-key-position.patch3.51 KBmerlinofchaos

Comments

dave reid’s picture

dave reid
he/him
Primary language English
Location Nebraska USA
commented

I had a similar patch, but this is a much better approach. Please also replace path_admin_filter_get_keys() as well.

dave reid’s picture

dave reid
he/him
Primary language English
Location Nebraska USA
commented

Tempted to mark this as a duplicate of #298561: menu_tail should automatically act as a load function as well to allow slashes in search. Also see #93854: Allow autocompletion requests to include slashes.

merlinofchaos’s picture

merlinofchaos commented

Hmm. chx's patch in #298561: menu_tail should automatically act as a load function as well to allow slashes in search hardcodes menu_tail. Though I guess by doing that you at least don't need the load function. I'm not sure I like that, though.

dries’s picture

dries commented 
+++ includes/menu.inc	9 Oct 2009 19:02:45 -0000
@@ -699,6 +699,13 @@ function menu_tail_to_arg($arg, $map, $i
+ * Return the entirety of the tail as an argument.
+ */
+function menu_tail_load($arg, $map, $index) {
+  return implode('/', array_slice($map, $index));
+}

Should we mention in the PHPdoc that this is a menu loader function? I think that would be helpful for people looking at this function without context. Otherwise this patch seems good.

merlinofchaos’s picture

merlinofchaos commented

I can't find any calls to path_admin_filter_get_keys() in either D6 or D7. Is this even in use? This may be cruft.

merlinofchaos’s picture

merlinofchaos commented
StatusFileSize
new search-hardcodes-key-position.patch3.94 KB

Updated patch. It includes Dries' desire for more documentation. It also occurred to me that %menu_tail might be implemented incorrectly, with no easy way to see the right way to implement it. In this case I had it at least just return $arg so that it does something rather than crash.

dave reid’s picture

dave reid
he/him
Primary language English
Location Nebraska USA
commented

I really wish that we didn't have to specify the load function in hook_menu() if we want to use this since it seems like bad DX. Is it that bad if we hard-code an exception to this like chx had in #298561: menu_tail should automatically act as a load function as well to allow slashes in search? Otherwise I'd love to RTBC.

merlinofchaos’s picture

merlinofchaos commented
Status: Needs review » Needs work

After playing around with this some more, I found out that it works less well than I had anticipated. For Panels I went back and did some similar stuff to this for D6, and I learned that %menu_tail can't get 'nothing'. So the way that search is working is really weird. When you go to search/node/keyword it goes to search/node/%. but when you go to 'search/node' it goes to 'search'. However, search/node/%menu_tail is still providing the tab, which is what that menu_tail_to_arg function is doing.

Upon further review, %menu_tail itself is simply broken and probably not fixable meaningfully, and you'll get a lot better results by registering 'search/node' and then doing $args = func_get_args(); $keys = implode('/', $args);

I now think %menu_tail should just be removed.

merlinofchaos’s picture

merlinofchaos commented

Also, it is worth nothing that the tabs for search/node and other searches have an extra '/' on the end that you won't find on other tabs. And there is no default tab like you'll find on other tabsets. None of these things matter, particularly, except that they are inconsistent and confusing.

jhodgdon’s picture

jhodgdon
she/her
Primary language English
commented

So... Is this really a bug report for D7, or a feature request for D8 at this point?

jhodgdon’s picture

jhodgdon
she/her
Primary language English
commented
Version: 7.x-dev » 8.x-dev
Category: bug » feature

Setting to D8 for now. Set back if you think that's more appropriate.

jhodgdon’s picture

jhodgdon
she/her
Primary language English
commented
Version: 8.x-dev » 7.x-dev

I think this should be fixed in Drupal 7 actually.

It's related to #497206: Avoid search conflicts with other forms, use menu API instead of search_get_keys(), somewhat, in that that issue is another problem introduced by the way search is not doing the right thing.

So right now, search.module is defining paths for:
search
search/(module_path)/%menu_tail [for each active search module]

All of these paths use the same search_view() function (in search.pages.inc) to build the page, which for some reason checks to see if $_POST['form_id'] is set (which is causing the problem in #497206), and then calls search_get_keywords() (in search.module) to read the keywords from $_GET.

It's all very odd. Probably the suggestion of #8 would be a good fix.

jhodgdon’s picture

jhodgdon
she/her
Primary language English
commented

Another related issue, I think:
#600392: search_form() has an $action argument but this argument is partially ignored

jhodgdon’s picture

jhodgdon
she/her
Primary language English
commented

And yet another that I think is related:
#518512: Search from block with no keywords doesn't display the "Please enter some keywords" message

jhodgdon’s picture

jhodgdon
she/her
Primary language English
commented

Hmmm... I am now recalling that I tried removing the %menu_tail from the menu items, and that caused problems. See
http://drupal.org/node/245103#comment-2662198

jhodgdon’s picture

jhodgdon
she/her
Primary language English
commented
Status: Needs work » Closed (duplicate)

This issue has been incorporated into
#497206: Avoid search conflicts with other forms, use menu API instead of search_get_keys()
which is doing I think everything in these patches, and resolves this issue.