Posted by chx on August 22, 2008 at 10:41am
11 followers
| Project: | Drupal core |
| Version: | 7.x-dev |
| Component: | menu system |
| Category: | bug report |
| Priority: | normal |
| Assigned: | chx |
| Status: | closed (fixed) |
Issue Summary
This is a feature request by Steven Wittens and I coded it but have not tested it.
| Attachment | Size | Status | Test result | Operations |
|---|---|---|---|---|
| menu_tail.patch | 1.36 KB | Idle | FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch menu_tail_3.patch. | View details |
Comments
#1
Some comments would be helpful. I have no idea when to use this, nor even what it does.
#2
This would be very useful for auto-completion functions. It's unfortunate that it feels like %menu_tail was added as an incomplete feature. :/
See
#93854: Allow autocompletion requests to include slashes
#3
#4
Hm, no, this one works.
#5
with test. I checked that the test fails when the patch is not applied.
#6
"arguments" is misspelled:
+ if (empty($item['load argumnets']) && $function == 'menu_tail_load') {#7
For efficiency's sake, here's the same exact patch except with the typo fixed.
#8
Could we get some PHPDoc for menu_tail_load()?
#9
No :p
edit: if someone comes up with a doxygen, be my guest. I have failed trying to write one. The code is short enough not to need explanation. It's not a function your code calls. So why bother with a lengthy complicated doxygen?
#10
subscribe
#11
Here's some very simple Doxygen for this and also for menu_tail_to_arg(). As chx wrote, not much has to be written here as these are automatically called functions documented here: http://drupal.org/node/109153#load (although this needs to be slightly updated for 7.x, namely the part saying that menu_tail_load doesn't exist).
#12
This seems a little too hard-coded:
+ if (empty($item['load arguments']) && $function == 'menu_tail_load') {+ $item['load arguments'] = array('%map', '%index');
+ }
Can we just make it:
+ if (empty($item['load arguments'])) {+ $item['load arguments'] = array('%map', '%index');
+ }
#13
Wouldn't that make this sort of an API change, since you'd now be passing two additional arguments to every load function?
#14
Yeah and then load functions might have condition, reset and other tidbits...
#15
ignore #12 - I was confusing load arguments with something else. I think we have little choice but to special-case this if we want it to work.
#16
What about requiring hook_menu()s to manually specify those load arguments, if they want to use menu_tail_load()?
A bit harder on the developers, but
- we wouldn't need a hard-coded special case,
- probably only few will use it anyways, and
- those should be able to read the documentation of what they are using.
I think this will be an API change anyways, since a page callback might now get different arguments, if %menu_tail was previously used without intended load functionality.
In any case, +1 for doing this, in some way. Has arguably been a bit of a pain up to now.
#17
menu_tail_load does not work without map and index. There is no point in mandating the poor module writers to add that always. Given that already %menu_tail is already at the end of the relevant paths, nothing gets broken by the loader. Special casing is not the best, yes but given it's an internal menu function it's not that bad.
#18
core only uses it one place - I think specifying the load arguments might be preferred?
modules/search/search.module:211: $items["$path/%menu_tail"] = array(#19
Core does. And? I do not get your point, do you really want module authors do an elaborate dance to use %menu_tail? Note that if you use %menu_tail with this patch and not load arguments, PHP will be very, very unhappy when it gets only one argument and expects three. This part is not optional.
#20
Well, if PHP will be very, very unhappy, then module developers will almost immediately realize their mistake and add those arguments. It's not an "elaborate dance", it's two strings and would be well-documented.
#21
Here's the patch putting it just in hook_menu.
#22
Looks good to me, seems to work perfectly and the bot doesn't complain either.
#23
+++ modules/search/search.test@@ -282,6 +282,31 @@ class SearchPageText extends DrupalWebTestCase {
+class SearchSlashTestCase extends DrupalWebTestCase {
+ public static function getInfo() {
+ return array(
+ 'name' => 'Search slash',
+ 'description' => 'Tests that searches containing slashes work.',
+ 'group' => 'Search'
Missing phpDoc for the test class.
But on a second thought, I wonder why this is a separate test case in the first place? Isn't there an existing one already?
+++ modules/search/search.test@@ -282,6 +282,31 @@ class SearchPageText extends DrupalWebTestCase {
+ // This verifies whether the arg appears in the search form. Not as
+ // accurate as writing a complicated xpath but works.
+ $this->assertRaw("\"$arg");
Can we at least use single quotes and string concatenation for the value, and make the comment actually state what is being asserted here, and why?
Powered by Dreditor.
#24
Looks like it can easily be merged with the prior test case - that's pretty short and generic.
#25
Still works perfectly for me. (Sorry I forgot about this issue …)
Let's just let the test bot check this once again (although, as said, it still applies cleanly).
#26
#24: 298561-menu-tail-load-24.patch queued for re-testing.
#27
#24: 298561-menu-tail-load-24.patch queued for re-testing.
#28
D7 is closed for features.
#29
webchick, right now date search is broken unless this patch goes in. i.e., searching for "1/15/2001" or "9/11" does not work on Drupal 7 (see #915214: Regression: Slashes broken in search for more info on this).
#30
Agreed - this is a bug fix. "task" was more a carry-over before we realized what else was broken.
#31
Ah, ok. I think I misread when I was whipping through these the other day. Sorry!
Committed to HEAD.
#32
Automatically closed -- issue fixed for 2 weeks with no activity.