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:

$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);
    }
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

joachim’s picture

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

Confirming, and again a regression from 6.

joachim’s picture

This line should probably be done with drupal_implode_tags().

$prefix = count($tags_typed) ? implode(', ', $tags_typed) . ', ' : '';
itamair’s picture

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

itamair’s picture

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

magnusk’s picture

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?

joachim’s picture

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

mstrom81’s picture

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

joachim’s picture

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

mstrom81’s picture

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?

joachim’s picture

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

mstrom81’s picture

Steven Jones’s picture

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!

keithm’s picture

subscribing

worldlinemine’s picture

Subscribing.

worldlinemine’s picture

stBorchert’s picture

Status: Needs work » Needs review
FileSize
1.32 KB

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.

stBorchert’s picture

sun’s picture

Issue tags: +Needs backport to D7

.

fangel’s picture

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
fangel’s picture

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

joachim’s picture

Status: Needs work » Needs review

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

Status: Needs review » Needs work
joachim’s picture

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

fangel’s picture

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

fangel’s picture

Status: Needs work » Needs review
catch’s picture

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

stBorchert’s picture

Something like this?

fangel’s picture

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

fangel’s picture

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.

catch’s picture

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.

fangel’s picture

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.

fangel’s picture

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
catch’s picture

Status: Needs work » Reviewed & tested by the community
Issue tags: -Needs tests
FileSize
4.2 KB

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.

webchick’s picture

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.

mikeker’s picture

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.

mrded’s picture

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

mrded’s picture

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