Problem
When using the term reference autocomplete widget, terms containing commas do not show up in autocomplete results when they should. In addition, trying to quote terms in the widget can cause the widget to throw javascript errors.
Environment
Drupal 7, rc2. Php 5.2.6 on Debian Lenny.
To reproduce
- Add a few terms containing commas to an existing vocabulary. For example, "Ellington, Duke", "Armstrong, Louis", "Coltrane, John".
- Add a Term reference field to a content type and have it use an autocomplete term widget. Have the widget reference the vocabulary used above, and have it allow an unlimited number of values.
- Try typing text contained in one of the terms entered above, e.g. 'ell' . Nothing happens.
- Furthermore, try typing text beginning with a quote, e.g. '"ell'. This time you get a javascript error.
Expected behavior
What should happen is that you get quoted terms which contain the text you type.
Proposed fix
It looks like the problem is in the 'taxonomy_autocomplete' function of taxonomy.pages.inc, in two places.
Problem #1
The first problem comes in this line:
$prefix = count($tags_typed) ? implode(', ', $tags_typed) . ', ' : '';
This will cause $prefix to mix unquoted tags. For example, if $tags_typed was originally '"Ellington, Duke", "Coltrane, John"' (two terms), it would become 'Ellington, Duke, Coltrane, John' (four terms), which is not correct.
One way to fix this would be to quote the tags before using in them the prefix. I did this by using this code:
// Bad! >:(
//$prefix = count($tags_typed) ? implode(', ', $tags_typed) . ', ' : '';
// Good! :)
$quoted_tags_typed = array();
foreach ($tags_typed as $tag){
if (strpos($tag, ',') !== FALSE || strpos($tag, '"') !== FALSE) {
$tag = '"' . str_replace('"', '""', $tag) . '"';
}
$quoted_tags_typed[] = $tag;
}
$prefix = count($quoted_tags_typed) ? implode(', ', $quoted_tags_typed) . '\
, ' : '';
Problem #2
The second problem occurs at this section:
$term_matches = array();
foreach ($tags_return as $tid => $name) {
$n = $name;
// Term names containing commas or quotes must be wrapped in quotes.
if (strpos($name, ',') !== FALSE || strpos($name, '"') !== FALSE) {
$n = '"' . str_replace('"', '""', $name) . '"';
}
else {
$term_matches[$prefix . $n] = check_plain($name);
}
}
It looks like the intention of this section was to quote autocomplete suggestions which contained commas or quotes. However, the quoted term never gets added to the list of matches because of the 'else'.
I fixed this problem by taking out the else, like this:
$term_matches = array();
foreach ($tags_return as $tid => $name) {
$n = $name;
// Term names containing commas or quotes must be wrapped in quotes.
if (strpos($name, ',') !== FALSE || strpos($name, '"') !== FALSE) {
$n = '"' . str_replace('"', '""', $name) . '"';
}
// Bad! >:(
//else {
// $term_matches[$prefix . $n] = check_plain($name);
//}
// Good! :)
$term_matches[$prefix . $n] = check_plain($name);
}
Comments
Comment #1
joachim CreditAttribution: joachim commentedConfirming, and again a regression from 6.
Comment #2
joachim CreditAttribution: joachim commentedThis line should probably be done with drupal_implode_tags().
Comment #3
itamair CreditAttribution: itamair commentedReally all this still doesn't work ... and it is needed to
Comment #4
itamair CreditAttribution: itamair commentedAh no ... sorry. I made I mistake in pasting the code. It works !!!
Comment #5
magnusk CreditAttribution: magnusk commentedWhat is needed to make sure this is fixed in the next core release and there is no further regression? a patch and several tests?
Comment #6
joachim CreditAttribution: joachim commentedA patch would be a good start; tests would be bonus.
Comment #7
mstrom81 CreditAttribution: mstrom81 commentedHi there,
I noticed another bug with the widget. Let's say the terms I have in the taxonomy are Architecture and Basketball. After typing in "a", you would expect to only see Architecture, however Basketball also shows up. That's because there's an issue with the db query.
Within taxonomy.pages.inc, there's a method called "taxonomy_autocomplete". It has the following db query code:
// Select rows that match by term name.
$tags_return = $query
->fields('t', array('tid', 'name'))
->condition('t.vid', $vids)
->condition('t.name', '%' . db_like($tag_last) . '%', 'LIKE')
->range(0, 10)
->execute()
->fetchAllKeyed();
The issue is with the following line:
->condition('t.name', '%' . db_like($tag_last) . '%', 'LIKE')
It's looking for LIKE %tag_last%, where it should be looking for LIKE tag_last%.
->condition('t.name', db_like($tag_last) . '%', 'LIKE')
Comment #8
joachim CreditAttribution: joachim commentedNo, that's correct behaviour -- it searches for text within the term rather than its start. At any rate, it's not at all connected with this issue. Another bug should be filed as another bug ;)
Comment #9
mstrom81 CreditAttribution: mstrom81 commentedHi joachim,
I wasn't aware that it's supposed to work that way. Based on most use cases I've seen of autocomplete, it looks for terms beginning with whatever the user entered. It seems that most people wouldn't be used to the designed functionality of autocomplete in this widget. Any thoughts?
Comment #10
joachim CreditAttribution: joachim commentedYou might have a point -- there may be an inconsistency. But please file a new issue for this.
Comment #11
mstrom81 CreditAttribution: mstrom81 commentedOk, thanks!
http://drupal.org/node/1120144
Comment #12
Steven Jones CreditAttribution: Steven Jones commentedThis needs to get fixed in D8 first, it needs a patch, and it needs tests.
This is related to #93854: Allow autocompletion requests to include slashes, which has tests and a patch, so have a look at that one for inspiration!
Comment #13
keithm CreditAttribution: keithm commentedsubscribing
Comment #14
worldlinemine CreditAttribution: worldlinemine commentedSubscribing.
Comment #15
worldlinemine CreditAttribution: worldlinemine commentedMarked #321080: Usage of Quotes in tag in combination with taxonomy_autocomplete as a duplicate of this one.
Comment #16
stBorchertHere is a first patch based on the notes of adorsk and joachim.
(PIFR seems to have some problems right now so I expect it to fail.)Comment #18
stBorchert#16: taxonomy_autocomplete_comma-1000736-15.patch queued for re-testing.
Comment #19
sun.
Comment #20
fangel CreditAttribution: fangel commentedI too found this bug, and accidentally created a duplicate #1211788: Unable to autocomplete terms containing commas or quotes. (I was only looking in 7.x issues).
I've reviewed the patch from #16 and it looks great! It can be made one line shorter by using the same ternary-operator as was in the original file. (The attached patch is just that)
But a +1 for the patch from my side.
Comment #22
fangel CreditAttribution: fangel commentedSorry, didn't mean to remove Stefan from the author list of the patch. Re-rolled patch with updated author-information.
Comment #23
joachim CreditAttribution: joachim commentedLet's get the bot to look at it :)
Comment #25
joachim CreditAttribution: joachim commentedLooks like the bot didn't like spaces in the filename?
Comment #26
fangel CreditAttribution: fangel commentedBah. I must have missed one. Here it is again, no spaces.. :)
Comment #27
fangel CreditAttribution: fangel commentedComment #28
catchPatch looks good. There should be an existing test case for tagging that we could add to for this in taxonomy.test
Comment #29
stBorchertSomething like this?
Comment #30
fangel CreditAttribution: fangel commentedstBorchert: Yes, something like this. However, since $term2 isn't necessarily the second item in the $terms-array, your test sometimes passes without the fix (if $term2 != $terms[1]), sometimes fails (if $term2 == $term[1]).
Working on a better test :)
Comment #31
fangel CreditAttribution: fangel commentedOkay, here's a new patch with the fix from #26 + a updated test-case that ensures that
$term2 == $term[1]
, and tests that $term2 is autocompleted with quoting, and $term3 is autocompleted with no quoting.Comment #32
catchThere's a few coding style issues in the patch but it otherwise looks good.
Should be $term_objects or just $terms - Drupal generally doesn't abbreviate.
No need to assign these to NULL, they'll be assigned in the foreach loop.
AS should not be capitalized - just 'as'.
+ }
The dynamic variable names make this a bit hard to read. If you used taxonomy_term_load_multiple() instead of taxonomy_get_tree() you'd have the terms keyed by $tid which seems easier to deal with?
Or what was wrong with the list()?
If you re-roll, please upload a patch that only contains the tests as well as the full patch - it's good to see what the failures look like via the test bot.
Comment #33
fangel CreditAttribution: fangel commentedcatch: I just adapted the existing the existing test, which is why I retained _get_tree. The reason for why I changed it to dynamic names instead of a list()-call is that _get_tree wouldn't always return the terms in the same order as they were listed in $terms - so $term1 wasn't necessarily $term[0]. This non-determinism made it very hard to properly test it.
I can look into changing it into using _load_multiple and seeing if that makes it more easily readable.
I will create a new test tomorrow afternoon and submit both a updated-test-only patch and fix+test patch.
Comment #34
fangel CreditAttribution: fangel commentedOkay, back at work after a weeks vacation. Here's the updated patch - it's much simpler with the use of
taxonomy_term_load_multiple
.Attached is a full and a test-only patch.
Comment #36
catchPatch looks great, just the test bot likes to mark the issue needs work if the most recent patch fails, even if it's in the same comment as a passing one.
Re-uploading the full patch from #34 and marking RTBC. No credit on commit please.
Comment #37
webchickGreat work! Thanks for the tests. Touched up the formatting on the .test comments a bit, then committed/pushed to 8.x and 7.x. Thanks!
Comment #39
mikeker CreditAttribution: mikeker commentedIn case anyone comes across the issue looking for a solution that avoids the use of double-quotes, take a look at the Taxonomy Single Term module.
Comment #40
mrded CreditAttribution: mrded commentedI caught this problem again on fresh Drupal 7.23 installation.
Steps to Reproduce:
Actual Results: Widget will wrap each quote to another quotes.
Expected Results: Widget should insert term's name without extra quotes.
Please, take a look the video: http://d.pr/v/rk6y
Comment #41
mrded CreditAttribution: mrded commentedSorry, I've moved issue to #1211788: Unable to autocomplete terms containing commas or quotes.