Make WordPress Core

Opened 12 years ago

Closed 12 years ago

#20765 closed defect (bug) (fixed)

get_attachment_fields_to_edit() returns term->name rather than term->slug, causing conflicts between terms

Reported by: eddiemoya's profile eddiemoya Owned by: nacin's profile nacin
Milestone: 3.5 Priority: normal
Severity: normal Version: 3.3.2
Component: Media Keywords: has-patch commit
Focuses: Cc:

Description

When adding support for taxonomy to attachments, the media page will automatically add a simple text field for entering comma delimited list of terms. This works fine, and appears to handle slugs as well as names entered.

However, once saved and re-opened, the attachment field will output only term names. This leads to conflicts in situations where users have entered two slugs for different terms which share the same name.

This field should be returning slugs or ID's, preferably slugs as they are more human-readable, and unlike the category metaboxes this depends on manual entry.

This bug was found to be the cause of a bug reported on one of my plugins

Attachments (2)

attachment-fields-to-edit.patch (378 bytes) - added by eddiemoya 12 years ago.
Changes term->name to term->slug in attachment_fields_to_edit() function in /wp-admin/includes/media.php
20765.patch (359 bytes) - added by SergeyBiryukov 12 years ago.

Download all attachments as: .zip

Change History (9)

@eddiemoya
12 years ago

Changes term->name to term->slug in attachment_fields_to_edit() function in /wp-admin/includes/media.php

#1 @eddiemoya
12 years ago

To be clear, the conflict is seen and data is lost when two terms of the same name are applied to an attachment and saved, then re-opened, and saved once again. Because the media page renders outputs the term names, it will always assume that the first term is finds with that name is the correct one.

There is no way through javascript or otherwise, to fix this, since there is no way to know which was the originally intended term. I've managed to work around this in my plugin by filtering the function that does this, duplicating all the code, except switching it to slugs instead.

http://plugins.trac.wordpress.org/changeset/549552/media-categories-2/tags/1.3.1/media-categories.php

#2 @eddiemoya
12 years ago

  • Version set to 3.3.2

I've posted about this issue on my blog which includes step-by-step directions on recreating this bug.

#3 @eddiemoya
12 years ago

  • Version changed from 3.3.2 to 3.4.1

#4 follow-up: @SergeyBiryukov
12 years ago

  • Milestone changed from Awaiting Review to 3.5
  • Version changed from 3.4.1 to 3.3.2

Version number indicates when the bug was initially introduced/reported.

Reproduced with the steps from comment:2. The code was introduced in [6659].

Considering backwards compatibility, would it make more sense to pass the whole $terms array in a separate key?

Related: #21391

#5 in reply to: ↑ 4 @eddiemoya
12 years ago

Sorry about setting the version number wrong, I have not done many patches.

Replying to SergeyBiryukov:

Considering backwards compatibility, would it make more sense to pass the whole $terms array in a separate key?

I dont know if it makes sense in this case to preserve backward compatibility in this particular case - any plugins (including earlier versions of mine) that rely on this would themselves also be broken. We'd be preserving broken behavior for the sake of backward compatibility with plugins that were, by extension, also broken.

If this were an enhancement, or a new feature then yes. However this is just broken - the category field using names can't work as they are, and they never did.

Thanks for looking into this.

#6 @nacin
12 years ago

  • Keywords commit added

Looks good.

#7 @nacin
12 years ago

  • Owner set to nacin
  • Resolution set to fixed
  • Status changed from new to closed

In [21889]:

Return term slugs rather than term names in attachment_fields_to_edit() as that is what we are dealing with. props eddiemoya, fixes #20765.

Note: See TracTickets for help on using tickets.