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
- 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/add
to 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
- 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. :-)
Comment | File | Size | Author |
---|---|---|---|
#258 | 93854-D6-258-do-not-test.patch | 777 bytes | pwolanin |
#252 | drupal-93854-autocomplete-slashes-251.patch | 4.3 KB | ndobromirov |
#213 | drupal-93854-autocomplete-slashes-extra-test-d6.patch | 779 bytes | Steven Jones |
#207 | drupal-93854-autocomplete-slashes-extra-test-d7.patch | 4.61 KB | Steven Jones |
#200 | autocomplete_ie9.png | 9.36 KB | Steven Jones |
Comments
Comment #1
moonray CreditAttribution: moonray commentedI originally filed this under 4.7, but I believe this is still an issue in 5.
Please double check...
Comment #2
moonray CreditAttribution: moonray commentedOops... 5.0.x-dev
Comment #3
Anonymous (not verified) CreditAttribution: Anonymous commentedthis 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
Comment #4
Steven CreditAttribution: Steven commentedThe 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.
Comment #5
dman CreditAttribution: dman commentedRelated, possibly a new issue against 5.0, but...
The current 5.0 drupal.js contains
which is a game effort, but buggy.
in javascript
replace('%2F', '/');
is non-greedy, and produces:(with unclean urls)
It unescaped just ONE of the slashes producing a problem.
I'd suggest patching to
... if swapping all encoded slashes back to slashes was indeed the intent.
Comment #6
Leonid.S CreditAttribution: Leonid.S commentedI 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.
This function is callback for path like
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
Comment #7
Leonid.S CreditAttribution: Leonid.S commentedWhy instead of GET method for request from autocomplet.js we can't use POST method?
Something like:
Comment #8
becw CreditAttribution: becw commentedThis 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:
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:
Make any sense?
Comment #9
drummYes, #8 has the correct approach. See http://drupal.org/patch/create for information on how to post a patch.
Comment #10
Dave ReidLet's check if this is fixed in 7.x now, fix there, then backport.
Comment #11
Dave ReidAlso marked #348679: terms with slashes (/) don't lookup properly in autocomplete fields as a duplicate of this issue.
Comment #12
Dave ReidOk, 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:
So now in the case of taxonomy_autocomplete(), we would do the following:
Initial patch with initial tests for review.
Comment #13
Dave ReidComment #15
Dave ReidAnd my argument for needing this in core, is that this function can replace:
I'm searching to see if there's more places this could be used.
Comment #17
Dave ReidInstead of adding a whole new path.test, let's see if this one passes the test bot.
Comment #18
pounardStill 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:
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.
Comment #19
pounardTalking 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.
Comment #20
Dave Reid@pounard: What about the proposal and solution in #12?
Comment #21
pounard@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.
Comment #22
Dave Reid@pounard: Your problem in #18 can be solved using the solution in #12 easily:
Comment #24
pounardWhat if my filter contains one or more slash caracters ?
Comment #25
Dave ReidYeah 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).
Comment #26
pounardThat'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.
Comment #27
Dave ReidIf 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.
Comment #28
pounardYes, 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.
Comment #29
becw CreditAttribution: becw commented@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:%menu_tail is mentioned in the menu system %wildcard_to_arg docs.
Comment #30
pounardWhat if I want to do this:
myautocomplete/%/foo
or
myautocomplete/%/%/foo
or
myautocomplete/%/foo/%
or
myautocomplete /%/%
Comment #31
Damien Tournoud CreditAttribution: Damien Tournoud commented%menu_tail
is there for a reason. Let's fix taxonomy_autocomplete() and friends to use it.Comment #32
Damien Tournoud CreditAttribution: Damien Tournoud commentedBy the way, this new drupal_get_path_segment($arg, $path) seems nothing more then:
Except if I'm missing the point?
Comment #33
becw CreditAttribution: becw commented@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 :)
Comment #34
pounardI 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.
Comment #35
Dave ReidYou've made your point pounard. Create a new issue to rewrite the autocompletion process and submit a patch.
Comment #36
Dave ReidIt 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
Comment #37
merlinofchaos CreditAttribution: merlinofchaos commented#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
Comment #38
Dave ReidComment #39
grendzy CreditAttribution: grendzy commentedIs there a reason not to just use regular GET parameters for autocomplete paths?
/foo/autocomplete?search=bar%2Fbaz works fine for me...
Comment #40
Dave ReidHrm, 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'
Comment #41
das-peter CreditAttribution: das-peter commentedJust 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 :)
Comment #42
thinkyhead CreditAttribution: thinkyhead commentedI 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:
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:
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?
Comment #43
thinkyhead CreditAttribution: thinkyhead commentedThe 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!
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
Comment #44
nkschaefer CreditAttribution: nkschaefer commentedslurslee, 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.
Comment #45
vitok-dupe CreditAttribution: vitok-dupe commentedsubscribe
Comment #46
mr.baileysMarked #995512: Autocomplete doesn't work with slashes as a duplicate.
Comment #47
pillarsdotnet CreditAttribution: pillarsdotnet commentedComment #48
droplet CreditAttribution: droplet commented+1
Comment #49
anonI cant sleep until this issue is fixed
Comment #50
pillarsdotnet CreditAttribution: pillarsdotnet commentedComment #51
anonpillarsdotnet, why do you have to attack me? Just said that I would love to see this issue fixed.
And where is your patch?
Comment #52
Steven Jones CreditAttribution: Steven Jones commentedThe 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.
Comment #53
anonAny progress with this Steven Jones?
Comment #54
Steven Jones CreditAttribution: Steven Jones commentedNot yet, but I will be able to look at it in a few hours :)
Comment #55
Steven Jones CreditAttribution: Steven Jones commentedOkay, so here's a test for the edge case of a term containing a slash. testbot should fail on this.
Comment #56
Steven Jones CreditAttribution: Steven Jones commentedRight 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.
Comment #57
Steven Jones CreditAttribution: Steven Jones commentedSo the test correctly catches the failure, and the fix for taxonomy, now need to fix the JS.
Comment #58
Steven Jones CreditAttribution: Steven Jones commentedRight, 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?
Comment #59
Steven Jones CreditAttribution: Steven Jones commentedBut 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?
Comment #60
Steven Jones CreditAttribution: Steven Jones commentedReading 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.
Comment #61
Steven Jones CreditAttribution: Steven Jones commentedBetter title (I think).
Comment #62
Steven Jones CreditAttribution: Steven Jones commentedAh, cross post with myself!
Comment #63
vitok-dupe CreditAttribution: vitok-dupe commentedYou good! it's work thx! =)
Comment #64
skov CreditAttribution: skov commentedTested patch in #60 and it works with new and existing autocomplete taxonomy terms. Thanks!
Comment #65
pillarsdotnet CreditAttribution: pillarsdotnet commentedI 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.
Comment #66
Steven Jones CreditAttribution: Steven Jones commented#65: taxonomy_autocomplete.patch queued for re-testing.
Comment #67
Steven Jones CreditAttribution: Steven Jones commentedIssue 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.
Comment #68
geek-merlinsubscribe
Comment #69
c960657 CreditAttribution: c960657 commentedI 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:
Comment #70
Steven Jones CreditAttribution: Steven Jones commentedKilling the menu hook seems like a good idea.
Patch attached for the 8.x branch, it should backport fairly easily.
Comment #71
Steven Jones CreditAttribution: Steven Jones commentedUpdating tags.
Comment #72
pillarsdotnet CreditAttribution: pillarsdotnet commentedYou 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".
Comment #73
c960657 CreditAttribution: c960657 commentedThis looks good.
A few nits:
We generally don't add comments like that in core.
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.
Comment #74
pillarsdotnet CreditAttribution: pillarsdotnet commentedRe-rolled with suggestions and some additional changes to avoid unnecessarily long code lines.
Comment #75
pillarsdotnet CreditAttribution: pillarsdotnet commentedBah! Sorry about the mixup. Here's the patch I meant to upload, and an interdiff from #70.
Comment #76
pillarsdotnet CreditAttribution: pillarsdotnet commentedSorry; missed that bit. Re-rolled accordingly.
Comment #77
swentel CreditAttribution: swentel commentedSo 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.
Comment #78
Steven Jones CreditAttribution: Steven Jones commentedIf you provide a list of the other uses of autocomplete in core, we can get those fixed too.
Comment #79
swentel CreditAttribution: swentel commentedWell, 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.
Comment #80
Steven Jones CreditAttribution: Steven Jones commentedThere 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?
Comment #81
c960657 CreditAttribution: c960657 commentedI 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:
Even though this formatting is easy to read, the convention is write it all on one line and skip the $message variable.
Comment #82
pillarsdotnet CreditAttribution: pillarsdotnet commentedActually, the (new) convention is to remove t() from assertion tests. See #500866: [META] remove t() from assert message
Comment #83
Steven Jones CreditAttribution: Steven Jones commented@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.
Comment #84
Steven Jones CreditAttribution: Steven Jones commentedAttached patch is #76 plus a fix for:
Comment #85
coolestdude1 CreditAttribution: coolestdude1 commentedLooking forward to the D7 back port. Sub-ing
Comment #86
grimbones CreditAttribution: grimbones commentedI would like to see this backported to D7 as well and maybe back to D6.
Comment #87
c960657 CreditAttribution: c960657 commentedComment #88
rv0 CreditAttribution: rv0 commentedsub + D7 backport
Comment #89
webchickThis 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
Comment #90
Steven Jones CreditAttribution: Steven Jones commentedIt'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.Comment #91
c960657 CreditAttribution: c960657 commentedOne 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.
Comment #92
Steven Jones CreditAttribution: Steven Jones commented#84: fix_autocompletion_with_slashes-93854-76.patch queued for re-testing.
Comment #93
geek-merlini support #91: errors should be errors
i spent too much hours with errors that are silently ignored...
Comment #94
indigoparadox CreditAttribution: indigoparadox commentedSubscribing in eager anticipation of a Drupal 7 backport.
Comment #95
longwaveSubscribing. 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:
Comment #96
tarmstrong CreditAttribution: tarmstrong commentedI'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.
Comment #97
pillarsdotnet CreditAttribution: pillarsdotnet commentedRe-rolled with change suggested in #89/91/93/95
Comment #99
pillarsdotnet CreditAttribution: pillarsdotnet commented@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.
Comment #100
e2thex CreditAttribution: e2thex commentedAttch patch is same as in #99 but works with drush make
Comment #102
pillarsdotnet CreditAttribution: pillarsdotnet commented(sigh) Re-uploading the patch in #99.
Comment #103
modstore CreditAttribution: modstore commentedsubscribing.
Comment #104
webchickWe need sign-off from sun/chx on this. This is really, really weird.
Comment #105
webchickComment #106
grendzy CreditAttribution: grendzy commentedIt looks like drupal_valid_path would accept
$element['#autocomplete_path'] . '/%'
, instead of 'dummy'. Thoughts?Comment #107
geek-merlin#97: fix_autocompletion_with_slashes-93854-96.patch queued for re-testing.
(still can't imagine how this fails.
Comment #108
pillarsdotnet CreditAttribution: pillarsdotnet commented@#106 -- okay; testing.
Comment #109
geek-merlin@webchick: as for your WTF in #89 about the line
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.
Comment #110
xjm#109: Lots of work has been done since #89. See #108.
Marking NR for #108.
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?
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.
Comment #111
geek-merlin>#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.
Comment #112
marcb CreditAttribution: marcb commentedUsing 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)
Comment #113
pillarsdotnet CreditAttribution: pillarsdotnet commentedPlease 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.
Comment #114
ericduran CreditAttribution: ericduran commentedCan we get a summary on this issue?
Comment #115
ericduran CreditAttribution: ericduran commentedI'm confused as to why everything else is happening here.
I did a quick test and by doing:
This fix the slash issue.
Can someone explain to me, what the rest of the patch is attempting to do.
Thanks.
Comment #116
pillarsdotnet CreditAttribution: pillarsdotnet commentedThe 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 definetaxonomy/autocomplete/%/%menu_tail
instead 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.inc
andtaxonomy.module
, we have the following patch:Comment #117
webchickYay, that patch looks MUCH better!!
Comment #118
ericduran CreditAttribution: ericduran commentedYea, 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.
Comment #119
ericduran CreditAttribution: ericduran commentedHere'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.
Comment #120
ericduran CreditAttribution: ericduran commentedLol, wrong patch, --ignore that.
I meant to upload this one.
Comment #121
ericduran CreditAttribution: ericduran commentedComment #122
pillarsdotnet CreditAttribution: pillarsdotnet commentedRe-uploading the patch from #108, with a clarifying comments added.
Comment #123
ericduran CreditAttribution: ericduran commented@pillarsdotnet, thats not right,
The above code isn't correct. This pretty much breaks autocomplete everywhere.
Comment #124
pillarsdotnet CreditAttribution: pillarsdotnet commentedHere's an interdiff between #108 and #122. Judge for yourself what changed.
Comment #124.0
pillarsdotnet CreditAttribution: pillarsdotnet commentedUpdated summary from template.
Comment #124.1
pillarsdotnet CreditAttribution: pillarsdotnet commentedInclude link to the correct patch.
Comment #125
ericduran CreditAttribution: ericduran commentedI get the difference, but #108 is also incorrect.
Comment #126
ericduran CreditAttribution: ericduran commentedHere's a patch,
This is very similar to the other patches but also different.
Here's what this patch does:
Comment #127
pillarsdotnet CreditAttribution: pillarsdotnet commentedOkay, if it works, but leave the extra comments in, at least.
Comment #127.0
pillarsdotnet CreditAttribution: pillarsdotnet commentedCorrection to comment number.
Comment #127.1
pillarsdotnet CreditAttribution: pillarsdotnet commentedUpdated links to latest patch.
Comment #127.2
pillarsdotnet CreditAttribution: pillarsdotnet commentedRe-order for clarity.
Comment #128
Steven Jones CreditAttribution: Steven Jones commentedHappy 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:
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.
Comment #129
pillarsdotnet CreditAttribution: pillarsdotnet commentedBetter yet, check the test results in a way that does not depend on a specific result order.
Comment #130
xjmTag cleanup.
Comment #131
pillarsdotnet CreditAttribution: pillarsdotnet commented@#128 by Steven Jones on October 18, 2011 at 8:30am:
Pretty close. Here's the interdiff.
Comment #132
pillarsdotnet CreditAttribution: pillarsdotnet commentedRe-arranged and added missing comments to make the interdiff smaller.
Comment #133
xjmTagging 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:
This should begin, "Tests term autocompletion..." (We are currently cleaning this up in #1310084: [meta] API documentation cleanup sprint.)
It would probably be better to spell out "First" here.
Comment #134
mstrelan CreditAttribution: mstrelan commentedRe-roll of #132 to place files in the core directory and incorporate changes from #133.
Comment #135
xjmThanks 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.
Comment #137
xjmTest failed as expected.
Comment #138
xjmNow 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/add
to 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.
Comment #138.0
xjmwordsmithing.
Comment #139
xjmUpdated summary.
Comment #140
mstef CreditAttribution: mstef commented#134 works for me.
Comment #141
ericduran CreditAttribution: ericduran commentedThis 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.
Comment #142
xjmThanks @ericduran! The screenshots are a big help. This feels RTBC to me too. :)
Comment #143
chx CreditAttribution: chx commented%menu_tail yay. looks good.
Comment #144
thinkyhead CreditAttribution: thinkyhead commentedDoes 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.
Comment #145
drupalnesia CreditAttribution: drupalnesia commentedNeed back ported to D6, see http://drupal.org/node/1088948
Comment #146
Steven Jones CreditAttribution: Steven Jones commentedUsing %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?
Comment #147
xjm#146: Nothing. It's marked RTBC.
Comment #148
catchLooks good here, committed/pushed to 8.x. Will need a re-roll for 7.x.
Comment #149
xjmHere'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.
Comment #150
xjmComment #151
damien_vancouver CreditAttribution: damien_vancouver commentedPatch from #149 applies cleanly, fixes the issue for me on 7.9. +1
Comment #152
Anonymous (not verified) CreditAttribution: Anonymous commentedI 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.
Comment #153
Steven Jones CreditAttribution: Steven Jones commented@linclark that's exactly what this issue should fix. What browser are you seeing this error with?
Comment #154
xjmYeah, 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?
Comment #155
Anonymous (not verified) CreditAttribution: Anonymous commentedOK, 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.
Comment #156
Dave ReidSo 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.
Comment #157
xjmWell, it's the same callback for both paths.
Comment #158
Steven Jones CreditAttribution: Steven Jones commentedSee http://drupal.org/node/93854#comment-4895132 for why two callbacks are needed if you want to have a callback that includes %menu_tail
Comment #159
xjmThough, 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?Comment #160
Dave ReidSure, 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?Comment #161
Dave ReidComment #161.0
Dave ReidUpdated issue summary.
Comment #161.1
xjmUpdated issue summary.
Comment #162
Steven Jones CreditAttribution: Steven Jones commentedSo are we saying that we need to fix the implementation that got into Drupal 8?
Comment #163
Dave ReidWe 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.
Comment #164
xjmWhich 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..
Comment #165
xjmHere's a patch to roll it back in D8. *sniffle*
Comment #166
Dave ReidReady for rollback. Let's do this simpler once this is rolled back. For catch or Dries, this should be easy to do with just:
Comment #167
catchOK, rolled this back. Back to CNW I think?
Comment #168
thinkyhead CreditAttribution: thinkyhead commentedI'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.
Comment #169
c960657 CreditAttribution: c960657 commentedWhy 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.
Comment #170
Steven Jones CreditAttribution: Steven Jones commentedLooking 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.
Comment #171
mstef CreditAttribution: mstef commentedRerolled #134 to apply to drupal root (7.x) - needed for drush make.
Comment #172
sixelats CreditAttribution: sixelats commentedPatch in #171 worked for me (v7.10)
Thanks!
Comment #173
danielnolde CreditAttribution: danielnolde commentedPatch fix-autocompletion-with-slashes-93854-171.patch from #171 worked for me, too (Drupal 7).
Comment #174
ptrl CreditAttribution: ptrl commentedPatch in #171 solves my problem with slashes in autocompletes for Drupal 7.
Comment #175
mxh#171 not working for me (D7.12), still retrieving AJAX 404.
Comment #176
danielb CreditAttribution: danielb commented#175 - how did you determine this?
Comment #177
c960657 CreditAttribution: c960657 commentedAny comments on my suggestion in #169? I think we try to solve this problem in a much too complicated way.
Comment #178
Steven Jones CreditAttribution: Steven Jones commentedRight, 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.
Comment #179
c960657 CreditAttribution: c960657 commented@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.
Comment #180
xjmActually, 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.
Comment #181
Steven Jones CreditAttribution: Steven Jones commentedRight, 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).Comment #181.0
Steven Jones CreditAttribution: Steven Jones commentedUpdated issue summary.
Comment #182
yannickooThat looks good, when will the patch be commited?
Comment #183
xjmt()
from the assertion message texts. Reference: http://drupal.org/simpletest-tutorial-drupal7#tTagging novice for two tasks: manual testing in various browsers, and expanding the test coverage . See #138 for suggestions of what to test.
Comment #184
Steven Jones CreditAttribution: Steven Jones commentedHere's #181, minus
t
and just the tests.Comment #185
Steven Jones CreditAttribution: Steven Jones commentedDave'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.Comment #186
xjmBit of trailing whitespace here. Also, it looks like the latest test-only patch did not apply.
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!
Comment #187
Steven Jones CreditAttribution: Steven Jones commentedLame, 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.
Comment #188
Steven Jones CreditAttribution: Steven Jones commentedHere is the patch with more tests, with the whitespace issue fixed.
Comment #189
Steven Jones CreditAttribution: Steven Jones commentedRight, apologies for the confusing of patches we have:
Apologies for the noise and all the different patches, hopefully that will clear things up.
Comment #189.0
Steven Jones CreditAttribution: Steven Jones commentedUpdated issue summary.
Comment #190
ryan.gibson CreditAttribution: ryan.gibson commentedOkay, 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.
Comment #191
ZenDoodles CreditAttribution: ZenDoodles commentedComment #192
Steven Jones CreditAttribution: Steven Jones commented@ryanissamson to confirm, the patch you applied was the one in #188?
Comment #193
ryan.gibson CreditAttribution: ryan.gibson commentedSorry, I should have clarified. That is correct, I applied the patch from #188.
Comment #194
Steven Jones CreditAttribution: Steven Jones commented@ryanissamson Thanks for the patch, I will try to work on this in a bit.
Comment #195
Steven Jones CreditAttribution: Steven Jones commentedOdd, 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.
Comment #196
ryan.gibson CreditAttribution: ryan.gibson commentedYou 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.
Comment #197
Steven Jones CreditAttribution: Steven Jones commentedI honestly can't thank you enough for testing. Whatever the outcome!
Comment #198
ryan.gibson CreditAttribution: ryan.gibson commentedAha! 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!
Comment #199
xjmThanks @ryanissamson and @Steven Jones. We still need IE testing though. :)
Comment #200
Steven Jones CreditAttribution: Steven Jones commentedOkay, well I've checked, and it works in:
I'm not going to mark it as RTBC because it's my own patch.
Comment #201
xjmYay!
Comment #202
aspilicious CreditAttribution: aspilicious commentedAfter 5 years... VICTORY
Comment #203
moonray CreditAttribution: moonray commentedHah! Finally!
And to think the first response was "this should be a simple fix.."
Comment #204
moonray CreditAttribution: moonray commented[duplicate post, contents removed]
Comment #205
catchThis 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.
Comment #206
Steven Jones CreditAttribution: Steven Jones commented@catch Thanks for the commit!
I am all over this one.
Comment #207
Steven Jones CreditAttribution: Steven Jones commentedHere's the patch for D7.
Comment #207.0
Steven Jones CreditAttribution: Steven Jones commentedUpdated issue summary.
Comment #207.1
Steven Jones CreditAttribution: Steven Jones commentedUpdated issue summary.
Comment #208
xjmBackport looks good!
Comment #209
webchickAWESOME! 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.
Comment #210
Steven Jones CreditAttribution: Steven Jones commentedI'll do that backport too, guessing the tests get dropped in this one.
Comment #211
webchickD6 does have some kind of test coverage, but I'm not sure exactly what or how it works.
Comment #212
Dave ReidTests need to just be dropped completely for D6 backports.
Comment #213
Steven Jones CreditAttribution: Steven Jones commentedAnd here is the fix for D6.
Comment #214
danielb CreditAttribution: danielb commentedThe 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?
Comment #215
xjm#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?Comment #216
Steven Jones CreditAttribution: Steven Jones commented@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.Comment #217
xjmI just meant a generic helper of this.
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
Comment #218
Steven Jones CreditAttribution: Steven Jones commentedMy follow up on getting menu tail load to a useful point is over here: #1492464: Make menu tail load work without specifying the loader arguments.
Comment #219
mxhDrupal 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.
Comment #220
xjm#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.
Comment #221
JvE CreditAttribution: JvE commentedI just created a test in #1559142: Allow autocompletion terms to start with a slash for the failing case of terms starting with slashes.
Comment #222
panche CreditAttribution: panche commentedI 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
Comment #223
panche CreditAttribution: panche commentedThis patch worked for me:
At line 273 of misc/autocomplete.js add:
Comment #224
Steven Jones CreditAttribution: Steven Jones commented@panche Please open a new issue for Drupal 8 with that bug please.
Comment #225
panche CreditAttribution: panche commentedWill do.
Comment #226
panche CreditAttribution: panche commented@Steven Jones http://drupal.org/node/1661098
Sorry, first timer.
Comment #227
mindbat CreditAttribution: mindbat commentedPatch 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?
Comment #228
mxh@mindbat
It could be a Node Reference problem. Would you please try Entity Reference for your case and report back here?see #229.
Comment #229
rv0 CreditAttribution: rv0 commented@mindbat
You need 7.13 at least
Comment #230
panche CreditAttribution: panche commented@mindbat Here's a better solution:
#581706: Protect .git, .hg and .bzr directories in .htaccess See comment 34
Comment #231
mindbat CreditAttribution: mindbat commented@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?
Comment #232
mindbat CreditAttribution: mindbat commented@panche: Thanks for the suggestion. Unfortunately, didn't fix the issue for me :(
Comment #233
effulgentsia CreditAttribution: effulgentsia commentedFor 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.
Comment #234
effulgentsia CreditAttribution: effulgentsia commentedAlso, 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?
Comment #235
hellolindsay CreditAttribution: hellolindsay commentedI 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:
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:
So it looks like these two pieces of logic are in conflict.
Comment #236
Pere OrgaI also think the issue was not properly fixed. In Drupal 7.17 if I include slashes in my search I'm getting wrong results:
Comment #237
tjg CreditAttribution: tjg commentedThis is still an issue in 7.22. Is there any chance of getting this fixed, or should I patch my own code?
Comment #238
roeldl CreditAttribution: roeldl commentedThis is still an issue in 7.23, can this please be properly fixed?
Comment #239
klonos...to avoid changing versions all the time ;)
Comment #240
thinkyhead CreditAttribution: thinkyhead commentedSo 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
Comment #241
mrfree CreditAttribution: mrfree commentedI 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"
Comment #241.0
mrfree CreditAttribution: mrfree commentedUpdated issue summary.
Comment #242
ndobromirov CreditAttribution: ndobromirov commentedWhat 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! :)
Comment #243
ndobromirov CreditAttribution: ndobromirov commentedForget to change the status!
Comment #245
ndobromirov CreditAttribution: ndobromirov commentedThere was a silly mistake in the previous patch.
Uploading a new one.
Comment #247
ndobromirov CreditAttribution: ndobromirov commentedThe 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.
Comment #249
ndobromirov CreditAttribution: ndobromirov commentedFixed typos.
Comment #251
JvE CreditAttribution: JvE commentedThis is not the correct place to put the slash back. drupal_explode_tags is a general function used by a lot more than taxonomy.
The definition of encodePath is encodeURIComponent(item).replace(/%2F/g, '/') so I do not see how it would return any %2F to replace.
Taxonomy is not the only module using autocomplete. There are many others. None of them expect [--SLASH--] in their input.
Comment #252
ndobromirov CreditAttribution: ndobromirov commentedAdded utility method drupal_escape_autocomplete_input to provide contributed modules to generate autocomplete paths.
Fixed some small issues within patch in #249.
Comment #254
ndobromirov CreditAttribution: ndobromirov commentedThanks 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.
Comment #255
webbykat CreditAttribution: webbykat commentedUnrelated 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.
Comment #256
thinkyhead CreditAttribution: thinkyhead commentedI'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!
Comment #257
pwolanin CreditAttribution: pwolanin as a volunteer and at Acquia commentedThis is fixed in 8 by moving the search string to a query string where it properly belongs.
Comment #258
pwolanin CreditAttribution: pwolanin as a volunteer and at Acquia commentedHere's a quick patch for 6 to backport for just taxonomy terms the fix that's in 7 core.
Comment #264
poker10 CreditAttribution: poker10 at ActivIT s.r.o. commentedThis was fixed in D7 in 2012 and it is fixed in D8+ as well. If there are still problems in D7, please open a new issue so we can take a look. Closing this as Fixed.