| Project: | Drupal core |
| Version: | 8.x-dev |
| Component: | forms system |
| Category: | bug report |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | needs work |
| Issue tags: | needs backport to D6, needs backport to D7 |
Issue Summary
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 #134 implements the following solution:
-
Changes
misc/javascript.jsto enable autocomplete terms to contain slashes. -
Adds a new
taxonomy/autocomplete/%/%menu_tailmenu path totaxonomy_menu()so that multiple path components may be passed to the search function..(See menu_tail_load().
-
Adds automated test coverage for this functionality.
Remaining tasks
- The Drupal 8 patch has been committed. A Drupal 7 patch is in #149; however, the patch changes the path for taxonomy autocompletion. If possible, we should find a way to make this safer to backport.
Steps to test this patch
- Create an article and enter a tag containing a slash. Save the article and verify the term is created correctly.
- 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/addto make sure it used the same tag rather than creating a new one. - 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. - Test autocompletion and saving of these terms as well.
User interface changes
None.
API changes
- The path for taxonomy autocompletion is now
taxonomy/autocomplete/%/%menu_tailrather thantaxonomy/autocomplete.
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. :-)
Comments
#1
I originally filed this under 4.7, but I believe this is still an issue in 5.
Please double check...
#2
Oops... 5.0.x-dev
#3
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
#4
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.
#5
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.
#6
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
#7
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},
...
...
});
#8
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:
<?php/**
* 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:
<?php
/**
* 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?
#9
Yes, #8 has the correct approach. See http://drupal.org/patch/create for information on how to post a patch.
#10
Let's check if this is fixed in 7.x now, fix there, then backport.
#11
Also marked #348679: terms with slashes (/) don't lookup properly in autocomplete fields as a duplicate of this issue.
#12
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:
<?php
/*
* 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:
<?phpfunction taxonomy_autocomplete($vid) {
$string = drupal_get_path_segment(3);
?>
Initial patch with initial tests for review.
#13
#14
The last submitted patch failed testing.
#15
And my argument for needing this in core, is that this function can replace:
<?phpfunction path_admin_filter_get_keys() {
// Extract keys as remainder of path
$path = explode('/', $_GET['q'], 5);
return count($path) == 5 ? $path[4] : '';
}
?>
<?phpfunction 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.
#16
The last submitted patch failed testing.
#17
Instead of adding a whole new path.test, let's see if this one passes the test bot.
#18
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:
<?php
// 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.
#19
Talking with a sysadmin, I saw multiple solutions, they all have benefits and disavantages:
Double encoding
Benefits
Disadvantages
Base 64 encoding
Benefits
The same as double encoding.
Disadvantages
The same as double encoding.
Send parameters through POST
Benefits
Disadvantages
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.
#20
@pounard: What about the proposal and solution in #12?
#21
@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.
#22
@pounard: Your problem in #18 can be solved using the solution in #12 easily:
<?php
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);
}
?>
#24
What if my filter contains one or more slash caracters ?
#25
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).
#26
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.
#27
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.
#28
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.
#29
@pounard - you should be using
%menu_tailinstead of%in your hook_menu() to grab all the stuff at the end of your menu path:<?php/**
* 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.
#30
What if I want to do this:
myautocomplete/%/foo
or
myautocomplete/%/%/foo
or
myautocomplete/%/foo/%
or
myautocomplete /%/%
#31
%menu_tailis there for a reason. Let's fix taxonomy_autocomplete() and friends to use it.#32
By the way, this new drupal_get_path_segment($arg, $path) seems nothing more then:
<?phpimplode('/', array_slice(arg(NULL, $path), $arg))
?>
Except if I'm missing the point?
#33
@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 :)
#34
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.
#35
You've made your point pounard. Create a new issue to rewrite the autocompletion process and submit a patch.
#36
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
#37
#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
#38
#39
Is there a reason not to just use regular GET parameters for autocomplete paths?
/foo/autocomplete?search=bar%2Fbaz works fine for me...
#40
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'
#41
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_encodeto 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 :)
#42
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...
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:
<?phpfunction 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?
#43
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!
<?php
/**
* 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
#44
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.
#45
subscribe
#46
Marked #995512: Autocomplete doesn't work with slashes as a duplicate.
#47
#48
+1
#49
I cant sleep until this issue is fixed
#50
#51
pillarsdotnet, why do you have to attack me? Just said that I would love to see this issue fixed.
And where is your patch?
#52
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.
#53
Any progress with this Steven Jones?
#54
Not yet, but I will be able to look at it in a few hours :)
#55
Okay, so here's a test for the edge case of a term containing a slash. testbot should fail on this.
#56
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.
#57
So the test correctly catches the failure, and the fix for taxonomy, now need to fix the JS.
#58
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?
#59
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?
#60
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.
#61
Better title (I think).
#62
Ah, cross post with myself!
#63
You good! it's work thx! =)
#64
Tested patch in #60 and it works with new and existing autocomplete taxonomy terms. Thanks!
#65
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.
#66
#65: taxonomy_autocomplete.patch queued for re-testing.
#67
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.
#68
subscribe
#69
+ // This path is also required for autocomplete to work.$items['taxonomy/autocomplete'] = array(
@@ -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';
#70
Killing the menu hook seems like a good idea.
Patch attached for the 8.x branch, it should backport fairly easily.
#71
Updating tags.
#72
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".
#73
This looks good.
A few nits:
+ // Needed for menu_tail_load().+ 'load arguments' => array('%map', '%index'),
+ // Try to autocomplete the term name, that contains a slash.+ // We should only get a single term returned.
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.
#74
Re-rolled with suggestions and some additional changes to avoid unnecessarily long code lines.
#75
Bah! Sorry about the mixup. Here's the patch I meant to upload, and an interdiff from #70.
#76
Sorry; missed that bit. Re-rolled accordingly.
#77
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.
#78
If you provide a list of the other uses of autocomplete in core, we can get those fixed too.
#79
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.
#80
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?
#81
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);
#82
Actually, the (new) convention is to remove t() from assertion tests. See #500866: Simpletest: remove t() from assert message
#83
@pillarsdotnet actually if we're using the placeholder functionality of
tthen 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.
#84
Attached patch is #76 plus a fix for:
#85
Looking forward to the D7 back port. Sub-ing
#86
I would like to see this backported to D7 as well and maybe back to D6.
#87
#88
sub + D7 backport
#89
- 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
#90
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.#91
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.
#92
#84: fix_autocompletion_with_slashes-93854-76.patch queued for re-testing.
#93
i support #91: errors should be errors
i spent too much hours with errors that are silently ignored...
#94
Subscribing in eager anticipation of a Drupal 7 backport.
#95
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']) {#96
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.
#97
Re-rolled with change suggested in #89/91/93/95
#98
The last submitted patch, fix_autocompletion_with_slashes-93854-96.patch, failed testing.
#99
@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.
#100
Attch patch is same as in #99 but works with drush make
#101
The last submitted patch, fix_autocompletion_with_slashes-93854-99-make.patch, failed testing.
#102
(sigh) Re-uploading the patch in #99.
#103
subscribing.
#104
We need sign-off from sun/chx on this. This is really, really weird.
#105
#106
It looks like drupal_valid_path would accept
$element['#autocomplete_path'] . '/%', instead of 'dummy'. Thoughts?#107
#97: fix_autocompletion_with_slashes-93854-96.patch queued for re-testing.
(still can't imagine how this fails.
#108
@#106 -- okay; testing.
#109
@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.
#110
#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.
#111
>#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.
#112
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)
#113
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.
#114
Can we get a summary on this issue?
#115
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.
#116
The patch in #108:
Changes
misc/javascript.jsto enable autocomplete term to contain slashes.Adds a test to ensure that the change to
misc/javascript.jsactually worked.Changes
taxonomy_menu()to definetaxonomy/autocomplete/%/%menu_tailinstead oftaxonomy/autocomplete, since no valid autocompletion request would ever equaltaxonomy/autocomplete.Changes
theme_textfield()to allow the taxonomy change to work.Dropping the (arguably unrelated) changes to
form.incandtaxonomy.module, we have the following patch:#117
Yay, that patch looks MUCH better!!
#118
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.
#119
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.
#120
Lol, wrong patch, --ignore that.
I meant to upload this one.
#121
#122
Re-uploading the patch from #108, with a clarifying comments added.
#123
@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.
#124
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',
#125
I get the difference, but #108 is also incorrect.
#126
Here's a patch,
This is very similar to the other patches but also different.
Here's what this patch does:
#127
Okay, if it works, but leave the extra comments in, at least.
#128
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_menufor 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_autocompleteI 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.
#129
Better yet, check the test results in a way that does not depend on a specific result order.
#130
Tag cleanup.
#131
@#128 by Steven Jones on October 18, 2011 at 8:30am:
Pretty close. Here's the interdiff.
#132
Re-arranged and added missing comments to make the interdiff smaller.
#133
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.
#134
Re-roll of #132 to place files in the core directory and incorporate changes from #133.
#135
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.
#136
The last submitted patch, autocomplete_slashes_test_only.patch, failed testing.
#137
Test failed as expected.
#138
Now let's have one or two other people test the patch from #134 functionally and report the results. Some things you might try:
admin/structure/taxonomy/tags/addto make sure it used the same tag rather than creating a new one.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.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.
#139
Updated summary.
#140
#134 works for me.
#141
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.
#142
Thanks @ericduran! The screenshots are a big help. This feels RTBC to me too. :)
#143
%menu_tail yay. looks good.
#144
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.
#145
Need back ported to D6, see http://drupal.org/node/1088948
#146
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?
#147
#146: Nothing. It's marked RTBC.
#148
Looks good here, committed/pushed to 8.x. Will need a re-roll for 7.x.
#149
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.
#150
#151
Patch from #149 applies cleanly, fixes the issue for me on 7.9. +1
#152
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.
#153
@linclark that's exactly what this issue should fix. What browser are you seeing this error with?
#154
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?
#155
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.
#156
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.
#157
Well, it's the same callback for both paths.
#158
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
#159
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?#160
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_trailwouldn't this add unwanted parameters to the taxonomy_vocabulary_load call?#161
#162
So are we saying that we need to fix the implementation that got into Drupal 8?
#163
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.
#164
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..
#165
Here's a patch to roll it back in D8. *sniffle*
#166
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
#167
OK, rolled this back. Back to CNW I think?
#168
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.
#169
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.
#170
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_vocabularywould 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.
#171
Rerolled #134 to apply to drupal root (7.x) - needed for drush make.
#172
Patch in #171 worked for me (v7.10)
Thanks!
#173
Patch fix-autocompletion-with-slashes-93854-171.patch from #171 worked for me, too (Drupal 7).