Problem/Motivation

The API function taxonomy_implode_tags() is passed an array of term objects and returns an imploded string to provide the default value of a taxonomy field. However, the function modifies the object's $term->name property, rather than creating a copy:

        if (strpos($tag->name, ',') !== FALSE || strpos($tag->name, '"') !== FALSE) {
          $tag->name = '"' . str_replace('"', '""', $tag->name) . '"';
        }

Proposed resolution

Don't modify $term->name; instead, save the desired value to the list directly. Patch in #3 implements this change.

Remaining tasks

None. Taxonomy maintainer has approved the patch.

User interface changes

None.

API changes

None.

Original report by @BayerMeister

Calling the function taxonomy_implode_tags not only returns imploded tags, but also modifies the array it has as a parameter.

What are the steps required to reproduce the bug?
It suffices to call the function twice with an array containing one tag like "John Smith, PhD." (containing a comma or a double-quote).
The first call modifies the input array so that the tag is additionally surrounded with double quotes.
The second call receives an array containing ""John Smith, PhD."". This is an unwanted and undocumented side-effect which is trivial to fix

What behavior were you expecting?
Just as documented: Should return something, takes something as parameter (implicitly does not modify).

What happened instead?
Modifies input.

I'm attaching a simple patch. This is the same for D6 except line numbers and a few differences in context.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

xjm’s picture

Status: Needs work » Needs review

Tracked down this bug today after beating my head on #1232686: TAC causes term reference autocomplete values containing commas to be triple-quoted. The patch looks like it should resolve the issue without any side effects.

Status: Active » Needs work

The last submitted patch, noTagModify.patch, failed testing.

xjm’s picture

Status: Needs review » Needs work
FileSize
746 bytes

Old patch format. Here's a reroll in the new format.

xjm’s picture

Version: 7.x-dev » 8.x-dev
Status: Needs work » Needs review
Issue tags: +Needs backport to D7
xjm’s picture

Title: function taxonomy_implode_tags has side effects » taxonomy_implode_tags() modifies term objects (instead of cloning) and adds extra " when called multiple times
sun’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: +Needs backport to D6

Thanks - don't think this can or should be tested. Typical PHP5 OO drama.

xjm’s picture

Issue summary: View changes

Added summary.

catch’s picture

Looks good.

catch’s picture

Issue summary: View changes

Added link to patch.

xjm’s picture

Dries’s picture

Status: Reviewed & tested by the community » Fixed

Committed to 7.x and 8.x. Good catch.

andypost’s picture

Version: 8.x-dev » 6.x-dev
Status: Fixed » Needs review
FileSize
725 bytes

Re-roll for 6.x

valthebald’s picture

Status: Needs review » Reviewed & tested by the community

Good in D6 too

valthebald’s picture

Issue summary: View changes

Updated issue summary.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 10: 1078398-taxo-tags.patch, failed testing.

valthebald’s picture

jyotisankar’s picture

Status: Needs work » Needs review
FileSize
727 bytes
valthebald’s picture

Status: Needs review » Needs work
Issue tags: -Needs reroll

Small formatting fix: please remove extra space on line 1400 (else clause)

jyotisankar’s picture

Status: Needs work » Needs review
FileSize
725 bytes

Extraspace removed.

valthebald’s picture

Status: Needs review » Reviewed & tested by the community

Looks good!

Status: Reviewed & tested by the community » Closed (outdated)

Automatically closed because Drupal 6 is no longer supported. If the issue verifiably applies to later versions, please reopen with details and update the version.