WordPress.org

Make WordPress Core

Opened 4 years ago

Closed 4 months ago

#18828 closed defect (bug) (fixed)

function wp_get_object_terms() passes sql data into `wp_get_object_terms` filter for $taxonomies

Reported by: postpostmodern Owned by: boonebgorges
Milestone: 4.2 Priority: normal
Severity: normal Version: 2.8
Component: Taxonomy Keywords: dev-feedback needs-docs has-patch needs-testing
Focuses: Cc:

Description

In wp-includes/taxonomy.php, the return data is passed through the wp_get_object_terms filter using the sql-ready data for $taxonomies, meaning it is quoted and comma seperated, for example "'category', 'post_tag', 'post_format'". Even for one taxonomy, it has quotes added, it seems this would be easier to work with if it matched the data passed into the function more closely.

Attachments (7)

wp_get_object_terms.diff (2.1 KB) - added by postpostmodern 4 years ago.
function wp_get_object_terms() fixing $taxonomy for filter
taxonomy_wp_get_object_terms.php.patch (2.2 KB) - added by doublesharp 14 months ago.
wp_get_object_terms filter fix for $taxonomies and $object_ids
taxonomy_wp_get_object_terms.php.2.patch (2.2 KB) - added by doublesharp 14 months ago.
taxonomy_wp_get_object_terms corrected
18828.diff (2.5 KB) - added by MikeHansenMe 5 months ago.
Refreshed.
18828.2.diff (2.4 KB) - added by wonderboymusic 4 months ago.
18828.3.diff (2.2 KB) - added by boonebgorges 4 months ago.
18828.4.patch (874 bytes) - added by doublesharp 4 months ago.
Pass $object_id_array instead of $object_ids to filter

Download all attachments as: .zip

Change History (29)

@postpostmodern4 years ago

function wp_get_object_terms() fixing $taxonomy for filter

comment:1 @ryan4 years ago

An array would indeed be better, but we need to see how many plugins this change impacts.

comment:2 @nacin4 years ago

  • Keywords 3.4-early added
  • Milestone changed from Awaiting Review to Future Release
  • Version changed from 3.3 to 3.2

comment:3 @wonderboymusic23 months ago

  • Milestone changed from Future Release to 3.7

Let's figure out if we can do this

comment:4 follow-ups: @nacin22 months ago

  • Milestone changed from 3.7 to Future Release

We need to figure out:

  • If any plugins are using the wp_get_object_terms filter, and what they do when they handle $taxonomies.
  • When we broke this, or if it was always like this.

comment:5 in reply to: ↑ 4 @jdgrimes20 months ago

  • Keywords 3.4-early removed
  • Version changed from 3.2 to 2.8

Replying to nacin:

We need to figure out:

  • If any plugins are using the wp_get_object_terms filter, and what they do when they handle $taxonomies.
  • When we broke this, or if it was always like this.

It appears that the taxonomies have been passed like this since the filter was first introduced in [11253].

comment:6 in reply to: ↑ 4 ; follow-up: @postpostmodern19 months ago

Replying to nacin:

We need to figure out:

  • If any plugins are using the wp_get_object_terms filter, and what they do when they handle $taxonomies.
  • When we broke this, or if it was always like this.

As far as I can tell, this affects two plugins in the plugins directory.

Custom Taxonomy Sort This plugin has not been updated in over two years. The method get_terms in http://plugins.trac.wordpress.org/browser/custom-taxonomy-sort/trunk/custom-taxonomy-sort.php uses this as the $taxonomy argument and ignores it.

The other plugin that uses this is Co-Authors Plus. The method filter_wp_get_object_terms in http://plugins.trac.wordpress.org/browser/co-authors-plus/trunk/co-authors-plus.php uses this data in the way WordPress is currently built, directly inserted into a manual sql query. However, this plugin is authored by Core contributors and could easily be updated, and as it is only tested to 3.5.2 it is due to be checked for compatibility anyways.

I would argue that using $taxonomies as an array in this filter is still better and more flexible to work with, a use case would be removing or inserting specific taxonomies into this data.

Last edited 19 months ago by postpostmodern (previous) (diff)

comment:7 @doublesharp14 months ago

  • Keywords dev-feedback added

The same issue applies to $object_ids as well - its type is changed from an array to a string before it is passed to the wp_get_object_terms filter. All of the caveats of use by plugins, etc apply as well but I am attaching a patch that updates both the $taxonomy (array) to $_taxonomy (string) and $object_ids (array) to $_object_ids (string).

@doublesharp14 months ago

wp_get_object_terms filter fix for $taxonomies and $object_ids

@doublesharp14 months ago

taxonomy_wp_get_object_terms corrected

comment:8 @SergeyBiryukov5 months ago

#31098 was marked as a duplicate.

comment:9 @SergeyBiryukov5 months ago

  • Milestone changed from Future Release to 4.2

@MikeHansenMe5 months ago

Refreshed.

comment:10 @MikeHansenMe5 months ago

Looks like one of the plugins has not been updated in a couple years and the other is authored by people who are active in the community. Should be fairly easy to get them to update their plugin. I refreshed the existing patch since it failed when I applied it.

comment:11 @slackbot5 months ago

This ticket was mentioned in Slack in #core by drew. View the logs.

comment:12 in reply to: ↑ 6 @boonebgorges5 months ago

  • Keywords needs-docs added; has-patch removed

Replying to postpostmodern:

As far as I can tell, this affects two plugins in the plugins directory.

Here's a complete list of the affected plugins I just found in a search of the repo:

  • wpmovielibrary
  • co-authors-plus
  • polylang
  • capa
  • webcomic
  • qtranslate-slug
  • cpt-onomies

All of these plugins are actually using the $taxonomies variable passed to the filter, and either using it directly in a SQL query, or manipulating it as a string (eg explode( ',', $taxonomies )). They'll all break more or less every time the 'wp_get_object_terms' filter is called.

If this list is indicative of the popularity of this filter - and I'd wager that it is, and that the nature of the filter is such that it would be used even less on bespoke WP customizations than in distributed plugins - I'd be OK with breaking it. Does anyone have a problem with this?

To move forward, I'd like to see an updated patch that explains the change in the docblock (maybe another @since annotation).

comment:13 @slackbot5 months ago

This ticket was mentioned in Slack in #core by boone. View the logs.

comment:14 @johnjamesjacoby5 months ago

A few thoughts:

  • Co-authors-plus has almost 100k downloads and is quite popular amongst the WordPress.com VIP group.
  • CPT-onomies is around 20k downloads, and probably relies on this bit to make its connections.

Before we break anything, can we loop those plugin authors in and have an approximation of a fix ready for them?

comment:15 @boonebgorges5 months ago

Thanks johnjamesjacoby. Yes, the authors of those plugins should definitely be looped in once a decision is made one way or another. The fix in most cases is trivial: nearly all of the plugins are doing manipulation to turn the string into an array, and the fix will be to wrap that logic in if ( ! is_array( $taxonomies ) ).

Heck, I'm happy to write the necessary patches myself for this small handful of plugins. I'm not worried about the authors of co-authors-plus and cpt-onomies - they'll likely be proactive about changes like this. It's the long tail of plugins and private customizations that are more worrisome, but my guess is that the tail actually isn't that long in this case.

comment:16 @slackbot4 months ago

This ticket was mentioned in Slack in #core by drew. View the logs.

@wonderboymusic4 months ago

@boonebgorges4 months ago

comment:17 @boonebgorges4 months ago

Per a suggestion by nacin, 18828.3.diff skirts the issue by introducing a new filter, 'get_object_terms', which is passed a proper array of taxonomies.

comment:18 @slackbot4 months ago

This ticket was mentioned in Slack in #core by boone. View the logs.

comment:19 @boonebgorges4 months ago

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

In 31581:

Introduce 'get_object_terms' filter in wp_get_object_terms().

The existing 'wp_get_object_terms' filter accepts a parameter $taxonomies,
which is a list of taxonomy names formatted for direct use in a MySQL IN clause.
This formatting makes it difficult to make use of the taxonomy list in filter
callbacks. However, changing the parameters passed to the existing filter
raises backward compatibility concerns, so we introduce a new filter that
receives a structured $taxonomy_array parameter.

We also take this opportunity to correct and clean up some of the documentation
on the 'wp_get_object_terms' filter.

Props postpostmodern, doublesharp, wonderboymusic, nacin.
Fixes #18828.

comment:20 @doublesharp4 months ago

  • Keywords has-patch needs-testing added
  • Resolution fixed deleted
  • Status changed from closed to reopened

@boonebgorges This same behavior is applied to the $object_ids as well - it is passed as a SQL fragment rather than an array of IDs. The array is imploded just after the taxonomy array with:

$object_ids = implode(', ', $object_ids);

This differs from your documentation on the get_object_terms filter. Instead of a single ID or array of IDs, it will be a comma separated string as it is currently implemented.

@doublesharp4 months ago

Pass $object_id_array instead of $object_ids to filter

comment:21 @boonebgorges4 months ago

Good catch - thanks, doublesharp.

comment:22 @boonebgorges4 months ago

  • Resolution set to fixed
  • Status changed from reopened to closed

In 31639:

Ensure that an array of object IDs is passed to the 'get_object_terms' filter.

Originally introduced in [31581].

Props doublesharp.
Fixes #18828.

Note: See TracTickets for help on using tickets.