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

Remaining tasks

  1. Decide whether to provide a helper.
  2. Roll a patch.
  3. 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.
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Steven Jones’s picture

Version: 8.x-dev » 7.x-dev

I'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:


function my_page_callback($somearg, $otherarg, $autocomplete) {
  // Get the correct $autocomplete terms
  $args = func_get_args();
  $autocomplete = drupal_autocomplete_helper(2, $args);

  // Now the actual callback

}

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!

Steven Jones’s picture

My 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.

xjm’s picture

Version: 7.x-dev » 8.x-dev

We still should put it in 8.x first. :)

Steven Jones’s picture

@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.

scottalan’s picture

Assigned: Unassigned » scottalan
scottalan’s picture

Status: Active » Needs review
FileSize
1.45 KB

I'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!!

xjm’s picture

Title: Document proper use of #autocomplete_path for slashes (and possibly provide a helper) » Document proper use of #autocomplete_path for slashes
Status: Needs review » Needs work
Issue tags: -Needs tests

Awesome, 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:

  1. +++ b/core/includes/form.incundefined
    @@ -3709,6 +3709,36 @@ function theme_vertical_tabs($variables) {
    + * * Here is an example of how to use an autocompletion callback:
    

    Double asterisk here (oops!). Though, I think we can leave this sentence out entirely.

  2. +++ b/core/includes/form.incundefined
    @@ -3709,6 +3709,36 @@ function theme_vertical_tabs($variables) {
    + * Autocomplete callback functions tend to have a bug wherein forward-slash
    + * characters cause an error.
    + *
    + * If the request has a '/' in the text, then the menu system will split it
    + * into multiple arguments and recover the intended $keywords.
    

    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."

  3. +++ b/core/includes/form.incundefined
    @@ -3709,6 +3709,36 @@ function theme_vertical_tabs($variables) {
    + *
    + * @code
    + * '#autocomplete_path' => 'mymodule_autocomplete/' . $some_key . '/' . $some_id,
    + * @endcode
    + *
    

    I think I'd remove the blank lines before and after the code snippet so that this section forms one paragraph.

  4. +++ b/core/includes/form.incundefined
    @@ -3709,6 +3709,36 @@ function theme_vertical_tabs($variables) {
    + * and the user types in "keywords" so the full path called is:
    + *
    + * 'mymodule_autocomplete/$some_key/$some_id/keywords'.
    

    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:"

  5. +++ b/core/includes/form.incundefined
    @@ -3709,6 +3709,36 @@ function theme_vertical_tabs($variables) {
    + *   array_shift($args);
    + *   array_shift($args);
    

    I'd add an inline comment here indicating that this removes $arg1 and $arg2 from the beginning of the array.

  6. +++ b/core/includes/form.incundefined
    @@ -3709,6 +3709,36 @@ function theme_vertical_tabs($variables) {
    + *   // Your code here.
    

    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!

scottalan’s picture

Re-submitting a patch with @xjm 's suggested modifications to better the documentation.

scottalan’s picture

Status: Needs work » Needs review

changing to needs review...

Is this step necessary for documentation, as to maintain protocol?

tstoeckler’s picture

Status: Needs review » Reviewed & tested by the community

Looks 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. :)

xjm’s picture

Status: Reviewed & tested by the community » Needs work

Thanks @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:

+++ b/core/includes/form.incundefined
@@ -3705,6 +3705,33 @@ function theme_vertical_tabs($variables) {
+ * If your autocomplete path in the menu is 'mymodule_autocomplete' and in
+ * your form you have:
...
+ * and the user types in "keywords" so the full path called is:

Let's make this three sentences:

Suppose your autocomplete path in the menu system is 'mymodule_autocomplete.' In your form you have:
...
The user types in "keywords" so that the full path called is:

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:

Since @xjm has already had a look at this and all of his her remarks have been fixed

:)

tstoeckler’s picture

@xjm: Oops, I didn't realize that. Sorry!!!

scottalan’s picture

Status: Needs work » Needs review
FileSize
1.61 KB

Trying once more :)

xjm’s picture

Status: Needs review » Reviewed & tested by the community

Alright, this looks ready to me. Thanks @scottalan and @tstoeckler!

catch’s picture

Version: 8.x-dev » 7.x-dev
Status: Reviewed & tested by the community » Patch (to be ported)

Docs 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.

tim.plunkett’s picture

Status: Patch (to be ported) » Needs review
FileSize
1.52 KB

Rerolled.

JvE’s picture

Status: Needs review » Needs work

+ * // 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 contain keywords.

xjm’s picture

Status: Needs work » Needs review

#16: drupal-1492378-16.patch queued for re-testing.

xjm’s picture

Status: Needs review » Reviewed & tested by the community

#17 would need to go into 8.x as well, so a followup would be fine.

The backport is RTBC.

webchick’s picture

Component: forms system » documentation
Assigned: scottalan » jhodgdon

Moving to proper component, adding to Jennifer's queue.

jhodgdon’s picture

Assigned: jhodgdon » Unassigned
Status: Reviewed & tested by the community » Fixed

Thanks -- nice work on the documentation here! Committed to 7.x.

tstoeckler’s picture

Version: 7.x-dev » 8.x-dev

SInce we're only at 22 comments now, re-opening for #17. Still novice.

tstoeckler’s picture

Status: Fixed » Active

Oops.

kbasarab’s picture

Status: Active » Needs review
FileSize
551 bytes

I feel like there is probably a better way to write this but it gets us started again.

jhodgdon’s picture

Status: Needs review » Needs work

Thanks, 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?

kbasarab’s picture

Status: Needs work » Needs review
FileSize
713 bytes
577 bytes

Good 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,.."

jhodgdon’s picture

Assigned: Unassigned » jhodgdon
Status: Needs review » Reviewed & tested by the community

That 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).

jhodgdon’s picture

Assigned: jhodgdon » Unassigned
Status: Reviewed & tested by the community » Fixed

Thanks! Committed follow-up patch to 8.x and 7.x.

Automatically closed -- issue fixed for 2 weeks with no activity.

Anonymous’s picture

Issue summary: View changes

Add link to another example.