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

  1. Add a few terms containing commas to an existing vocabulary. For example, "Ellington, Duke", "Armstrong, Louis", "Coltrane, John".
  2. 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.
  3. Try typing text contained in one of the terms entered above, e.g. 'ell' . Nothing happens.
  4. 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:

<?php
$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:

<?php
   
// 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:

<?php
    $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:

<?php
    $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);
    }
?>
Files: 
CommentFileSizeAuthor
#36 taxonomy-Term-reference-autocomplete-widget-having-problems-h-1000736-34.full_.patch4.2 KBcatch
PASSED: [[SimpleTest]]: [MySQL] 33,592 pass(es).
[ View ]
#34 taxonomy-Term-reference-autocomplete-widget-having-problems-h-1000736-34.full_.patch4.2 KBfangel
PASSED: [[SimpleTest]]: [MySQL] 33,589 pass(es).
[ View ]
#34 taxonomy-Term-reference-autocomplete-widget-having-problems-h-1000736-34.test-only.patch2.8 KBfangel
FAILED: [[SimpleTest]]: [MySQL] 33,581 pass(es), 2 fail(s), and 0 exception(es).
[ View ]
#31 taxonomy-Term-reference-autocomplete-widget-having-problems-h-1000736-31.patch4.35 KBfangel
PASSED: [[SimpleTest]]: [MySQL] 33,560 pass(es).
[ View ]
#29 taxonomy_autocomplete_comma-1000736-29.patch2.48 KBstBorchert
PASSED: [[SimpleTest]]: [MySQL] 33,559 pass(es).
[ View ]
#26 taxonomy-Term-reference-autocomplete-widget-having-problems-handling-terms-with-commas-1000736-26.patch1.38 KBfangel
PASSED: [[SimpleTest]]: [MySQL] 33,537 pass(es).
[ View ]
#22 taxonomy-Term-reference-autocomplete-widget-having-problems-handling-terms-with commas-1000736-21.patch1.38 KBfangel
FAILED: [[SimpleTest]]: [MySQL] Fetch test patch: failed to retrieve [taxonomy-Term-reference-autocomplete-widget-having-problems-handling-terms-with commas-1000736-21.patch] from [drupal.org].
[ View ]
#20 taxonomy-Term-reference-autocomplete-widget-having-problems-handling-terms-with commas-1000736-20.patch1.31 KBfangel
FAILED: [[SimpleTest]]: [MySQL] Fetch test patch: failed to retrieve [taxonomy-Term-reference-autocomplete-widget-having-problems-handling-terms-with commas-1000736-20_0.patch] from [drupal.org].
[ View ]
#16 taxonomy_autocomplete_comma-1000736-15.patch1.32 KBstBorchert
PASSED: [[SimpleTest]]: [MySQL] 29,930 pass(es).
[ View ]

Comments

Version:7.0-rc2» 7.x-dev
Priority:Normal» Major

Confirming, and again a regression from 6.

This line should probably be done with drupal_implode_tags().

$prefix = count($tags_typed) ? implode(', ', $tags_typed) . ', ' : '';

Really all this still doesn't work ... and it is needed to

Ah no ... sorry. I made I mistake in pasting the code. It works !!!

What is needed to make sure this is fixed in the next core release and there is no further regression? a patch and several tests?

A patch would be a good start; tests would be bonus.

Hi 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')

No, 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 ;)

Hi 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?

You might have a point -- there may be an inconsistency. But please file a new issue for this.

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

This 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!

subscribing

Subscribing.

Status:Needs work» Needs review
StatusFileSize
new1.32 KB
PASSED: [[SimpleTest]]: [MySQL] 29,930 pass(es).
[ View ]

Here 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.)

Status:Active» Needs work

The last submitted patch, taxonomy_autocomplete_comma-1000736-15.patch, failed testing.

Issue tags:+needs backport to D7

.

StatusFileSize
new1.31 KB
FAILED: [[SimpleTest]]: [MySQL] Fetch test patch: failed to retrieve [taxonomy-Term-reference-autocomplete-widget-having-problems-handling-terms-with commas-1000736-20_0.patch] from [drupal.org].
[ View ]

I 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.

Status:Needs review» Needs work

StatusFileSize
new1.38 KB
FAILED: [[SimpleTest]]: [MySQL] Fetch test patch: failed to retrieve [taxonomy-Term-reference-autocomplete-widget-having-problems-handling-terms-with commas-1000736-21.patch] from [drupal.org].
[ View ]

Sorry, didn't mean to remove Stefan from the author list of the patch. Re-rolled patch with updated author-information.

Status:Needs work» Needs review

Let's get the bot to look at it :)

Status:Needs review» Needs work

Looks like the bot didn't like spaces in the filename?

StatusFileSize
new1.38 KB
PASSED: [[SimpleTest]]: [MySQL] 33,537 pass(es).
[ View ]

Bah. I must have missed one. Here it is again, no spaces.. :)

Status:Needs work» Needs review

Issue tags:+Needs tests

Patch looks good. There should be an existing test case for tagging that we could add to for this in taxonomy.test

StatusFileSize
new2.48 KB
PASSED: [[SimpleTest]]: [MySQL] 33,559 pass(es).
[ View ]

Something like this?

stBorchert: 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 :)

StatusFileSize
new4.35 KB
PASSED: [[SimpleTest]]: [MySQL] 33,560 pass(es).
[ View ]

Okay, 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.

Status:Needs review» Needs work

There's a few coding style issues in the patch but it otherwise looks good.

+    $term_objs = taxonomy_get_tree($this->vocabulary->vid);

Should be $term_objects or just $terms - Drupal generally doesn't abbreviate.

+    $term1 = $term2 = $term3 = NULL;

No need to assign these to NULL, they'll be assigned in the foreach loop.

+    foreach ($term_objs AS $term) {

AS should not be capitalized - just 'as'.

+      ${'term' . (array_search($term->name, $terms)+1)} = $term;

+ }

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.

catch: 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.

Status:Needs work» Needs review
StatusFileSize
new2.8 KB
FAILED: [[SimpleTest]]: [MySQL] 33,581 pass(es), 2 fail(s), and 0 exception(es).
[ View ]
new4.2 KB
PASSED: [[SimpleTest]]: [MySQL] 33,589 pass(es).
[ View ]

Okay, 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.

Status:Needs review» Needs work

Status:Needs work» Reviewed & tested by the community
Issue tags:-Needs tests
StatusFileSize
new4.2 KB
PASSED: [[SimpleTest]]: [MySQL] 33,592 pass(es).
[ View ]

Patch 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.

Status:Reviewed & tested by the community» Fixed

Great 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!

Status:Fixed» Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.

In 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.

Version:8.x-dev» 7.23
Status:Closed (fixed)» Needs work

I caught this problem again on fresh Drupal 7.23 installation.

Steps to Reproduce:

  • Create term with quotes into name
  • Add a autocomplete field to a entity
  • Try to fill this field

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

Version:7.23» 8.x-dev
Status:Needs work» Closed (fixed)