Document proper use of #autocomplete_path for slashes

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

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!

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

:)

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

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

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

The backport is RTBC.