Problem/Motivation
Drupal autocompletion fails for the #autocomplete_path
property if the value to be autocompleted may contain slashes. This is fixed for core taxonomy autocomplete in #93854: Allow autocompletion requests to include slashes, but it may not be clear that contributed modules need to make a similar change.
Proposed resolution
- Possibly factor out a generic autocomplete helper based on this bit added in #93854: Allow autocompletion requests to include slashes:
// If the request has a '/' in the search text, then the menu system will have // split it into multiple arguments, recover the intended $tags_typed. $args = func_get_args(); // Shift off the $field_name argument. array_shift($args); $string = implode('/', $args);
See also:
http://api.drupal.org/api/drupal/developer%21topics%21forms_api_referenc... - Document how to support autocompletion of values containing slashes on the
#autocomplete_path
property.
Remaining tasks
- Decide whether to provide a helper.
- Roll a patch.
- If a helper is provided, add a FAPI test with a dummy module with its own autocompletion, in addition to the existing taxonomy tests? Maybe?
API changes
- Possible API addition of a helper function for autocomplete path handling.
Comment | File | Size | Author |
---|---|---|---|
#26 | drupal-1492378-26.patch | 577 bytes | kbasarab |
#26 | interdiff.txt | 713 bytes | kbasarab |
#24 | drupal-1492378-24.patch | 551 bytes | kbasarab |
#16 | drupal-1492378-16.patch | 1.52 KB | tim.plunkett |
#13 | autocomplete_path-doc-1492378-13.patch | 1.61 KB | scottalan |
Comments
Comment #1
Steven Jones CreditAttribution: Steven Jones commentedI'm not sure a helper would help that much, because you'd still need to call it correctly in PHP 5.2 it would have to look something like this:
This would still be obscure, more so than just documenting it I think.
I'm moving this to D7, because in D8 a better solution would be to just fix
menu_tail_load
so it can just be used as we'd all like it to be. I'll raise another issue for that. Feel free to disagree with me on this point!Comment #2
Steven Jones CreditAttribution: Steven Jones commentedMy follow up on getting menu tail load to a useful point is over here: #1492464: Make menu tail load work without specifying the loader arguments.
Comment #3
xjmWe still should put it in 8.x first. :)
Comment #4
Steven Jones CreditAttribution: Steven Jones commented@xjm although I agree with that in principle, we should take the opportunity to change the API in D8 and 'fix' this using: #1492464: Make menu tail load work without specifying the loader arguments then we probably just need to document that in D8, and document this issue in D7 and below.
Comment #5
scottalan CreditAttribution: scottalan commentedComment #6
scottalan CreditAttribution: scottalan commentedI'm attaching a patch that contains documentation explaining how to handle forward-slashes with #autocomplete_path. I've never contributed documentation, so I apologize in advance if this is completely out of line. This seemed to make the most sense as far as placement is concerned. Criticism welcome!!
Comment #7
xjmAwesome, thanks @scottalan. That looks like the correct API function. (Once we have finalized the wording, we should probably add it in the FAPI handbook chart as well. See also #100680: [meta] Make API module generate Form API documentation.)
One correction I have is that these paragraphs of text should go above the parameter documentation rather than below it. It should immediately follow the one-line summary after a blank line. Reference: http://drupal.org/node/1354#functions
Other feedback:
Double asterisk here (oops!). Though, I think we can leave this sentence out entirely.
I think it would be better to merge these two paragraphs. I'd actually replace the first sentence to be more specific (and to not refer to a bug since we're making this a documented behavior.) Maybe:
"Note that autocomplete callbacks should include special handling if the user input may contain forward slashes. If the user-submitted string has a '/' in the text that is sent in the autocomplete request, the menu system will split the text and pass it to the callback as multiple arguments."
I think I'd remove the blank lines before and after the code snippet so that this section forms one paragraph.
We can remove the blank line between these two. I'd also remove the period at the end of the second line, I think.
Maybe we can add another paragraph after it that says something like: "Then you should include code like the following to handle slashes in the input:"
I'd add an inline comment here indicating that this removes
$arg1
and$arg2
from the beginning of the array.I'd add a comment above this line with some more specific information. Maybe something like "The user's original input is now in $keywords, including any slashes."
Great job on this!
Comment #8
scottalan CreditAttribution: scottalan commentedRe-submitting a patch with @xjm 's suggested modifications to better the documentation.
Comment #9
scottalan CreditAttribution: scottalan commentedchanging to needs review...
Is this step necessary for documentation, as to maintain protocol?
Comment #10
tstoecklerLooks awesome! Since @xjm has already had a look at this and all of his remarks have been fixed, I feel comfortable RTBC-ing this myself.
This would have saved me ~1 hour of figuring this out myself recently, so let's get this in. :)
Comment #11
xjmThanks @tstoeckler and @scottalan!
It looks like we still need the second part of point 4, though (paragraph between the "if" part and the "then" part). I think that would make it a little more clear. I'd also like to suggest a slight improvement to the wording so it doesn't sound like a run-on:
Let's make this three sentences:
I think that sounds a little better. With those two changes (as above, plus adding the second sentence from point 4 above), then I'd consider it RTBC too.
@scottalan, Yep, you can set the issue to "Needs Review" any time an updated patch is submitted. This triggers testbot to automatically make sure the patch applies (which is relevant even for documentation patches), and also signals to reviewers that there are new changes to review.
Aside @tstoeckler:
:)
Comment #12
tstoeckler@xjm: Oops, I didn't realize that. Sorry!!!
Comment #13
scottalan CreditAttribution: scottalan commentedTrying once more :)
Comment #14
xjmAlright, this looks ready to me. Thanks @scottalan and @tstoeckler!
Comment #15
catchDocs look good, would be good to fix this properly in 8.x but documenting the requirements here makes sense to avoid drift.
Committed/pushed to 8.x, moving to 7.x for backport.
Comment #16
tim.plunkettRerolled.
Comment #17
JvE CreditAttribution: JvE commented+ * // We store the user's original input in $keywords, including any slashes.
Perhaps we should add a note that a leading or trailing slash in the user's original input will not be kept.
i.e. if the user enters
/keywords/
then $keywords will containkeywords
.Comment #18
xjm#16: drupal-1492378-16.patch queued for re-testing.
Comment #19
xjm#17 would need to go into 8.x as well, so a followup would be fine.
The backport is RTBC.
Comment #20
webchickMoving to proper component, adding to Jennifer's queue.
Comment #21
jhodgdonThanks -- nice work on the documentation here! Committed to 7.x.
Comment #22
tstoecklerSInce we're only at 22 comments now, re-opening for #17. Still novice.
Comment #23
tstoecklerOops.
Comment #24
kbasarab CreditAttribution: kbasarab commentedI feel like there is probably a better way to write this but it gets us started again.
Comment #25
jhodgdonThanks, that's pretty close!
But I think #17 (and your example) are saying that slashes are removed from the front and back of the string -- and your wording says "preprended" only (maybe say "leading and trailing"?).
Also, a nitpick: "i.e. If the..." should be "I.e., if the..." (i.e. starts this sentence)... well actually, it should be "E.g." or better yet just "For example, ". i.e. means "that is", and this is an example.
I also think the example would be better if the sample text was something other than "keywords" and if it contained a slash. Maybe use "/a/few/words/" as the input text?
Comment #26
kbasarab CreditAttribution: kbasarab commentedGood call. When I originally wrote this I had multiple keywords in the doc and then I took them out for some reason or another. Added back in and switch to "For example,.."
Comment #27
jhodgdonThat looks good to me, thanks! Assuming the test bot agrees, I'll get it committed to 8.x and 7.x (not sure if it will need a port/reroll for 7 or not until I try).
Comment #28
jhodgdonThanks! Committed follow-up patch to 8.x and 7.x.
Comment #29.0
(not verified) CreditAttribution: commentedAdd link to another example.