Introduce "Tail arguments" in menu paths [#157510]

We currently have the % wild card, which acts as explained in the following example:

hook_menu path: search/node/%
Sample path 1: search/node/some+keywords
Here: % is equals to some keywords.
Sample path 2: search/node/some/keywords
Here: % is equals to some and not some/keywords. Obviously because % wild card is meant to provide the argument within the slashes

So in this case, the tabs on search page link to say: search/user/some instead of search/user/some/keywords.

So, the proposal is to introduce a tail wild card. Say a symbol like $. It can be only used at the end of the path, to return the whole proceeding url starting that particular wild card position. Consider the following example:

hook_menu path: search/node/$
Sample path 1: search/node/some+keywords
Here: $ is equals to some keywords.
Sample path 2: search/node/some/keywords
Here: $ is equals to some/keywords.
Which is the desired result, and the tabs link to correct paths as well like search/user/some/keywords.

This feature could be used in path module's new upcoming interface at http://drupal.org/node/147143

I know we are in code freeze, but this is a necessity(unless someone has a way for D6 to have correct tab links and correct arguments passed to the forms). Also encoded strings can be used, but won't vote for it, it won't be much readable. chx had earlier pointed that this could be hard to implement but not impossible.. It's upto the menu system developers.

Comment	File	Size	Author
#19	menu_tail_1.patch	2.36 KB	chx
#19
#18	s2_0.png	9.04 KB	chx
#17	s1.png	9.17 KB	chx
#12	menu_tail_0.patch	2.69 KB	chx
#12
#9	menu_tail_2.patch	2.69 KB	pwolanin
#9
#2	menu_tail.patch	2.45 KB	chx
#2

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Comment #1

Gurpartap Singh CreditAttribution: Gurpartap Singh commented 7 July 2007 at 12:19

Category:

feature

» bug

The tabs are broken on search pages in the case explained above. Marking as bug report.

Comment #2

chx CreditAttribution: chx commented 7 July 2007 at 22:50

Status:

Active

» Needs review

File	Size
menu_tail.patch	2.45 KB

I have introduced tail arguments and fixed a search warning. Note that there is another issue with search tabs, content always points to 'search' but that's a separate issue.

Comment #3

chx CreditAttribution: chx commented 7 July 2007 at 22:52

Note: a PHP function can always take less arguments than called with so your to_arg functions needs not to be changed after this patch.

Comment #4

eaton CreditAttribution: eaton commented 7 July 2007 at 23:45

It took me a bit to figure out what the underlying issue is, but the key is paths where trailing parameters may contain slashes.

The example of search tabs is instructive.

1) You use the search box in the sidebar to search for "foo/bar"
2) That gets turned into "search/node/foo/bar
3) The search module automatically generates tabs on that page to search users, etc for the same terms
4) Because "/" appears in the search term, the menu system splits it incorrectly
5) The resulting user search points to "search/user/foo" and the "/bar" portion of the term is lost.

This patch solves it by introducing the '$' wildcard character for menu path definitions. It means, "Treat everything after this point as a single argument, regardless of slashes."

It's a specific set of steps, but other modules that need dynamic paths based on user input, or pagets that use GET arguments form a form, are likely to encounter it.

I'm not familiar enough with the system to know whether this is common enough to make the solution part of core, but I thought iving a more detailed explanation for those not already immersed in the menu system woudl help.

Comment #5

chx CreditAttribution: chx commented 7 July 2007 at 23:52

This patch solves it by introducing the menu_tail_to_arg function. No separate wildcard.

Comment #6

eaton CreditAttribution: eaton commented 7 July 2007 at 23:53

Whoops. My mistake. Thanks for correcting, chx.

Comment #7

pwolanin CreditAttribution: pwolanin commented 8 July 2007 at 00:31

Status:

Needs review

» Needs work

this isn't quite the right fix for search module search_help call - you can take that out since there's a patch I made earlier: http://drupal.org/node/157577

I think the patch needs to be more general. For some reason, tabs go away all together on a path like node/2/xxx

Comment #8

pwolanin CreditAttribution: pwolanin commented 8 July 2007 at 00:53

Ahh, this is a problem specific to node/% since comment module declares a node/%/% path.

Comment #9

pwolanin CreditAttribution: pwolanin commented 8 July 2007 at 01:17

File	Size
menu_tail_2.patch	2.69 KB

This patch leave the search_help fix to the other patch.

Also, a partial fix for the 'Content' tab - the reason it doesn't get the right path is that it's a default local task to /search, and our previous work makes it so that the default task tab must point to the root path. However, this fix isn't totally right either.

Comment #10

Gurpartap Singh CreditAttribution: Gurpartap Singh commented 8 July 2007 at 07:56

Status:

Needs work

» Needs review

So in a way, %menu_tail is the new wild card(or the way to fetch anything after that arg). Right?

Marking code needs review, which hopefully is done as it's being published by both chx and pwolanin :)

Comment #11

Gurpartap Singh CreditAttribution: Gurpartap Singh commented 8 July 2007 at 07:59

The comment changes in patch:

  * @param $to_arg_functions
- *   An array of helper function (ex: array(1 => 'node_load'))
+ *   An array of helper function (ex: array(2 => 'menu_last_to_arg'))
  */
function _menu_link_map_translate(&$map, $to_arg_functions) {

Was the "menu_last_to_arg" meant to be "menu_tail_to_arg"?

Comment #12

chx CreditAttribution: chx commented 8 July 2007 at 15:06

Status:

Needs review

» Reviewed & tested by the community

File	Size
menu_tail_0.patch	2.69 KB

If all the problems pwolanin could found was removing feature creep then it's as good as it gets and we will fix the content tab in a separate issue.

Comment #13

Gábor Hojtsy

he/him

Hungarian

Hungary

CreditAttribution: Gábor Hojtsy commented 11 July 2007 at 20:18

Status:

Reviewed & tested by the community

» Needs review

Let's discuss this a bit. If the search paths are *the* use case, then IMHO we are better off with encoding the slash. We do encode other non-URL chars too (think of any accented chars).

Comment #14

pwolanin CreditAttribution: pwolanin commented 17 July 2007 at 23:29

here's a simpler solution - slashes are not allowed as separators...

Comment #15

Gurpartap Singh CreditAttribution: Gurpartap Singh commented 25 July 2007 at 16:33

Patch needs to be re-rolled.

Comment #16

Steven CreditAttribution: Steven commented 25 July 2007 at 23:46

Let's discuss this a bit. If the search paths are *the* use case, then IMHO we are better off with encoding the slash. We do encode other non-URL chars too (think of any accented chars).

Since urlencoded characters are syntactically equivalent to the unencoded character (in cases where the unencoded character has no special meaning), this would achieve nothing. This is exactly the reason why our workaround for Apache's %2F-blocking 'feature' works.

Comment #17

chx CreditAttribution: chx commented 25 August 2007 at 05:04

File	Size
s1.png	9.17 KB

So I typed in foo/bar/this that/more as a search and look where the users tab point to: foo.

Comment #18

chx CreditAttribution: chx commented 25 August 2007 at 05:07

File	Size
s2_0.png	9.04 KB

This is after the patch.

Comment #19

chx CreditAttribution: chx commented 25 August 2007 at 05:08

File	Size
menu_tail_1.patch	2.36 KB

Applied still with offset. Clearer if I reroll.

Comment #20

Gurpartap Singh CreditAttribution: Gurpartap Singh commented 25 August 2007 at 05:13

Title:	Introduce "Tail arguments" wild card in menu paths	» Introduce "Tail arguments" in menu paths
Status:	Needs review	» Reviewed & tested by the community

Those screenshots explain is more properly.