Problem/Motivation

Taxonomy terms may contain slashes, but the autocomplete widget used to populate a taxonomy term list does not allow slashes.

Proposed resolution

The latest patch in #207 implements the following solution:

  • Changes misc/javascript.js to enable autocomplete terms to contain slashes.

  • Tweaks the autocomplete menu callback to include all arguments passed by the menu system.

  • Adds automated test coverage for this functionality.

Remaining tasks

  • The approach needs to be reviewed, manually tested and then committed to D8, D7 and D6.

Steps to test this patch

  1. Create an article and enter a tag containing a slash. Save the article and verify the term is created correctly.
  2. Create a second article and try to use the autocomplete to add the term you added to the first article. Save the article and check at admin/structure/taxonomy/tags/add to make sure it used the same tag rather than creating a new one.
  3. Add some other terms containing slashes to the tags vocabulary at admin/structure/taxonomy/tags/add. Try some different patterns: 11/1/2011, 11/2/2011, import/export, "Term name containing a comma, plus / slashes / too.", This term's got / slashes and apostrophes. Etc.
  4. Test autocompletion and saving of these terms as well.

User interface changes

None.

API changes

  • None.

Original report by moonray

Using a slash in an autocomplete field breaks functionality.

example url:
http://localhost/drupal/recipe/ingredient/autocomplete/looking%20for%201...

error:
Not Found

The requested URL /drupal/recipe/ingredient/autocomplete/looking for 1/2 a loaf was not found on this server.
Apache/2.0.55 (Ubuntu) PHP/5.1.6 Server at localhost Port 80

The reason (I believe) is that it interprets the escaped slash as a regular slash, and thus a path delimiter. This error does not occur when you don't use clean urls.

Is there another way to escape forward slashes than %2F, so they don't get interpreted as a path delimiter?

If you give me that answer I can write a patch to submit. :-)

Files: 
CommentFileSizeAuthor
#258 93854-D6-258-do-not-test.patch777 bytespwolanin
#252 drupal-93854-autocomplete-slashes-251.patch4.3 KBndobromirov
FAILED: [[SimpleTest]]: [MySQL] 40,597 pass(es), 1 fail(s), and 0 exception(s). View
#213 drupal-93854-autocomplete-slashes-extra-test-d6.patch779 bytesSteven Jones
PASSED: [[SimpleTest]]: [MySQL] 190 pass(es). View
#207 drupal-93854-autocomplete-slashes-extra-test-d7.patch4.61 KBSteven Jones
PASSED: [[SimpleTest]]: [MySQL] 38,318 pass(es). View
#200 autocomplete_ie9.png9.36 KBSteven Jones
#200 autocomplete_ie8.png7.69 KBSteven Jones
#200 autocomplete_ie7.png9.16 KBSteven Jones
#190 Screen Shot 2012-03-07 at 12.20.22 PM.png74.04 KBryanissamson
#188 drupal-93854-autocomplete-slashes-extra-test.patch4.67 KBSteven Jones
PASSED: [[SimpleTest]]: [MySQL] 35,086 pass(es). View
#185 drupal-93854-autocomplete-slashes-no-t.patch3.77 KBSteven Jones
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal-93854-autocomplete-slashes-no-t.patch. Unable to apply patch. See the log in the details link for more information. View
#185 drupal-93854-autocomplete-slashes-extra-test.patch4.68 KBSteven Jones
PASSED: [[SimpleTest]]: [MySQL] 35,060 pass(es). View
#184 drupal-93854-autocomplete-slashes-just-test.patch2.07 KBSteven Jones
FAILED: [[SimpleTest]]: [MySQL] 35,047 pass(es), 1 fail(s), and 0 exception(s). View
#181 drupal-93854-autocomplete-slashes-simple.patch3.78 KBSteven Jones
PASSED: [[SimpleTest]]: [MySQL] 34,810 pass(es). View
#178 drupal-93854-autocomplete-slashes-menu_tail_load.patch6.2 KBSteven Jones
PASSED: [[SimpleTest]]: [MySQL] 34,798 pass(es). View
#171 fix-autocompletion-with-slashes-93854-171.patch4.02 KBmstef
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch fix-autocompletion-with-slashes-93854-171.patch. Unable to apply patch. See the log in the details link for more information. View
#165 rollback-93854.patch4.08 KBxjm
PASSED: [[SimpleTest]]: [MySQL] 34,308 pass(es). View
#149 autocomplete-slashes-d7-93854-149.patch4.02 KBxjm
PASSED: [[SimpleTest]]: [MySQL] 37,070 pass(es). View
#141 Screen Shot 2011-11-16 at 8.03.18 PM.png13.21 KBeric.duran7@gmail.com
#141 Screen Shot 2011-11-16 at 8.03.08 PM.png13 KBeric.duran7@gmail.com
#141 Screen Shot 2011-11-16 at 8.00.34 PM.png10.18 KBeric.duran7@gmail.com
#141 Screen Shot 2011-11-16 at 8.00.21 PM.png10.26 KBeric.duran7@gmail.com
#135 autocomplete_slashes_test_only.patch2.08 KBxjm
FAILED: [[SimpleTest]]: [MySQL] 33,857 pass(es), 2 fail(s), and 0 exception(es). View
#134 fix-autocompletion-with-slashes-93854-134.patch4.08 KBmstrelan
PASSED: [[SimpleTest]]: [MySQL] 33,849 pass(es). View
#132 fix-autocompletion-with-slashes-93854-132.patch4.77 KBpillarsdotnet
PASSED: [[SimpleTest]]: [MySQL] 33,445 pass(es). View
#132 interdiff_93584_60-132.txt4.35 KBpillarsdotnet
#131 interdiff_93584_60-129.txt4.65 KBpillarsdotnet
#129 fix-autocompletion-with-slashes-93854-129.patch4.82 KBpillarsdotnet
PASSED: [[SimpleTest]]: [MySQL] 33,456 pass(es). View
#127 fix-autocompletion-with-slashes-93854-127.patch4.23 KBpillarsdotnet
PASSED: [[SimpleTest]]: [MySQL] 33,458 pass(es). View
#126 93854-1.patch3.18 KBeric.duran7@gmail.com
PASSED: [[SimpleTest]]: [MySQL] 33,450 pass(es). View
#122 fix-autocompletion-with-slashes-93854-119.patch4.75 KBpillarsdotnet
PASSED: [[SimpleTest]]: [MySQL] 33,470 pass(es). View
#119 0001-Issue-865536-by-ericduran-Added-drupal_add_js-is-mis.patch18.92 KBeric.duran7@gmail.com
PASSED: [[SimpleTest]]: [MySQL] 33,445 pass(es). View
#120 93854.patch3.15 KBeric.duran7@gmail.com
PASSED: [[SimpleTest]]: [MySQL] 33,457 pass(es). View
#116 fix-autocompletion-with-slashes-93854-116.patch2.68 KBpillarsdotnet
FAILED: [[SimpleTest]]: [MySQL] 33,460 pass(es), 1 fail(s), and 0 exception(es). View
#109 0001-Issue-93854-by-moonray-becw-Dave-Reid-pounard-das-pe.patch4.19 KBaxel.rutz
PASSED: [[SimpleTest]]: [MySQL] 33,662 pass(es). View
#108 fix_autocompletion_with_slashes-93854-108.patch4.35 KBpillarsdotnet
PASSED: [[SimpleTest]]: [MySQL] 33,645 pass(es). View
#102 fix_autocompletion_with_slashes-93854-99.patch4.36 KBpillarsdotnet
PASSED: [[SimpleTest]]: [MySQL] 33,662 pass(es). View
#100 fix_autocompletion_with_slashes-93854-99-make.patch4.33 KBe2thex
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch fix_autocompletion_with_slashes-93854-99-make.patch. See the log in the details link for more information. View
#99 fix_autocompletion_with_slashes-93854-99.patch4.36 KBpillarsdotnet
PASSED: [[SimpleTest]]: [MySQL] 33,649 pass(es). View
#97 fix_autocompletion_with_slashes-93854-96.patch4.29 KBpillarsdotnet
FAILED: [[SimpleTest]]: [MySQL] 33,651 pass(es), 1 fail(s), and 0 exception(es). View
#84 fix_autocompletion_with_slashes-93854-76.patch4.36 KBSteven Jones
PASSED: [[SimpleTest]]: [MySQL] 33,598 pass(es). View
#76 fix_autocompletion_with_slashes-93854-76.patch4.41 KBpillarsdotnet
PASSED: [[SimpleTest]]: [MySQL] 29,759 pass(es). View
#75 fix_autocompletion_with_slashes-93854-75.patch4.35 KBpillarsdotnet
PASSED: [[SimpleTest]]: [MySQL] 29,764 pass(es). View
#75 fix_autocompletion_with_slashes-93854-70_to_75-interdiff.txt2.5 KBpillarsdotnet
#74 fix_autocompletion_with_slashes-93854-74.patch18.44 KBpillarsdotnet
PASSED: [[SimpleTest]]: [MySQL] 29,890 pass(es). View
#70 drupal-autocomplete-93854.patch4.15 KBSteven Jones
PASSED: [[SimpleTest]]: [MySQL] 29,405 pass(es). View
#65 taxonomy_autocomplete.patch2.75 KBpillarsdotnet
PASSED: [[SimpleTest]]: [MySQL] 31,547 pass(es). View
#60 drupal-93854-60-test.patch2.81 KBSteven Jones
PASSED: [[SimpleTest]]: [MySQL] 31,540 pass(es). View
#56 drupal-93854-56-test.patch2.27 KBSteven Jones
PASSED: [[SimpleTest]]: [MySQL] 31,541 pass(es). View
#55 drupal-93854-55-test.patch1.34 KBSteven Jones
FAILED: [[SimpleTest]]: [MySQL] 31,542 pass(es), 1 fail(s), and 0 exception(es). View
#41 autocomplete-optional-double-encoding.patch1.38 KBdas-peter
FAILED: [[SimpleTest]]: [MySQL] Invalid patch format in autocomplete-optional-double-encoding.patch. View
#17 93854-drupal-get-path-segment-2-D7.patch6.1 KBDave Reid
Passed: 10624 passes, 0 fails, 0 exceptions View
#15 93854-drupal-get-path-segment-D7.patch16.94 KBDave Reid
Failed: 10266 passes, 7 fails, 0 exceptions View
#12 93854-drupal-get-path-segment-D7.patch14.03 KBDave Reid
Failed: 10266 passes, 7 fails, 0 exceptions View

Comments

moonray’s picture

Version: 4.7.4 » 5.0-rc1

I originally filed this under 4.7, but I believe this is still an issue in 5.
Please double check...

moonray’s picture

Version: 5.0-rc1 » 5.x-dev

Oops... 5.0.x-dev

Anonymous’s picture

this should be a simple fix..

if i don't get a chance to fix it when i get home, i bet steven will get to it before me :P

Steven’s picture

Version: 5.x-dev » 4.7.x-dev

The diagnosis here is incorrect. The culprit is a 'security' feature in apache that 404s URLs containing %2F. The solution was added in Drupal 5 with Drupal.encodeURIComponent.

This needs to be backported to 4.7, but 5 works fine.

dman’s picture

Related, possibly a new issue against 5.0, but...
The current 5.0 drupal.js contains

/**
 * Wrapper to address the mod_rewrite url encoding bug
 * (equivalent of drupal_urlencode() in PHP).
 */
Drupal.encodeURIComponent = function (item, uri) {
  uri = uri || location.href;
  item = encodeURIComponent(item).replace('%2F', '/');
  return uri.indexOf('?q=') ? item : item.replace('%26', '%2526').replace('%23', '%2523');
};

which is a game effort, but buggy.
in javascript replace('%2F', '/'); is non-greedy, and produces:

alert(Drupal.encodeURIComponent("http://my.demo/etc.etc"))
http%3A/%2Fmy.demo%2Fetc.etc

(with unclean urls)

It unescaped just ONE of the slashes producing a problem.

I'd suggest patching to

  item = encodeURIComponent(item).replace(/%2F/g, '/');

... if swapping all encoded slashes back to slashes was indeed the intent.

Leonid.S’s picture

I use Drupal 5.3.
As author of this post has write, the issue with autocomplete is that drupal interprets any slash as path delimiter.

For generating data for jQuery request profile.module (and bunch of other modules) use function with some arguments, one of which is search string.

function profile_autocomplete($field, $string) {

}

This function is callback for path like

    $items[] = array('path' => 'profile/autocomplete', 'title' => t('Profile autocomplete'),
      'callback' => 'profile_autocomplete',
      'access' => 1,
      'type' => MENU_CALLBACK);

So for example if I search for "http://a" in one of profile autocomplete box jQuery ask server for path like
profile/autocomplete/8/http%3A/%252Fa. First argument $field in function will be '8', second argument $string will be 'http:'. And will be another, third argument '/a'. We'll search for 'http:' instead of 'http://a'.

There is some more issues with autocomplete:
1. if I type in autocomplete box something like 'abc.' Drupal (or Apache) strip '.' from uri. (it is not true for something like 'abc.def')
2. if there is '%' (wildcard in MySQL query) in search string it is must be escaped

Leonid.S’s picture

Why instead of GET method for request from autocomplet.js we can't use POST method?
Something like:

    $.ajax({
      type: "POST",
      url: db.uri,
      data: {search: searchString},
...
...
    });

becw’s picture

Version: 4.7.x-dev » 5.7

This can be worked around in individual autocomplete functions by using func_get_args() instead of individually enumerating arguments.

For example, in taxonomy.module, taxonomy_autocomplete() looks like this:

/**
 * Helper function for autocompletion
 */
function taxonomy_autocomplete($vid, $string = '') {
  // The user enters a comma-separated list of tags...
  ...

Currently, if a user is trying to autocomplete something like "children / youth" from vocab 2, taxonomy_autocomplete will get called like this: taxonomy_autocomplete(2, "children ", " youth"); everything after the "/" gets ignored.

If taxonomy_autocomplete() is changed to this, then the extra arguments get re-joined together and the correct $string is used:

/**
 * Helper function for autocompletion
 */
function taxonomy_autocomplete() {
  $args = func_get_args();
  $vid = array_shift($args);
  $string = implode('/', $args);

  // The user enters a comma-separated list of tags...
  ...

Make any sense?

drumm’s picture

Component: javascript » taxonomy.module
Priority: Critical » Normal

Yes, #8 has the correct approach. See http://drupal.org/patch/create for information on how to post a patch.

Dave Reid’s picture

Version: 5.7 » 7.x-dev

Let's check if this is fixed in 7.x now, fix there, then backport.

Dave Reid’s picture

Dave Reid’s picture

Component: taxonomy.module » base system
Status: Active » Needs review
Issue tags: +autocomplete, +path.inc
FileSize
14.03 KB
Failed: 10266 passes, 7 fails, 0 exceptions View

Ok, we should really generalize this since it's small and it can be re-used throughout core. It seems this is very useful for more than just auto-completion paths. I propose a new function 'drupal_get_path_segment' for path.inc:

/*
 * Extract part of the current Drupal path from a certain 'argument' onward.
 *
 * For example, if the path is 'http://example-drupal/blah/foo/foobar/ferzle':
 *   $pos = 0, returns 'blah/foo/foobar/ferzle'
 *   $pos = 2, returns 'foobar/ferzle'
 *   $pos = 4, returns ''
 *
 * @param $pos
 *   The argument of the path to start at, use 0 to get the whole path.
 * @return
 *   The extracted part of the path.
 */
function drupal_get_path_segment($arg = 0, $path = NULL) {
  if (!isset($path)) {
    $path = trim($_GET['q']);
  }

  if ($arg > 0) {
    $path = explode('/', $path, $arg + 1);
    $path = (count($path) > $arg ? end($path) : '');
  }

  return $path;
}

So now in the case of taxonomy_autocomplete(), we would do the following:

function taxonomy_autocomplete($vid) {
  $string = drupal_get_path_segment(3);

Initial patch with initial tests for review.

Dave Reid’s picture

Title: Autocomplete and slash breaks » Add drupal_get_path_segment() and fix autocompletion strings with slashes

Status: Needs review » Needs work

The last submitted patch failed testing.

Dave Reid’s picture

Status: Needs work » Needs review
FileSize
16.94 KB
Failed: 10266 passes, 7 fails, 0 exceptions View

And my argument for needing this in core, is that this function can replace:

function path_admin_filter_get_keys() {
  // Extract keys as remainder of path
  $path = explode('/', $_GET['q'], 5);
  return count($path) == 5 ? $path[4] : '';
}
function search_get_keys() {
  static $return;
  if (!isset($return)) {
    // Extract keys as remainder of path
    // Note: support old GET format of searches for existing links.
    $path = explode('/', $_GET['q'], 3);
    $keys = empty($_REQUEST['keys']) ? '' : $_REQUEST['keys'];
    $return = count($path) == 3 ? $path[2] : $keys;
  }
  return $return;
}

I'm searching to see if there's more places this could be used.

Status: Needs review » Needs work

The last submitted patch failed testing.

Dave Reid’s picture

Status: Needs work » Needs review
FileSize
6.1 KB
Passed: 10624 passes, 0 fails, 0 exceptions View

Instead of adding a whole new path.test, let's see if this one passes the test bot.

pounard’s picture

Still getting this error with Drupal 6, any patch supplied ?

#8 DOES NOT have the right approach. In my case, I have a custom element, providing an autocomplete, and I send multiple parameters to my autocomplete method.

With func_get_args() I'll get an array of argument, but what happens is this case:


// Menu
function my_menu() {
  $items['myautocomplete/%/%'] = array(
    'page callback' => 'myautocomplete',
    'page arguments' => array(1, 2),
    // ..
  );
  return $items;
}

// My url in form element is like this
function some_form($form_state, $somefilter) {
  $form['foo'] = array(
    // ..
    'autocomplete_path' => url('myautocomplete/'.$somefilter);
  );
  return $form;
}

// Method signature
function my_autocomplete($arg1, $arg2) {
  // Code here
  // Then, I call the url mydomain.com/myautocomplete/a/list/of/words/slash/separated
  // How do I get the right arg1, and the right arg2 ?
}

// This clear hu?

Note: my arg1 is a filter, I use in my autocomplete as result filtering. This filter is set as an option of my custom element. But what if this filter contains slashes, your patch won't work. It's absolutely not a general solution, neither a good solution, it's a working workaround for general use case only.

Multiple post edit: typo and details.

pounard’s picture

Talking with a sysadmin, I saw multiple solutions, they all have benefits and disavantages:

Double encoding

Encode twice the strings for autocomplete.

Benefits

  • Will get throught all eventual security checks;

Disadvantages

  • Somehow can be really heavy in term of cpu usage;
  • Needs post treatment in callback.

Base 64 encoding

Encode all parameters with base64

Benefits
The same as double encoding.

Disadvantages
The same as double encoding.

Send parameters through POST

Send (all) autocomplete parameter(s) using a POST request

Benefits

  • This is send as REAL data, no URL encode problems at all;
  • Will implicitly tell the eventual proxies that it cant send back some cached results.

Disadvantages

  • Needs a serious review and rewrite of the feature's Javascript code;
  • Will implicitly tell the eventual proxies that it cant send back some cached results.

I decided to write caching as both a benefit OR a disadvantage, it depends on what kind of data the web server must send back, of your use case, etc.

In all cases..

..you have to provide both encoding and decoding method, in both client side (Javascript) and server side (PHP) languages so the users can tweak whatever the want, without the risk of breaking anything trying to do their own encoding/decoding functions.

Dave Reid’s picture

@pounard: What about the proposal and solution in #12?

pounard’s picture

@Dave Reid: The #12 proposition won't work, in my case, with the issue I'm facing.

This is still a working workarround for general use case, it will work for most autocompletes.

EDIT: and I really think that using POST (which has some disadvantages) seems to be the best solution to avoid those workarrounds.

Dave Reid’s picture

@pounard: Your problem in #18 can be solved using the solution in #12 easily:

function my_menu() {
  $items['myautocomplete/%'] = array(
    'page callback' => 'myautocomplete',
    'page arguments' => array(1),
    // ..
  );
  return $items;
}

// My url in form element is like this
function some_form($form_state, $somefilter) {
  $form['foo'] = array(
    // ..
    'autocomplete_path' => url('myautocomplete/'.$somefilter);
  );
  return $form;
}

// Method signature
function myautocomplete($arg1) {
  // $arg1 will contain the value of $somefilter (reasonable to assume that this should not contain any backslashes)
  $arg2 = drupal_get_path_segment(2);
}
pounard’s picture

What if my filter contains one or more slash caracters ?

Dave Reid’s picture

Yeah if that's the case, then you're probably best dealing with something that should not use the Drupal-provided autoc-completion since you don't know where the filter variable could end and the string to match begins (since both variables could be n Drupal path segments).

pounard’s picture

That's what I'm going to do. What I'm doing here is opening a discussion about a real, long-term, non blocking solution for autocomplete.

I'm asking you to really think about other solutions than adding complexety to api. Segmenting the path, IMHO, is really not a good solution, it's an ugly workarround.

I'm ready to do "proof of concept" patches if you are ready to review it, and really think about it.
Autocomplete, because of these encoding problems, which may happen in other cases I think, really should be reviewed.

EDIT: if a solution does not work, a workarround is not a long term solution. Some JS refactor is not a big deal when you are used to code with. When I look the way drupal menu system works, I dont think happending those /% parameters is a good solution. As we really don't need clean URL on this, real GET parameter, or POST request & paremeters should way more do the trick, in a cleaner and proper way.

Dave Reid’s picture

If you're looking to really change the internals of Drupal's autocompletion, you should probably file a separate issue. Link back to them here, I'd like to check them out!

However, I don't think my solution is a workaround. I've given the valid use case for making this an API function since there are other places in core where nearly the same exact code is already in place (see #15), but could just be generalized.

pounard’s picture

Yes, but it'll never work if you do a my/%/menu.

This is NOT a long term solution.

PS: I think this is working for current existing cases, but still ugly.

becw’s picture

@pounard - you should be using %menu_tail instead of % in your hook_menu() to grab all the stuff at the end of your menu path:

/**
 * Implementation of hook_menu().
 */
function my_menu() {
  $items['myautocomplete/%menu_tail'] = array(
    'page callback' => 'myautocomplete',
    'page arguments' => array(1),
    // ..
  );
  return $items;
}

%menu_tail is mentioned in the menu system %wildcard_to_arg docs.

pounard’s picture

What if I want to do this:

myautocomplete/%/foo

or

myautocomplete/%/%/foo

or

myautocomplete/%/foo/%

or

myautocomplete /%/%

Damien Tournoud’s picture

Status: Needs review » Needs work

%menu_tail is there for a reason. Let's fix taxonomy_autocomplete() and friends to use it.

Damien Tournoud’s picture

By the way, this new drupal_get_path_segment($arg, $path) seems nothing more then:

implode('/', array_slice(arg(NULL, $path), $arg))

Except if I'm missing the point?

becw’s picture

@pounard - if %menu_tail doesn't do quite what you need, you should look at using a custom wildcard loader with special wildcard loader arguments; you can pass your custom wildcard loader function the %map and %index special values, and then your loader can take its pick of which menu arguments to use :)

pounard’s picture

I did some wildcard loaders in custom code.
This is not the question.
ANY wildcard loader WILL BE BROKEN if slashes remains in the url.

Autocomplete parameters SHOULD NOT be passed through the drupal path. It's against logic to keep this behavior in future drupal releases.

Dave Reid’s picture

You've made your point pounard. Create a new issue to rewrite the autocompletion process and submit a patch.

Dave Reid’s picture

It appears that %menu_tail will not actually load the value into a callback argument. It's basically ignored. How frustrating that it could be so useful, but can't cut it in this case. :/

See:
#298561: menu_tail should automatically act as a load function as well to allow slashes in search
#157510: Introduce "Tail arguments" in menu paths
http://drupal.org/node/357959
http://drupal.org/node/109153#to_arg

merlinofchaos’s picture

#600424: search.module pulls arguments directly from $_GET rather than using the menu system contains a patch to make %menu_tail function properly as part of fixing search.module -- when it goes in, these autocompletes can all be fixed properly by utilizing %menu_tail

Dave Reid’s picture

Component: base system » path.module
grendzy’s picture

Is there a reason not to just use regular GET parameters for autocomplete paths?

/foo/autocomplete?search=bar%2Fbaz works fine for me...

Dave Reid’s picture

Hrm, there appears to be a core bug with the autocomplete.js as well because this causes a 404 error when using an autocomplete string like 'content/[node:'. The only menu callback I have defined is 'token/autocomplete/node' and the complete autocompletion URL request being generated is 'http://localhost/drupal7dev/token/autocomplete/node/content%2F%5Bnode%3A'

das-peter’s picture

FileSize
1.38 KB
FAILED: [[SimpleTest]]: [MySQL] Invalid patch format in autocomplete-optional-double-encoding.patch. View

Just came across this issue :|
And I've to find a solution - thus I decided to create a patch which introduces the double encoding approach suggested in #93854-19: Allow autocompletion requests to include slashes on an optional base.
If you add, besides #autocomplete_path, the setting #autocomplete_double_encode to your element, the search string is encoded twice.

Of course you have to do an extra decode in your callback function, but since you've set the extra parameter you should be aware of that.
This patch is just a workaround. Hope a general solution will come :)

thinkyhead’s picture

Issue tags: +taxonomy, +slash

I just hit this problem on a Drupal 6 site that really needs to be able to have slashes in their taxonomy terms and an autocomplete field. I was about ready to just hack core and be done with it, but I decided it's better to delve and see if a general solution can be found.

For the record, here's the autocomplete flow as of Drupal 6.19...

  • You type a string into an autocomplete field
  • The field is handled by Drupal.ACDB.prototype.search() [autocomplete.js]
  • The string is encoded by Drupal.encodeURIComponent() [drupal.js] like so:
    • Encode the string with encodeURIComponent
    • Turn encoded slashes back to regular slashes
    • If your site has Clean URLs then:
      • Double-encode & and #, and finally...
      • Encode '//' as '/%252F'
  • Drupal.ACDB.prototype.search uses the result as the AJAX request parameter

So the problem is pretty obvious. The slash character always goes through unencoded, and this looks like a further path extension to the Clean URL system. The autocomplete function only gets the string before the first slash, and will return results as if that was all you had typed in the field.

Other solutions here have suggested going around the menu system by using a POST parameter. Probably a POST would be best. Then the string doesn't need to be encoded at all. I actually considered using a COOKIE or SESSION but I don't recall offhand if Drupal is fully bootstrapped when autocomplete functions run.

For my interim solution I decided to take a different approach - just get rid of the offending slash! For this I first use my site's custom module to add some Javascript that replaces Drupal's encoding function with my own:

Drupal.encodeURIComponent = function (item, uri) {
  uri = uri || location.href;
  item = encodeURIComponent(item).replace(/%2F/g, '%1B');
  return (uri.indexOf('?q=') != -1) ? item : item.replace(/%26/g, '%2526').replace(/%23/g, '%2523').replace(/\/\//g, '/%252F');
};

Now slashes will be encoded as escape characters, something which users can't type in and which are very rarely used. Any non-printing, un-typeable, or rare string (e.g., "~~~~") should do. Then all I have to do is add one line to the beginning of my autocomplete functions to make it all work:

function my_autocomplete($vid, $string = '') {
  $string = str_replace(chr(27), '/', $string);
  // call a core autocomplete function here or implement my own
}

In my case for testing I just hacked the core taxonomy.pages.inc file (and the equivalent function in the taxonomy_manager module). For the final version I'll use hook_form_alter in my site's custom module to point the autocomplete fields at my autocomplete functions instead of the built-in ones.

This might seem like a hackerly approach, but frankly for 99.9% of sites this would be a perfectly viable solution, and I don't think it's too "out there" to be considered in-principle for core. There's no reason the autocomplete.js functions need to use Drupal.encodeURIComponent for their particular job. They might as well have their own encoder for this situation, and then the Form API can handle the special case of decoding for the autocomplete fields it implements.

But hmm, if you can do a POST through ajax I don't know why you'd want to use anything else. Is a different approach being used for D7?

thinkyhead’s picture

The POST Approach

I also just tried the POST solution outlined in comment #7 and it works great, no pre-encoding required. Even ampersand works fine, as jQuery does the right thing. An earlier comment here says there are caveats with using POST as a general solution, but it seems like it should definitely be an option.

The only step to make the POST method work was to get the value of $_POST['search'] within the body of all the autocomplete functions. This just isn't viable, but fortunately it's also completely unnecessary. Once again proving how cool Drupal is, yes you can do it... without hacking core!

The Little Module that Could

The module outlined below modifies the menu router so that all 'autocomplete' paths are re-routed to our function, haha! Now we can append the contents of $_POST['search'] to the parameters and call the original autocomplete handler. The code below has been tested on a production site with thousands of tags, lots of them containing slashes, and it works great!

/**
 * Implementation of hook_init().
 *
 * Override Drupal's autocomplete to use ajax.POST
 * the autocomplete.js in the module's folder should have...
 * 
 * Drupal.behaviors.autofix = function(c) {
 *   Drupal.ACDB.prototype.search = function (searchString) {
 *     ... code that uses ajax POST as shown above in comment #7
 *   }
 * };
 *
 */
function autofix_init() {
  drupal_add_js(drupal_get_path('module', 'autofix') ."/autocomplete.js");
}

/**
 * Implementation of hook_menu_alter().
 *
 * Override anything with '/autocomplete' in the path
 * A fuller implementation might handle special cases.
 */
function autofix_menu_alter(&$items) {
  foreach ($items as $k=>$foo) {
    if (strpos($k, '/autocomplete') > -1) {
      $auto =& $items[$k];
      $args = $auto['page arguments'];
      $auto['page arguments'] = array($auto['page callback']);
      if ($args) $auto['page arguments'] += (is_array($args) ? $args : array($args));
      $auto['page callback'] = '_autofix_autocomplete';
    }
  }
}

/**
 * This handler takes the original function as its argument
 * then does what menu_execute_active_handler() does.
 */
function _autofix_autocomplete($func) {
  if ($router_item = menu_get_item($_GET['q'])) {
    if ($router_item['access']) {
      $ri = array_slice($router_item['page_arguments'], 1);
      $ri[] = $_POST['search'];
      call_user_func_array($func, $ri);
    }
  }
}

Autocomplete Fix Modules

For your convenience, and mine, I've posted a pair of modules to allow you to use slashes in autocomplete fields. Autocomplete Hack encodes slashes as funky strings, while Autocomplete POST makes autocomplete fields use the POST method. I'll package them up for a release if this issue isn't fixed in Drupal core soon. Meanwhile you can download them at
http://www.thinkyhead.com/design/autocomplete-fix

nkschaefer’s picture

slurslee, you are awesome and saved me a ton of time. The autocomplete field that was giving me trouble was a CCK Autocomplete Widgets field -- the above methods didn't work as-is, but I got the $_POST method working after a couple of minor tweaks:

I wrote a hook_form_alter that fires when editing node forms for my content type. In this hook, I added autocomplete_post.js from slurslee's autocomplete_post module (I don't find it necessary to override every autocomplete field for my site, just the one causing problems for now).

I wrote a hook_menu_alter that changes the page callback for autocomplete_widgets to my own function. This function is an exact copy of autocomplete_widgets_json, with an added line at the top that checks for $_POST['search'] and sets $string = $_POST['search'] if it exists.

Note: if you have any other autocomplete fields on the same form/page (where you're adding that JS file), you'll need to override those autocomplete callbacks and similarly check for $_POST['search'] at the beginning of each.

Sorry if this is insultingly obvious to everyone, but I thought I'd share in case it saves anyone time, just as the above posts saved me a headache. And I also like the idea of the $_POST method being incorporated into core, since it's no fun to have to find and fix individual edge cases like this.

vitok-dupe’s picture

subscribe

mr.baileys’s picture

pillarsdotnet’s picture

droplet’s picture

+1

anon’s picture

I cant sleep until this issue is fixed

pillarsdotnet’s picture

Funeral notice:
anon, that poor soul, will have died of sleep deprivation by Valentine's Day. Send patches, not flowers.
anon’s picture

pillarsdotnet, why do you have to attack me? Just said that I would love to see this issue fixed.
And where is your patch?

Steven Jones’s picture

Assigned: Unassigned » Steven Jones

The menu_tail_load function went into Drupal 7, so it should be fairly easy to fix the core uses of autocomplete that aren't working.

anon’s picture

Any progress with this Steven Jones?

Steven Jones’s picture

Not yet, but I will be able to look at it in a few hours :)

Steven Jones’s picture

Status: Needs work » Needs review
FileSize
1.34 KB
FAILED: [[SimpleTest]]: [MySQL] 31,542 pass(es), 1 fail(s), and 0 exception(es). View

Okay, so here's a test for the edge case of a term containing a slash. testbot should fail on this.

Steven Jones’s picture

FileSize
2.27 KB
PASSED: [[SimpleTest]]: [MySQL] 31,541 pass(es). View

Right and now here's a slightly ugly fix.

Note that still doesn't actually work, because the JS on the client escapes the slash :(
Simpletest can't test the JS yet, so this will need work.

Steven Jones’s picture

Status: Needs review » Needs work

So the test correctly catches the failure, and the fix for taxonomy, now need to fix the JS.

Steven Jones’s picture

Title: Add drupal_get_path_segment() and fix autocompletion strings with slashes » Fix autocompletion strings with slashes

Right, looking into this it seems that we now fail because apache will 404 a URL with %2F (an escaped '/') so our autocomplete callback never gets a chance. The solution might be to change the %2F back to a '/' by using Drupal.encodePath?

Steven Jones’s picture

Status: Needs work » Needs review

But apparently that was fixed in #284899: Drupal url problem with clean urls so the above patch should be sufficient for taxonomy, can anyone else test?

Steven Jones’s picture

Component: path.module » taxonomy.module
Assigned: Steven Jones » Unassigned
FileSize
2.81 KB
PASSED: [[SimpleTest]]: [MySQL] 31,540 pass(es). View

Reading through #995512: Autocomplete doesn't work with slashes would seem to suggest that this bug was caused by #284899: Drupal url problem with clean urls not fixed by it. So here's a patch to fix this issue for taxonomy.

Steven Jones’s picture

Title: Fix autocompletion strings with slashes » Allow autocompletion requests with slashes
Component: taxonomy.module » path.module
Assigned: Unassigned » Steven Jones
Status: Needs review » Needs work

Better title (I think).

Steven Jones’s picture

Title: Allow autocompletion requests with slashes » Allow autocompletion requests to include slashes
Component: path.module » taxonomy.module
Assigned: Steven Jones » Unassigned
Status: Needs work » Needs review

Ah, cross post with myself!

vitok-dupe’s picture

Ah, cross post with myself!

You good! it's work thx! =)

skov’s picture

Tested patch in #60 and it works with new and existing autocomplete taxonomy terms. Thanks!

pillarsdotnet’s picture

FileSize
2.75 KB
PASSED: [[SimpleTest]]: [MySQL] 31,547 pass(es). View

I am told that we never link to drupal issues in core. So, re-rolled patch in #60 accordingly.

That said, I can testify that this patch works, and the code looks good.

Steven Jones’s picture

#65: taxonomy_autocomplete.patch queued for re-testing.

Steven Jones’s picture

Issue summary:

This issue is about allowing auto-completion of taxonomy terms that include slashes in the search text. The autocompletion is mostly commonly used when adding terms to a tagging field on a node.

To test:

Automated tests exist for testing the backend of the autocompletion, but we can't test the front-end JS, so lots of testing in different browsers and servers would be useful.

axel.rutz’s picture

subscribe

c960657’s picture

+  // This path is also required for autocomplete to work.
   $items['taxonomy/autocomplete'] = array(

I suggest that we kill the menu hook. It is only necessary because of the call to drupal_valid_path() in theme_textfield(). I think we should fix that at the source instead. When doing autocomplete it is not necessary that the path specified by #autocomplete_path itself is a valid callback, as long as it becomes a valid path when you append something to it, so I suggest something like this:

@@ -3636,7 +3636,7 @@ function theme_textfield($variables) {
   $extra = '';
-  if ($element['#autocomplete_path'] && drupal_valid_path($element['#autocomplete_path'])) {
+  if ($element['#autocomplete_path'] && drupal_valid_path($element['#autocomplete_path'] . '/foo')) {
     drupal_add_library('system', 'drupal.autocomplete');
     $element['#attributes']['class'][] = 'form-autocomplete';
 
Steven Jones’s picture

Version: 7.x-dev » 8.x-dev
FileSize
4.15 KB
PASSED: [[SimpleTest]]: [MySQL] 29,405 pass(es). View

Killing the menu hook seems like a good idea.

Patch attached for the 8.x branch, it should backport fairly easily.

Steven Jones’s picture

Issue tags: +needs backport to D7

Updating tags.

pillarsdotnet’s picture

You can help this issue by posting a review:

  • Decide whether all code and comments comply with Drupal coding standards.

  • Decide whether any included tests are necessary and sufficient.

  • Decide whether the patch actually solves the problem. There should be a reproducible test where the current code fails and the patched code succeeds.

A review that affirms success in all of the above should also:

  • Change the issue status from "Needs Review" to "Reviewed and Tested By the Community".

Otherwise, the review should:

  • Explain what is wrong with the proposed patch and/or supply a corrections to it.

  • Change the issue status from "Needs Review" to "Needs Work".

c960657’s picture

Status: Needs review » Needs work

This looks good.

A few nits:

+    // Needed for menu_tail_load().
+    'load arguments' => array('%map', '%index'),

We generally don't add comments like that in core.

+    // Try to autocomplete the term name, that contains a slash.
+    // We should only get a single term returned.

AFAIK, comma is not allowed in front of “that”.

The test will fail in the rare event that the two randomString() calls begin with the same two letters. Could you make the prefix longer so that a collision is more unlikely (it seems we generally allow tests to fail due to random collisions, as long as these are just very rare).

Perhaps it would be slightly more readable if the two terms were called $term1 and $term2 instead of reusing the $term variable.

pillarsdotnet’s picture

Status: Needs work » Needs review
FileSize
18.44 KB
PASSED: [[SimpleTest]]: [MySQL] 29,890 pass(es). View

Re-rolled with suggestions and some additional changes to avoid unnecessarily long code lines.

pillarsdotnet’s picture

FileSize
2.5 KB
4.35 KB
PASSED: [[SimpleTest]]: [MySQL] 29,764 pass(es). View

Bah! Sorry about the mixup. Here's the patch I meant to upload, and an interdiff from #70.

pillarsdotnet’s picture

FileSize
4.41 KB
PASSED: [[SimpleTest]]: [MySQL] 29,759 pass(es). View

The test will fail in the rare event that the two randomString() calls begin with the same two letters. Could you make the prefix longer so that a collision is more unlikely (it seems we generally allow tests to fail due to random collisions, as long as these are just very rare).

Sorry; missed that bit. Re-rolled accordingly.

swentel’s picture

So this fixes the autocomplete for taxonomy, but not autcomplete for *any* other autocomplete out there. Is this the way to go. Didn't read the whole issue for now, but it seems either we fix this for any autocomplete or add good documentation for module developers to fix this for their modules.

Steven Jones’s picture

If you provide a list of the other uses of autocomplete in core, we can get those fixed too.

swentel’s picture

Well, it looks like the autocomplete of the author on a node form isn't affected by this, but I'm thinking of contrib modules, at this point #1024460: Slashes doesnt work with autocomplete (waiting for D7 update to fix this) is affected (but I'm looking for a patch to make it work there). I'll go through the other core autocompletes (although I think those are the only ones right now) and report back if I find any.

Steven Jones’s picture

There are a couple of changes to core's javascript that allow this to work, but existing modules should work and can take advantage of being able to use proper menu arguments if they want.

Is there some documentation for module developers about implementing autocomplete callbacks somewhere?

c960657’s picture

Status: Needs review » Needs work

I guess the %menu_tail stuff is the official way of doing this. As for module developers, they should basically do what the Taxonomy module is about to do once this patch gets in.

You could argue that this is overly complicated (I have only looked at it briefly, so I don't know whether it is complex for a good reason), but I suggest we discuss this separately (changing this is D8 material, so let's not have it block this bug that affects D7 also).

Just one last nit:

+    $message = t(
+      'Autocomplete returns term %term_name after typing the first %num letters, including a slash in the name.',
+      array('%term_name' => $search_term->name, '%num' => $base_len + 3)
+    );
+    $this->assertRaw(drupal_json_encode($target), $message);

Even though this formatting is easy to read, the convention is write it all on one line and skip the $message variable.

pillarsdotnet’s picture

Actually, the (new) convention is to remove t() from assertion tests. See #500866: [META] remove t() from assert message

Steven Jones’s picture

@pillarsdotnet actually if we're using the placeholder functionality of t then I think we're fine to use it.

@c960657 I don't think we're making it more complicated, we're just defining and using arguments to a page callback instead of using the 'magic' extra ones that the menu system sends. Looking over the code that's being changed here, I actually think we're making it simpler, as people looking at this code and learning from it will be able to see where all the stuff comes from.

Steven Jones’s picture

Status: Needs work » Needs review
FileSize
4.36 KB
PASSED: [[SimpleTest]]: [MySQL] 33,598 pass(es). View

Attached patch is #76 plus a fix for:

Even though this formatting is easy to read, the convention is write it all on one line and skip the $message variable.

coolestdude1’s picture

Looking forward to the D7 back port. Sub-ing

grimbones’s picture

I would like to see this backported to D7 as well and maybe back to D6.

c960657’s picture

Status: Needs review » Reviewed & tested by the community
rv0’s picture

sub + D7 backport

webchick’s picture

Status: Reviewed & tested by the community » Needs review
-  if ($element['#autocomplete_path'] && drupal_valid_path($element['#autocomplete_path'])) {
+  if ($element['#autocomplete_path'] && drupal_valid_path($element['#autocomplete_path'] . '/dummy')) {

This is incredibly strange. Previous versions of the patch didn't have this. Is it possible to remove it? I don't like adding this kind of stuff to form.inc, and especially in a theme function that might be overridden and thus functionality still broken.

Thanks a ton for the automated tests though. YAY! Test-driven development. :D

Steven Jones’s picture

It's needed because $element['#autocomplete_path'] is no longer a valid path, see comment 69. It needs to be autocompleting something to become a valid path.

c960657’s picture

One could argue that there is no reason to check whether it is a valid path. It is the responsibility of the module developer to supply a valid path. In fact, I think muting this error during development makes debugging harder. So unless I am missing a use-case, I am in favour of removing the check altogether in D8.

Steven Jones’s picture

axel.rutz’s picture

i support #91: errors should be errors

i spent too much hours with errors that are silently ignored...

indigoparadox’s picture

Subscribing in eager anticipation of a Drupal 7 backport.

longwave’s picture

Subscribing. This bug makes it impossible to use textfields to autocomplete a file path, which Ubercart used successfully in D6 but that no longer work in D7.

I also don't see why the line noted in #89 shouldn't be changed to just:

if ($element['#autocomplete_path']) {
tarmstrong’s picture

I've tested Steven Jones's patch at #84 on Drupal 7. It fixes the broken autocompletion with slashes in the allowed values I had experienced before.

pillarsdotnet’s picture

FileSize
4.29 KB
FAILED: [[SimpleTest]]: [MySQL] 33,651 pass(es), 1 fail(s), and 0 exception(es). View

Re-rolled with change suggested in #89/91/93/95

Status: Needs review » Needs work

The last submitted patch, fix_autocompletion_with_slashes-93854-96.patch, failed testing.

pillarsdotnet’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
4.36 KB
PASSED: [[SimpleTest]]: [MySQL] 33,649 pass(es). View

@webchick -- There's your answer. The change you objected to is necessary in order to pass the profile module tests. Back to RTBC as before.

e2thex’s picture

FileSize
4.33 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch fix_autocompletion_with_slashes-93854-99-make.patch. See the log in the details link for more information. View

Attch patch is same as in #99 but works with drush make

Status: Reviewed & tested by the community » Needs work

The last submitted patch, fix_autocompletion_with_slashes-93854-99-make.patch, failed testing.

pillarsdotnet’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
4.36 KB
PASSED: [[SimpleTest]]: [MySQL] 33,662 pass(es). View

(sigh) Re-uploading the patch in #99.

modstore’s picture

subscribing.

webchick’s picture

Status: Reviewed & tested by the community » Needs review

We need sign-off from sun/chx on this. This is really, really weird.

webchick’s picture

Component: taxonomy.module » forms system
grendzy’s picture

It looks like drupal_valid_path would accept $element['#autocomplete_path'] . '/%', instead of 'dummy'. Thoughts?

axel.rutz’s picture

#97: fix_autocompletion_with_slashes-93854-96.patch queued for re-testing.

(still can't imagine how this fails.

pillarsdotnet’s picture

FileSize
4.35 KB
PASSED: [[SimpleTest]]: [MySQL] 33,645 pass(es). View

@#106 -- okay; testing.

axel.rutz’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
4.19 KB
PASSED: [[SimpleTest]]: [MySQL] 33,662 pass(es). View

@webchick: as for your WTF in #89 about the line

-  if ($element['#autocomplete_path'] && drupal_valid_path($element['#autocomplete_path'])) {
+  if ($element['#autocomplete_path'] && drupal_valid_path($element['#autocomplete_path'] . '/dummy')) {
// attach autocomplete stuff here

i researched a bit and grokked it, yay:
* #91 and some others like me thought "drupal_valid_path only checks for a valid path. let's drop that as it is the responsibility of the module developer to supply a valid path"
* so we had patch #97 with failing tests
* why did the tests fail? profile.module runs a test that a normal user without autocomplete permissions should NOT see an autocomplete widget
* so the clue is: drupal_valid_path NOT only checks for a valid path, but: if it is a valid path AND the current user has access permission

==> so i think the permission check is a good thing and we need the drupal_valid_path thing with a dummy autocomplete argument.

now for #106: YES, drupal_valid_path may take wildcard paths BUT only with signature drupal_valid_path($path, $dynamic_allowed = TRUE), AND then it only checks agains the menu_router table.
which means it will not recognize a path like profile/autocomplete/myfieldid/% as the router_table contains profile/autocomplete/%/% so above mentioned test will fail too.
(EDIT: the $dynamic_allowed parameter is buggy anyway, see #1256978: drupal_valid_path: fix&document OR refactor )

==> so the clearest thing imho is to use the original patch in #84 which was reviewed by several people, and the only objection was #89 about drupal_valid_path. which i hope could clarify.
just to be sure i re-rolled it with a clarifying comment.

so finally rtbc with this? it would be so nice to let this RIP.

xjm’s picture

Status: Reviewed & tested by the community » Needs review
Issue tags: +Needs issue summary update

#109: Lots of work has been done since #89. See #108.

Marking NR for #108.

+++ b/includes/form.incundefined
@@ -3661,7 +3661,7 @@ function theme_textfield($variables) {
-  if ($element['#autocomplete_path'] && drupal_valid_path($element['#autocomplete_path'])) {
+  if ($element['#autocomplete_path'] && drupal_valid_path($element['#autocomplete_path'] . '/%')) {

This certainly looks more reasonable, but could we get a comment explaining it? Also, I still don't think we've addressed webchick's remark about the fact that this check is being done in a theme function?

+++ b/modules/taxonomy/taxonomy.testundefined
@@ -641,6 +641,31 @@ class TaxonomyTermTestCase extends TaxonomyWebTestCase {
+   * Test term autocompletion edge cases.

Minor nitpick but I learned yesterday this should say Tests... Also, we're not testing multiple cases, so maybe "Tests term autocompletion for term names that contain a slash."?

Edit: Also, webchick has asked for a sign-off by one of the forms maintainers before this is marked RTBC.

axel.rutz’s picture

>#109: Lots of work has been done since #89. See #108.
i reverted this and did explain why (especially why "%" makes no sense here).

i quite spent some hours researching this so i kindly ask to spend a minute to read and understand my comment thoroughly.

>I still don't think we've addressed webchick's remark about the fact that this check is being done in a theme function?
right, good to remind about this.

marcb’s picture

Version: 8.x-dev » 7.8
Assigned: Unassigned » marcb
Category: bug » support

Using drupal 7.8
i have a $form['myform'], '#type' => 'textfield', '#autocomplete_path' => 'my path/'.$my_arg,
I also use '#ajax' , 'callback' => my_callback', 'wrapper' => 'my_wrapper' in 'myform'.

The user get the 404 error when typing : '/', '.', '\'. and maybe others.

Is there a way to filter or replace the characters before they get process by autocomplete?(without modifying form.inc and autocomplete.js)

pillarsdotnet’s picture

Version: 7.8 » 8.x-dev
Assigned: marcb » Unassigned
Category: support » bug

Please do not hijack a bug report with a support request. By doing so, you delay the fix from being accepted so that it is available for everybody.

Please do not assign yourself to issues unless you are taking responsibility for resolving the problems reported in that issue. That is what the "assigned" block is for.

Please do not change the Version to a lower number unless the problem has been fixed in the higher-numbered version. In Drupal, we always fix the latest development version first, then backport the fixes to earlier versions.

Thank you.

eric.duran7@gmail.com’s picture

Can we get a summary on this issue?

eric.duran7@gmail.com’s picture

I'm confused as to why everything else is happening here.

I did a quick test and by doing:

     $.ajax({
       type: 'GET',
-      url: db.uri + '/' + encodeURIComponent(searchString),
+      url: db.uri + '/' + Drupal.encodePath(searchString),
       dataType: 'json',

This fix the slash issue.

Can someone explain to me, what the rest of the patch is attempting to do.

Thanks.

pillarsdotnet’s picture

FileSize
2.68 KB
FAILED: [[SimpleTest]]: [MySQL] 33,460 pass(es), 1 fail(s), and 0 exception(es). View

The patch in #108:

  • Changes misc/javascript.js to enable autocomplete term to contain slashes.

  • Adds a test to ensure that the change to misc/javascript.js actually worked.

  • Changes taxonomy_menu() to define taxonomy/autocomplete/%/%menu_tail instead of taxonomy/autocomplete, since no valid autocompletion request would ever equal taxonomy/autocomplete.

  • Changes theme_textfield() to allow the taxonomy change to work.

Dropping the (arguably unrelated) changes to form.inc and taxonomy.module, we have the following patch:

webchick’s picture

Yay, that patch looks MUCH better!!

eric.duran7@gmail.com’s picture

Status: Needs review » Needs work

Yea, this looks better, but now I see why this doesn't actually work.

We need the %menu_tail.

What actually ends up happening is that any request after the slash isn't really added to the search term. So lets say we where looking for a date such as 10/16/2011 (which existed on the database).

Once you type 10/ everything after the slash is ignore in the search param.

eric.duran7@gmail.com’s picture

Status: Needs review » Needs work
FileSize
18.92 KB
PASSED: [[SimpleTest]]: [MySQL] 33,445 pass(es). View

Here's a different patch.

This actually keeps the auto-complete path as is. All this does is adds a new one with the menu_tail so the proper params are taking into account. This allows us to keep the autocomplete_path to be taxonomy/autocomplete yet, let it properly filter anything with a slash in it.

I left the test as is. But by the looks of it the test falls short.

What the test need to do it :

1. Create a term such as 10/16/2011
2. Test that a request such as 10/16 returns the correct value.
3. Test that the request such as 10/17 doesn't return anything.

EDIT: IGNORE THIS PATCH.

eric.duran7@gmail.com’s picture

FileSize
3.15 KB
PASSED: [[SimpleTest]]: [MySQL] 33,457 pass(es). View

Lol, wrong patch, --ignore that.

I meant to upload this one.

eric.duran7@gmail.com’s picture

Status: Needs work » Needs review
pillarsdotnet’s picture

FileSize
4.75 KB
PASSED: [[SimpleTest]]: [MySQL] 33,470 pass(es). View

Re-uploading the patch from #108, with a clarifying comments added.

eric.duran7@gmail.com’s picture

@pillarsdotnet, thats not right,

+++ b/includes/form.incundefined
@@ -3686,7 +3686,8 @@ function theme_textfield($variables) {
-  if ($element['#autocomplete_path'] && drupal_valid_path($element['#autocomplete_path'])) {
+  // We append '/placeholder' to represent an autocomplete search term.
+  if ($element['#autocomplete_path'] && drupal_valid_path($element['#autocomplete_path'] . '/placeholder')) {

The above code isn't correct. This pretty much breaks autocomplete everywhere.

pillarsdotnet’s picture

Here's an interdiff between #108 and #122. Judge for yourself what changed.

diff -u b/includes/form.inc b/includes/form.inc
--- b/includes/form.inc
+++ b/includes/form.inc
@@ -3686,7 +3686,8 @@
   _form_set_class($element, array('form-text'));
 
   $extra = '';
-  if ($element['#autocomplete_path'] && drupal_valid_path($element['#autocomplete_path'] . '/%')) {
+  // We append '/placeholder' to represent an autocomplete search term.
+  if ($element['#autocomplete_path'] && drupal_valid_path($element['#autocomplete_path'] . '/placeholder')) {
     drupal_add_library('system', 'drupal.autocomplete');
     $element['#attributes']['class'][] = 'form-autocomplete';
 
diff -u b/misc/autocomplete.js b/misc/autocomplete.js
--- b/misc/autocomplete.js
+++ b/misc/autocomplete.js
@@ -287,7 +287,8 @@
   this.timer = setTimeout(function () {
     db.owner.setStatus('begin');
 
-    // Ajax GET request for autocompletion.
+    // Ajax GET request for autocompletion. We use Drupal.encodePath instead of
+    // encodeURIComponent to allow autocomplete search terms to contain slashes.
     $.ajax({
       type: 'GET',
       url: db.uri + '/' + Drupal.encodePath(searchString),
diff -u b/modules/taxonomy/taxonomy.module b/modules/taxonomy/taxonomy.module
--- b/modules/taxonomy/taxonomy.module
+++ b/modules/taxonomy/taxonomy.module
@@ -312,6 +312,7 @@
     'type' => MENU_CALLBACK,
     'file' => 'taxonomy.pages.inc',
   );
+  // We append /%/%menu_tail to allow autocompletion terms to contain slashes.
   $items['taxonomy/autocomplete/%/%menu_tail'] = array(
     'title' => 'Autocomplete taxonomy',
     'page callback' => 'taxonomy_autocomplete',
pillarsdotnet’s picture

Issue summary: View changes

Updated summary from template.

pillarsdotnet’s picture

Issue summary: View changes

Include link to the correct patch.

eric.duran7@gmail.com’s picture

Status: Needs work » Needs review

I get the difference, but #108 is also incorrect.

eric.duran7@gmail.com’s picture

FileSize
3.18 KB
PASSED: [[SimpleTest]]: [MySQL] 33,450 pass(es). View

Here's a patch,

This is very similar to the other patches but also different.

Here's what this patch does:

  • Adds a new menu_path with %menu_tail to make sure the slashes are included in the filter result set
  • Does not modified theme_textfield, that should be left as is
  • Uses Drupal.encodePath to make sure the slashes are encoded properly
  • Adds 2 extra test to make sure the proper results set is return when slashes are added to the auto-complete path.
pillarsdotnet’s picture

FileSize
4.23 KB
PASSED: [[SimpleTest]]: [MySQL] 33,458 pass(es). View

Okay, if it works, but leave the extra comments in, at least.

pillarsdotnet’s picture

Issue summary: View changes

Correction to comment number.

pillarsdotnet’s picture

Issue summary: View changes

Updated links to latest patch.

pillarsdotnet’s picture

Issue summary: View changes

Re-order for clarity.

Steven Jones’s picture

Status: Needs review » Needs work

Happy days, we're basically back at #60, from 8 months ago! But hopefully this time more people understand why it's all needed. But thanks for rolling patches and working on this. Right, on to the review:

+++ b/modules/taxonomy/taxonomy.module
@@ -319,6 +319,16 @@ function taxonomy_menu() {
+  // We append /%/%menu_tail to allow autocompletion terms to contain slashes.

We actually just append '%menu_tail' to deal with the slashes, the previous '%' is for the vocabulary machine name. Can we tidy up that comment.

Could we add a comment to the other menu item in the taxonomy_menu for autocomplete so that we know why it is there? Otherwise it looks a little random to have two menu items for the autocomplete. Actually thinking about this, I think that we can add an additional '%' to that menu item, as the access is checked including the vocab machine name, never without it.

Looking at taxonomy_autocomplete I notice that we never sort the return result, so the test will break if we ever decide to sort on something that returns the results differently from the way that they're being checked for in the test. I wonder if we need to get around this by only every testing for things that will return a single result?

-16 days to next Drupal core point release.

pillarsdotnet’s picture

Status: Needs work » Needs review
FileSize
4.82 KB
PASSED: [[SimpleTest]]: [MySQL] 33,456 pass(es). View

Better yet, check the test results in a way that does not depend on a specific result order.

xjm’s picture

Issue tags: -taxonomy, -autocomplete, -path.inc, -slash

Tag cleanup.

pillarsdotnet’s picture

FileSize
4.65 KB

@#128 by Steven Jones on October 18, 2011 at 8:30am:

Happy days, we're basically back at #60, from 8 months ago!

Pretty close. Here's the interdiff.

pillarsdotnet’s picture

FileSize
4.35 KB
4.77 KB
PASSED: [[SimpleTest]]: [MySQL] 33,445 pass(es). View

Re-arranged and added missing comments to make the interdiff smaller.

xjm’s picture

Status: Needs review » Needs work
Issue tags: +Novice

Tagging as novice for the task of rerolling the Drupal 8.x patch on account of #22336: Move all core Drupal files under a /core folder to improve usability and upgrades.

If you need help rerolling this patch, you can come to core office hours or ask in #drupal-gitsupport on IRC.

Also, a few minor comment corrections that I'd suggest fixing in the new patch:

+++ b/modules/taxonomy/taxonomy.testundefined
@@ -641,6 +641,41 @@ class TaxonomyTermTestCase extends TaxonomyWebTestCase {
+   * Test term autocompletion edge cases with slashes in the names.

This should begin, "Tests term autocompletion..." (We are currently cleaning this up in #1310084: [meta] API documentation cleanup sprint.)

+++ b/modules/taxonomy/taxonomy.testundefined
@@ -641,6 +641,41 @@ class TaxonomyTermTestCase extends TaxonomyWebTestCase {
+    // Try to autocomplete a term name that matches 1st term.

It would probably be better to spell out "First" here.

mstrelan’s picture

Status: Needs work » Needs review
FileSize
4.08 KB
PASSED: [[SimpleTest]]: [MySQL] 33,849 pass(es). View

Re-roll of #132 to place files in the core directory and incorporate changes from #133.

xjm’s picture

Issue tags: -Novice
FileSize
2.08 KB
FAILED: [[SimpleTest]]: [MySQL] 33,857 pass(es), 2 fail(s), and 0 exception(es). View

Thanks for the reroll; it looks great.

Here's the latest version of the test in a patch by itself, to demonstrate the expected test failure without the fix.

Status: Needs review » Needs work

The last submitted patch, autocomplete_slashes_test_only.patch, failed testing.

xjm’s picture

Status: Needs work » Needs review

Test failed as expected.

xjm’s picture

Issue tags: +Novice, +Needs manual testing

Now let's have one or two other people test the patch from #134 functionally and report the results. Some things you might try:

  1. Create an article and enter a tag containing a slash. Save the article and verify the term is created correctly.
  2. Create a second article and try to use the autocomplete to add the term you added to the first article. Save the article and check at admin/structure/taxonomy/tags/add to make sure it used the same tag rather than creating a new one.
  3. Add some other terms containing slashes to the tags vocabulary at admin/structure/taxonomy/tags/add. Try some different patterns: 11/1/2011, 11/2/2011, import/export, "Term name containing a comma, plus / slashes / too.", This term's got / slashes and apostrophes. Etc.
  4. Test autocompletion and saving of these terms as well.

Screenshots are always helpful as well. Be sure to crop them only to the relevant portion.

Adding novice tag for some manual review and testing.

xjm’s picture

Issue summary: View changes

wordsmithing.

xjm’s picture

Updated summary.

mstef’s picture

#134 works for me.

eric.duran7@gmail.com’s picture

This works perfectly for me. Not really sure I should be marking it RTBC being that I contributed to the patch, but feel free to change it back otherwise.

Here are some screenshots of my test.

Screen Shot 2011-11-16 at 8.03.18 PM.png

Screen Shot 2011-11-16 at 8.03.08 PM.png

Screen Shot 2011-11-16 at 8.00.34 PM.png

Screen Shot 2011-11-16 at 8.00.21 PM.png

xjm’s picture

Issue tags: -Novice, -Needs manual testing

Thanks @ericduran! The screenshots are a big help. This feels RTBC to me too. :)

chx’s picture

%menu_tail yay. looks good.

thinkyhead’s picture

Does this now add up to a good general solution that's back-portable to D6? I'm still relying on my autocomplete_post module to take over autocompletion on my D6 sites.

drupalisme’s picture

Need back ported to D6, see http://drupal.org/node/1088948

Steven Jones’s picture

Issue tags: +needs backport to D6

Using %menu_tail is the way to go for sure. It won't be an easy backport though.

What is holding this up from getting committed into 8.x?

xjm’s picture

#146: Nothing. It's marked RTBC.

catch’s picture

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

Looks good here, committed/pushed to 8.x. Will need a re-roll for 7.x.

xjm’s picture

FileSize
4.02 KB
PASSED: [[SimpleTest]]: [MySQL] 37,070 pass(es). View

Here's a D7 reroll. I also tested with this patch applied to 7.9 with the steps in #138 to make sure the backport was okay (wasn't sure if the remarks above were only about D6 or not). It still properly autocompletes terms and search strings that include slashes.

xjm’s picture

Status: Patch (to be ported) » Reviewed & tested by the community
damien_vancouver’s picture

Patch from #149 applies cleanly, fixes the issue for me on 7.9. +1

linclark’s picture

Status: Reviewed & tested by the community » Needs work

I might be missing something, but this doesn't seem to work with comma separated autocomplete fields, such as Tags. The first string comes in as input for the autocomplete, but the second one comes in as 'http:'.

If I'm on crazy pills, please feel free to switch back.

Steven Jones’s picture

@linclark that's exactly what this issue should fix. What browser are you seeing this error with?

xjm’s picture

Yeah, I can't reproduce #152 with D8. Could you provide exact steps to reproduce?

Edit: I can reproduce this in D7 with the patch above applied if I don't clear my browser cache. Can you reproduce the issue after clearing your browser cache?

linclark’s picture

Status: Needs work » Reviewed & tested by the community

OK, I think there were a couple of issues that were compounding to make this not work for me (including me using the API incorrectly). It is now working for me on D7 with an autocomplete path in Microdata module.

Dave Reid’s picture

So now if you want to add an autocomplete callback in your own module, you have to define two different menu callbacks? That's a little odd.

xjm’s picture

Well, it's the same callback for both paths.

Steven Jones’s picture

So now if you want to add an autocomplete callback in your own module, you have to define two different menu callbacks? That's a little odd.

See http://drupal.org/node/93854#comment-4895132 for why two callbacks are needed if you want to have a callback that includes %menu_tail

xjm’s picture

Status: Reviewed & tested by the community » Needs review
Issue tags: +Needs change record

Though, since the path is changing, I think we need a change notice. I also just realized this is actually a dicey backport because it can screw with people's hook_menu_alter()s. Is there anything we can add to the D7 patch to alleviate that?

Dave Reid’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs change record

Sure, this approach works for this specific autocomplete in core. But I'm just wondering since this approach seems like it won't work if you have another actual menu load callback in your autocomplete path? For example if this were taxonomy/autocomplete/%taxonomy_vocabulary/%menu_trail wouldn't this add unwanted parameters to the taxonomy_vocabulary_load call?

Dave Reid’s picture

Status: Reviewed & tested by the community » Needs review
Issue tags: +Needs change record
Dave Reid’s picture

Issue summary: View changes

Updated issue summary.

xjm’s picture

Issue summary: View changes

Updated issue summary.

Steven Jones’s picture

So are we saying that we need to fix the implementation that got into Drupal 8?

Dave Reid’s picture

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

We need to roll this back for D8 or document the major deficiency in using %menu_tail. Contrib maintainers will look to core for how to write autocompletions and this will not work for them the moment they have another menu load argument in their path.

xjm’s picture

davereid: xjm: Actually the solution here is to do a $args = func_get_args(); array_shift($args); $tags_typed = implode('/', $args) in taxonomy_autocomplete()

Which is actually a very simple solution. Though, it's actually a little more complicated than that because of other issues. See, for example, #992020: Taxonomy autocomplete does not respect cursor position..

xjm’s picture

Title: Allow autocompletion requests to include slashes » [ROLLBACK] Allow autocompletion requests to include slashes
FileSize
4.08 KB
PASSED: [[SimpleTest]]: [MySQL] 34,308 pass(es). View

Here's a patch to roll it back in D8. *sniffle*

Dave Reid’s picture

Status: Needs review » Reviewed & tested by the community

Ready for rollback. Let's do this simpler once this is rolled back. For catch or Dries, this should be easy to do with just:

git revert 6b54d3
// Then press enter to confirm the revert message
git push
catch’s picture

Title: [ROLLBACK] Allow autocompletion requests to include slashes » Allow autocompletion requests to include slashes
Status: Reviewed & tested by the community » Needs work
Issue tags: -Needs change record

OK, rolled this back. Back to CNW I think?

thinkyhead’s picture

I'm pretty fond of my autocomplete_fix approach, even though it's draconian, taking over all the autocompletes it can find and patching them universally. But once it's installed there's no added overhead. And it's only 60 lines of code. Only thing is that it currently must be re-run to take over autocomplete for any newly-enabled autocompleting modules. It's a hack to fix a hack, namely having autocomplete/ajax requests to go through Drupal's usual menu system - be nice to have a menu path just for ajax needs.

c960657’s picture

Why not just pass the autocomplete string as a GET argument, i.e. taxonomy/autocomplete/foo-vocab?search=123 ? This is the usual approach outside the Drupal world, and it is very simple to implement. Trying to force an arbitrary string into a regular menu path apparently is very tricky (I guess the menu system was not designed for this use-case), so why bother? We don't benefit from having clean URLs, because the autocomplete URLs are not visible to the end-user.

Steven Jones’s picture

Sure, this approach works for this specific autocomplete in core. But I'm just wondering since this approach seems like it won't work if you have another actual menu load callback in your autocomplete path? For example if this were taxonomy/autocomplete/%taxonomy_vocabulary/%menu_trail wouldn't this add unwanted parameters to the taxonomy_vocabulary_load call?

Looking at the documentation for the Wildcard Loader Arguments (http://drupal.org/node/224170) it would seem that the arguments specified in 'load arguments' are appended to load functions, so in this example, taxonomy_vocabulary would indeed get an extra two arguments, but as it only uses the first passed to it, it would work fine. I'd agree though that this is weird. chx suggested in http://drupal.org/node/298561#comment-3489624 that we should just make %menu_tail magically work, but that wasn't really listened to I'd guess.

It's bit distressing that we're still not supporting something as simple as wanting a '/' in a term to autocomplete.

mstef’s picture

FileSize
4.02 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch fix-autocompletion-with-slashes-93854-171.patch. Unable to apply patch. See the log in the details link for more information. View

Rerolled #134 to apply to drupal root (7.x) - needed for drush make.

sixelats’s picture

Patch in #171 worked for me (v7.10)

Thanks!

danielnolde’s picture

Patch fix-autocompletion-with-slashes-93854-171.patch from #171 worked for me, too (Drupal 7).

ptrl’s picture

Patch in #171 solves my problem with slashes in autocompletes for Drupal 7.

mxh’s picture

#171 not working for me (D7.12), still retrieving AJAX 404.

danielb’s picture

#175 - how did you determine this?

c960657’s picture

Any comments on my suggestion in #169? I think we try to solve this problem in a much too complicated way.

Steven Jones’s picture

Status: Needs work » Needs review
FileSize
6.2 KB
PASSED: [[SimpleTest]]: [MySQL] 34,798 pass(es). View

Right, so looking at the the menu system it seems like it might be possible to get menu_tail_load working without having to specify the load_arguments for all other loaders. I've done this in the attached patch. Its a bit hacky, but it's what chx suggested in

If this is acceptable then this would address the concerns raised in #163.

Otherwise, then we can fall back to imploding the arguments sent to the page callback.

@c960657 If we changed to use a GET parameter then I'm not sure we'd ever be able to backport this as we'd break every contrib module using autocomplete.

c960657’s picture

@Steven Jones: When developing for D8, I don't think we should limit ourselves to solutions that can easily be backported. Let's create a simple (and robust) fix for D8, and then deal with fixing D6 and D7 later.

xjm’s picture

Actually, I'd disagree. Let's get this functional bug that affects real, production sites fixed in all three branches first, and then we can iterate to improve it in D8.

Steven Jones’s picture

FileSize
3.78 KB
PASSED: [[SimpleTest]]: [MySQL] 34,810 pass(es). View

Right, here's a less clever approach to this issue that doesn't change any APIs (I think) and is a very safe bug fix, it should be possible to backport this approach to D7 and D6 I think, it is basically the approach from #8. No menu paths change, it contains just the fix for the javascript and then some code in the menu callback to re-combine the arguments with slashes.

I think that the 'correct' way to fix this in D8 is actually to go further and fix menu_tail_load as in #178, but I don't think that should be backported anywhere really, and probably needs a lot more review than we want in this issue (this issue is already over five years old).

Steven Jones’s picture

Issue summary: View changes

Updated issue summary.

yannickoo’s picture

That looks good, when will the patch be commited?

xjm’s picture

Issue tags: +Novice, +Needs manual testing
  • The patch still needs review.
  • We should make sure it addresses @Dave Reid's concerns from #160 and below (looks like it does).
  • The patch should also be tested manually similarly to in #141, in all major browsers (IE6-9, FF, Chrome, Safari) since it involves a JS change.
  • I'd suggest removing the t() from the assertion message texts. Reference: http://drupal.org/simpletest-tutorial-drupal7#t
  • I also think we might want to expand the test coverage a bit; see some of the examples in #138.
  • Finally, it would be good to re-upload the automated test in #181 in a patch by itself to make sure still fails without the patch.

Tagging novice for two tasks: manual testing in various browsers, and expanding the test coverage . See #138 for suggestions of what to test.

Steven Jones’s picture

FileSize
2.07 KB
FAILED: [[SimpleTest]]: [MySQL] 35,047 pass(es), 1 fail(s), and 0 exception(s). View

Here's #181, minus t and just the tests.

Steven Jones’s picture

FileSize
4.68 KB
PASSED: [[SimpleTest]]: [MySQL] 35,060 pass(es). View
3.77 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal-93854-autocomplete-slashes-no-t.patch. Unable to apply patch. See the log in the details link for more information. View

We should make sure it addresses @Dave Reid's concerns from #160 and below (looks like it does).

Dave's concern was with the usage of menu_tail_load() which we isn't used by this patch any more, so Dave should be happy :)

Here are two further patches, one is #181 without t, and the other includes an extra test that tries to do an autocomplete with both a comma and a slash in it. I.e. it does a search for: "term with, comma and / a which is I think how you would do an autocomplete search with a comma in the term name, but I may be wrong. However, although I'm up for fixing the autocompletion as much as we can, this issue is only about slashes, so I'm not sure if we should even 'care' about commas.

xjm’s picture

+++ b/core/modules/taxonomy/taxonomy.testundefined
@@ -717,6 +717,58 @@ class TaxonomyTermTestCase extends TaxonomyWebTestCase {
+    ¶

Bit of trailing whitespace here. Also, it looks like the latest test-only patch did not apply.

However, although I'm up for fixing the autocompletion as much as we can, this issue is only about slashes, so I'm not sure if we should even 'care' about commas.

Aye that's true; however, the interaction with other special characters that have been broken before (like commas and apostrophes) is worth having test coverage for. That's also why I suggested testing these manually. Existing test coverage for those characters doesn't cover the slash in the same term. Thanks!

Steven Jones’s picture

Lame, my first patch above is the 'negative' diff, I must have had my arguments to git diff the wrong way around, will try to get them correct later.

Steven Jones’s picture

FileSize
4.67 KB
PASSED: [[SimpleTest]]: [MySQL] 35,086 pass(es). View

Here is the patch with more tests, with the whitespace issue fixed.

Steven Jones’s picture

Right, apologies for the confusing of patches we have:

  1. Comment #184 is the test only version of this patch, as predicted, it applies and the test fails.
  2. Comment #188 is the tests + fix version of this patch, and should be what is being reviewed and discussed.

Apologies for the noise and all the different patches, hopefully that will clear things up.

Steven Jones’s picture

Issue summary: View changes

Updated issue summary.

ryanissamson’s picture

Okay, I tested and confirmed the issue following the steps to reproduce in both webkit and ff.

The patch applied cleanly.

After applying the patch and reproducing the steps in both webkit and ff, I received an error (error below and also screenshot). After saving the article and navigating the the taxonomy page, the terms were listed. I attempted to create another article - when I entered in the saved terms, the error message once again pops up.

An AJAX HTTP error occurred.
HTTP Result Code: 404
Debugging information follows.
Path: http://localhost:8888/drupal8/taxonomy/autocomplete/field_tags
StatusText: Not Found
ResponseText: 
404 Not Found
Not Found
The requested URL /drupal8/taxonomy/autocomplete/field_tags/4/5/6, this is a tag/slach was not found on this server.

error

ZenDoodles’s picture

Status: Needs review » Needs work
Steven Jones’s picture

Status: Needs work » Needs review

@ryanissamson to confirm, the patch you applied was the one in #188?

ryanissamson’s picture

Sorry, I should have clarified. That is correct, I applied the patch from #188.

Steven Jones’s picture

Status: Needs review » Needs work

@ryanissamson Thanks for the patch, I will try to work on this in a bit.

Steven Jones’s picture

Status: Needs work » Needs review

Odd, I just just tried this again on my server and it works just fine, @ryanissamson were you testing other core issues before this one? Were the caches cleared on your browser.

It's really odd that you'd get a 404, because the automated tests should have failed if that was the case, but they don't. Either this is down to the client side JS being cached as an old version, the server not being set up to handle clean URLs quite right, or some mystery other issue.

If anyone else wanted to review the patch from #188 that would be much appreciated.

ryanissamson’s picture

You know, I forgot to clear the caches. I apologize. I am about to get back to work and I'll retest. Sorry everyone. Will update ASAP.

Steven Jones’s picture

I honestly can't thank you enough for testing. Whatever the outcome!

ryanissamson’s picture

Status: Needs review » Reviewed & tested by the community

Aha! So, the mistake was on my part.

I tested again on a fresh install of Drupal just to be sure. I reproduced the problem by following the steps. I then applied the patch, cleared the browser and server caches and tested again in both webkit and FF browsers and voila! Everything seems to be great! I followed the steps and couldn't see any issue with saving terms with slashes. They also weren't duplicated and autocomplete had no problem calling them up.

Looks good to me!

xjm’s picture

Status: Reviewed & tested by the community » Needs review

Thanks @ryanissamson and @Steven Jones. We still need IE testing though. :)

Steven Jones’s picture

Okay, well I've checked, and it works in:

  • IE9:
    autocomplete_ie9.png
  • IE8:
    autocomplete_ie8.png
  • And for those that care (Drupal 6 & 7 will) IE7:
    autocomplete_ie7.png

I'm not going to mark it as RTBC because it's my own patch.

xjm’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: -Novice, -Needs manual testing

Yay!

aspilicious’s picture

After 5 years... VICTORY

moonray’s picture

Hah! Finally!
And to think the first response was "this should be a simple fix.."

moonray’s picture

[duplicate post, contents removed]

catch’s picture

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

This looks like the least possible change we could make, and the test coverage is great.

Committed/pushed to 8.x, moving to 7.x for backport.

Steven Jones’s picture

Assigned: Unassigned » Steven Jones

@catch Thanks for the commit!

I am all over this one.

Steven Jones’s picture

Assigned: Steven Jones » Unassigned
Status: Patch (to be ported) » Needs review
FileSize
4.61 KB
PASSED: [[SimpleTest]]: [MySQL] 38,318 pass(es). View

Here's the patch for D7.

Steven Jones’s picture

Issue summary: View changes

Updated issue summary.

Steven Jones’s picture

Issue summary: View changes

Updated issue summary.

xjm’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: +7.13 release notes

Backport looks good!

webchick’s picture

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

AWESOME! Great work on this, folks. Totally excited to see how many of these 200+-reply, 5+-year old issues are getting fixed in this release. :D

Committed and pushed to 7.x. Marking to 6.x for backport.

Steven Jones’s picture

Assigned: Unassigned » Steven Jones
Issue tags: -needs backport to D7

I'll do that backport too, guessing the tests get dropped in this one.

webchick’s picture

D6 does have some kind of test coverage, but I'm not sure exactly what or how it works.

Dave Reid’s picture

Tests need to just be dropped completely for D6 backports.

Steven Jones’s picture

Assigned: Steven Jones » Unassigned
Status: Patch (to be ported) » Needs review
FileSize
779 bytes
PASSED: [[SimpleTest]]: [MySQL] 190 pass(es). View

And here is the fix for D6.

danielb’s picture

The patches in this issue were also usable to fix the autocomplete forward-slash in one of my modules.
However I maintain seven modules with autocompletes that I can think of, and no doubt there are many maintainers out there with the same issue... *sigh*
Someone should do up a documentation/tutorial on how to do the autocomplete callbacks properly?

I made a comment on the FAPI documentation in the same spirit: http://api.drupal.org/api/drupal/developer%21topics%21forms_api_referenc...

Is there a quick way to determine which modules on d.o. use #autocomplete_path and file a bug report?

xjm’s picture

#214 raises an excellent point. I define a custom autocomplete callback in a module, and even following this issue it did not occur to me that I would need to fix the bug in that callback as well. I wonder if there's a way that we could make this change more systematic for #autocomplete_path, or at least factor out a helper for contrib to use?

Steven Jones’s picture

@xjm the helper would be called menu_tail_load which in D7 and below is a bit naff in that you need to include the loader arguments in your menu callback. I plan to take my cleanup from #178 and get them into D8, which would mean that people could just use %menu_tail in their menu item definitions, page callbacks would be simplified, and it would be obvious what is going on.

For D7, documentation would be the way to go, maybe docs on the #autocomplete_path property would be the most helpful place.

xjm’s picture

+++ b/modules/taxonomy/taxonomy.pages.incundefined
@@ -108,6 +108,13 @@ function taxonomy_term_feed($term) {
+  // 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);

I just meant a generic helper of this.

For D7, documentation would be the way to go, maybe docs on the #autocomplete_path property would be the most helpful place.

Sounds reasonable to me.

I don't want to block this getting into D6, so I opened a followup: #1492378: Document proper use of #autocomplete_path for slashes

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.

mxh’s picture

Drupal 7.12: Cygwin console output "Hunk failed at line 108" while trying to apply patch from #207.
#220: thanks, my fault. please remove this post.

xjm’s picture

#219: The patch has already been committed to Drupal 7. It will be in the next release. The issue still needs to be fixed in D6.

JvE’s picture

I just created a test in #1559142: Allow autocompletion terms to start with a slash for the failing case of terms starting with slashes.

panche’s picture

I have a similar issue but with dot, what about a term beginning with dot (.), say: .NET, this immediately brakes the URL adding the current path's last argument to the autocomplete URL.
Any toughs on that?
Thanks!
PD.
Using Drupal 7.14

panche’s picture

This patch worked for me:
At line 273 of misc/autocomplete.js add:

  // See if this string begins with dot (Patch by panche)
  searchString = searchString.replace(/^\./, '');
  this.searchString = searchString; // This is needs to be done in order to show the matches, otherwise it won't show them
Index: autocomplete.js
===================================================================
--- autocomplete.js	(revision 35)
+++ autocomplete.js	(working copy)
@@ -270,6 +270,8 @@
 
   // See if this string needs to be searched for anyway.
   searchString = searchString.replace(/^\s+|\s+$/, '');
+  // See if this string begins with dot
+  searchString = searchString.replace(/^\./, '');
+  this.searchString = searchString; // To show the matches, otherwise it won't show them
   if (searchString.length <= 0 ||
     searchString.charAt(searchString.length - 1) == ',') {
     return;
Steven Jones’s picture

@panche Please open a new issue for Drupal 8 with that bug please.

panche’s picture

Will do.

panche’s picture

@Steven Jones http://drupal.org/node/1661098

Sorry, first timer.

mindbat’s picture

Patch doesn't seem to work for Node Reference autocompletes. Applied patch to Drupal 7.9, and I'm still seeing this issue for Node Reference autocompletes.

Anyone have a fix for node-based autocompletes?

mxh’s picture

@mindbat

It could be a Node Reference problem. Would you please try Entity Reference for your case and report back here?
see #229.

rv0’s picture

@mindbat

You need 7.13 at least

panche’s picture

@mindbat Here's a better solution:
#581706: Protect .git, .hg and .bzr directories in .htaccess See comment 34

mindbat’s picture

@rv0, so 7.13 has a fix for node reference autocompletes, not just taxonomy references?

Remember, I applied the patch that's been incorporated into a later release to fix this issue, and it did not work for me. Do you have a reference to the patch or commit that fixed the node reference autocomplete issue?

mindbat’s picture

@panche: Thanks for the suggestion. Unfortunately, didn't fix the issue for me :(

effulgentsia’s picture

For D8, we're looking at changing this to #1831324: Form API #autocomplete_path JS and callbacks to use GET q= parameter instead of menu tail.

However, we can't do that for D7 or D6, so it doesn't affect this issue, just linking for reference.

effulgentsia’s picture

Also, the code in #213 looks like a nice backport to me, but I don't have time to test it right now. If someone can confirm that it works, please RTBC. Looks like the comments since then offer up some good follow up material, but why not get the fix that's already in 7.x into 6.x?

hellolindsay’s picture

Status: Needs review » Needs work

I believe this issue has resurfaced. I am using Drupal 7.20 and when I type a "/" character into any autocomplete field the slashes are not url-encoded along with the rest of the search string. Take a look at this:

If we look inside of autocomplete.js, we see a comment that explains how we use "Drupal.encodePath" instead of "encodeURIComponent" to allow for search terms to include slashes. Here is the comment and the code that follows it. Lines 292 to 312 in misc/autocomplete.js:

// Ajax GET request for autocompletion. We use Drupal.encodePath instead of
// encodeURIComponent to allow autocomplete search terms to contain slashes.
$.ajax({
      type: 'GET',
      url: db.uri + '/' + Drupal.encodePath(searchString),
      dataType: 'json',
      success: function (matches) {

        if (typeof matches.status == 'undefined' || matches.status != 0) {
          db.cache[searchString] = matches;
          // Verify if these are still the matches the user wants to see.
          if (db.searchString == searchString) {
            db.owner.found(matches);
          }
          db.owner.setStatus('found');
        }
      },
      error: function (xmlhttp) {
        alert(Drupal.ajaxError(xmlhttp, db.uri));
      }
});

But if we actually look at the code for Drupal.encodePath we see another comment that says "For aesthetic reasons slashes are not escaped." Within the code, it looks like slashes are specifically not encoded. Notice the ".replace" method at the end of the third line. These are lines 320 to 328 in misc/drupal.js:

/**
 * Encodes a Drupal path for use in a URL.
 *
 * For aesthetic reasons slashes are not escaped.
 */
Drupal.encodePath = function (item, uri) {
  uri = uri || location.href;
  return encodeURIComponent(item).replace(/%2F/g, '/');
};

So it looks like these two pieces of logic are in conflict.

Pere Orga’s picture

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

I also think the issue was not properly fixed. In Drupal 7.17 if I include slashes in my search I'm getting wrong results:

  • some strings that should not match are shown
  • strings that should match are not shown (maybe because the 10 max results limitation)
tjg’s picture

Version: 7.x-dev » 7.22

This is still an issue in 7.22. Is there any chance of getting this fixed, or should I patch my own code?

roeldl’s picture

Version: 7.22 » 7.23

This is still an issue in 7.23, can this please be properly fixed?

klonos’s picture

Version: 7.23 » 7.x-dev

...to avoid changing versions all the time ;)

thinkyhead’s picture

So far my testing has shown that slashes sort-of work with Drupal 7. But autocomplete strips off all trailing slashes rather than treating them as characters to be searched. So "Bob/" will match "Bob/Weave" but also "Bob and Mary" and "/" by itself or "/////" will match nothing.

I've ported my old "Autocomplete Fix" modules (See Comment #43) to Drupal 7. This seems to me to be one of those intractable problems that requires a somewhat hackish solution. The autocomplete code is trying to use a codepath that treats some characters as special, and this is bound to keep breaking things.

So here you go, a hackish but apparently still working solution for D6 and D7...

https://drupal.org/project/autocomplete_hack
https://drupal.org/project/autocomplete_post

mrfree’s picture

I think I have the same problem but using the EntityReference widget on nodes title so it isn't strictly related with taxonomy term only.

If I try to search some thing like "28/10/2013" in the entityreference module's autocomplete_callback I can only find "28"

mrfree’s picture

Issue summary: View changes

Updated issue summary.

ndobromirov’s picture

Issue summary: View changes
Related issues: +#2175043: Autocomplete breaks when there are slashes in the search string
FileSize
1.79 KB
FAILED: [[SimpleTest]]: [MySQL] 40,681 pass(es), 54 fail(s), and 61 exception(s). View

What do you think for solution like the following one:
1. Use a custom tag/placeholder for slashes in autocomplete request URL, therefore menu will not split it wrongly.
2. Manually replace the tag back to the original symbol in drupal_explode_tags as it is called every time.

Adding a simple patch against 7.x branch, that implements the idea described above.

There is the possibility that in future other special symbols can be added through some info/alter hook or system config page.
Please, share your thoughts on this! :)

ndobromirov’s picture

Status: Needs work » Needs review

Forget to change the status!

Status: Needs review » Needs work

The last submitted patch, 242: drupal-93854-autocomplete-slashes-242.patch, failed testing.

ndobromirov’s picture

Status: Needs work » Needs review
FileSize
1.8 KB
FAILED: [[SimpleTest]]: [MySQL] 40,607 pass(es), 1 fail(s), and 0 exception(s). View

There was a silly mistake in the previous patch.
Uploading a new one.

Status: Needs review » Needs work

The last submitted patch, 245: drupal-93854-autocomplete-slashes-245.patch, failed testing.

ndobromirov’s picture

Status: Needs work » Needs review
FileSize
3.9 KB
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in modules/taxonomy/taxonomy.test. View

The one failing test is caused by the difference in auto-complete URL generation between the tests in Drupal's back-end and the new JavaScript implementation.
Upload of a new patch that updates the test cases + some tweaks.

Status: Needs review » Needs work

The last submitted patch, 247: drupal-93854-autocomplete-slashes-247.patch, failed testing.

ndobromirov’s picture

Status: Needs work » Needs review
FileSize
3.91 KB
FAILED: [[SimpleTest]]: [MySQL] 40,721 pass(es), 2 fail(s), and 0 exception(s). View

Fixed typos.

Status: Needs review » Needs work

The last submitted patch, 249: drupal-93854-autocomplete-slashes-249.patch, failed testing.

JvE’s picture

  1. +++ b/includes/common.inc
    @@ -7419,7 +7419,7 @@ function drupal_explode_tags($tags) {
    +      $tags[] = str_replace('[--SLASH--]', '/', $tag);
    

    This is not the correct place to put the slash back. drupal_explode_tags is a general function used by a lot more than taxonomy.

  2. +++ b/misc/autocomplete.js
    @@ -293,7 +293,7 @@ Drupal.ACDB.prototype.search = function (searchString) {
    +      url: db.uri + '/' + Drupal.encodePath(searchString).replace('%2F', '[--SLASH--]'),
    

    The definition of encodePath is encodeURIComponent(item).replace(/%2F/g, '/') so I do not see how it would return any %2F to replace.

  3. +++ b/modules/taxonomy/taxonomy.pages.inc
    @@ -119,13 +119,6 @@ function taxonomy_term_feed($term) {
    function taxonomy_autocomplete($field_name = '', $tags_typed = '') {
    -  // If the request has a '/' in the search text, then the menu system will have
    -  // 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);
    -  $tags_typed = implode('/', $args);
    	
    

    Taxonomy is not the only module using autocomplete. There are many others. None of them expect [--SLASH--] in their input.

ndobromirov’s picture

Status: Needs work » Needs review
FileSize
4.3 KB
FAILED: [[SimpleTest]]: [MySQL] 40,597 pass(es), 1 fail(s), and 0 exception(s). View

Added utility method drupal_escape_autocomplete_input to provide contributed modules to generate autocomplete paths.
Fixed some small issues within patch in #249.

Status: Needs review » Needs work

The last submitted patch, 252: drupal-93854-autocomplete-slashes-251.patch, failed testing.

ndobromirov’s picture

Thanks for the comments.

Comment 1:
Everyone that uses auto-complete in some sort, makes it similar to the one used in taxonomy module. And there is always the drupal_explode_tags somewhere in the callback to extract the autocomplete tags.

The implementation change in drupal_explode_tags is not structural.

Comment 2:
It is fixed in the last patch.

Comment 3:
Yes there are many others that have skipped on the menu path magick that is present in the taxonomy_autocomplete just an example: i18n_taxonomy, workbench_access and most likely others.

They have not experienced the issues with the current slash problems, so the [--SLASH--] input will not break them - not much more than before.

Now just thinking!
1. If we add the drupal_escape_autocomplete_input it might be better to rename it to drupal_encode_autocomplete_input, so we can add the meaningful drupal_decode_autocomplete_input.
Then modules can do on their own.

2. It should be possible to preprocess the autocomplete menu parameters prior sending them to the callback (some menu hook). If there is autocomplete_path in it's menu definition, parameters can be processed before calling the menu callback, so contributed modules will not notice the [--SLASH--] problem. So you are most likely right about the place of the [--SLASH--] :) .

And there will be no need for the menu paths magic.

webbykat’s picture

Unrelated to the patch above, added for fellow developers who've wound up at this thread looking for a solution:

If you don't want to patch core, you can get around this rather hackily using the Chosen module (https://www.drupal.org/project/chosen) as follows:

- Change your relevant reference fields to Select lists instead of Autocompletes under Manage fields > [Field] > Widget type.
- Follow the instructions to install the module (don't forget to add its library).
- On the configuration page, set the first two options to Always Allow.
- If you don't want Chosen to get applied everywhere on your site, you can set this value in the "Apply Chosen to the following elements" box: .field-type-node-reference.field-widget-options-select select:visible

Worked like a charm for me. Now you can autocomplete with a fairly slick interface. My case was node references, not taxonomy references, but it should hold up the same - I've used Chosen on taxonomy references in the past.

Please note that this module is currently in beta, so use at your own risk.

thinkyhead’s picture

I've just updated my Autocomplete POST and Autocomplete Hack modules for D6 and D7 to deal with some common issues. The Javascript now makes sure that Drupal.ACDB is defined before trying to patch its 'search' method. And the behavior is now re-entrant for AJAX loaded pages (AJAX elements are supposed to apply all Drupal behaviors in their 'success' handler). The D7 versions fix a longstanding bug in the custom PHP autocomplete handler.

These two modules are still the best options that I have found to solve the slash problem. For one thing, these modules patch all autocomplete fields, not just those (like Taxonomy) in Drupal core, and they don't add any significant overhead to the process. The best part is that module maintainers don't have to worry about some late API change in Drupal core, and users don't need to worry about patching any contrib modules that may be broken by Drupal core changes.

Perhaps a better name for these modules would be: "Allow Slashes in Autocomplete Fields" and they could be touted as a helpful add-on for those sites that want to be able to have tags like "9/11" or "/jk/." At the moment these useful modules are pretty obscure, with only 5 sites reporting usage, and at least 2 of those are my own! I am presuming that this is totally fixed in D8!

pwolanin’s picture

This is fixed in 8 by moving the search string to a query string where it properly belongs.

pwolanin’s picture

FileSize
777 bytes

Here's a quick patch for 6 to backport for just taxonomy terms the fix that's in 7 core.

  • catch committed 6b54d34 on 8.3.x
    Issue #93854 by pillarsdotnet, Steven Jones, Dave Reid, ericduran, das-...
  • catch committed 4fa6611 on 8.3.x
    Revert "Issue #93854 by pillarsdotnet, Steven Jones, Dave Reid,...
  • catch committed 0549db8 on 8.3.x
    Issue #93854 by pillarsdotnet, Steven Jones, Dave Reid, ericduran, xjm,...

  • catch committed 6b54d34 on 8.3.x
    Issue #93854 by pillarsdotnet, Steven Jones, Dave Reid, ericduran, das-...
  • catch committed 4fa6611 on 8.3.x
    Revert "Issue #93854 by pillarsdotnet, Steven Jones, Dave Reid,...
  • catch committed 0549db8 on 8.3.x
    Issue #93854 by pillarsdotnet, Steven Jones, Dave Reid, ericduran, xjm,...

  • catch committed 6b54d34 on 8.4.x
    Issue #93854 by pillarsdotnet, Steven Jones, Dave Reid, ericduran, das-...
  • catch committed 4fa6611 on 8.4.x
    Revert "Issue #93854 by pillarsdotnet, Steven Jones, Dave Reid,...
  • catch committed 0549db8 on 8.4.x
    Issue #93854 by pillarsdotnet, Steven Jones, Dave Reid, ericduran, xjm,...

  • catch committed 6b54d34 on 8.4.x
    Issue #93854 by pillarsdotnet, Steven Jones, Dave Reid, ericduran, das-...
  • catch committed 4fa6611 on 8.4.x
    Revert "Issue #93854 by pillarsdotnet, Steven Jones, Dave Reid,...
  • catch committed 0549db8 on 8.4.x
    Issue #93854 by pillarsdotnet, Steven Jones, Dave Reid, ericduran, xjm,...