WordPress.org

Make WordPress Core

Opened 8 months ago

Closed 8 months ago

Last modified 5 months ago

#40496 closed defect (bug) (fixed)

get_the_terms() doesn't respect register_taxonomy()'s 'orderby' => 'term_order'

Reported by: danielbachhuber Owned by: boonebgorges
Milestone: 4.7.5 Priority: normal
Severity: normal Version: 4.7
Component: Taxonomy Keywords: has-patch needs-testing
Focuses: Cc:

Description

The following test passes in 4.6 and fails in 4.7:

public function test_get_the_terms_should_respect_taxonomy_orderby() {
	register_taxonomy( 'wptests_tax', 'post', array(
		'sort' => true,
		'args' => array(
			'orderby' => 'term_order',
		),
	) );
	$term_ids = self::factory()->term->create_many( 2, array(
		'taxonomy' => 'wptests_tax',
	) );
	$post_id = self::factory()->post->create();
	wp_set_object_terms( $post_id, array( $term_ids[0], $term_ids[1] ), 'wptests_tax' );
	$terms = get_the_terms( $post_id, 'wptests_tax' );
	$this->assertEquals( array( $term_ids[0], $term_ids[1] ), wp_list_pluck( $terms, 'term_id' ) );
	// Flip the order
	wp_set_object_terms( $post_id, array( $term_ids[1], $term_ids[0] ), 'wptests_tax' );
	$terms = get_the_terms( $post_id, 'wptests_tax' );
	$this->assertEquals( array( $term_ids[1], $term_ids[0] ), wp_list_pluck( $terms, 'term_id' ) );
}

I haven't tracked down the source of the break though.

Attachments (2)

40496.diff (2.2 KB) - added by boonebgorges 8 months ago.
40496.2.diff (3.3 KB) - added by danielbachhuber 8 months ago.

Download all attachments as: .zip

Change History (11)

#1 @johnbillion
8 months ago

Broken in [38667].

cc @boonebgorges

#2 @boonebgorges
8 months ago

  • Keywords has-patch needs-testing added; needs-patch removed

Yes, totally my fault. This argument merging was omitted when wp_get_object_terms() was refactored to use WP_Term_Query.

In my (lame) defense, I had no idea that this weird feature existed - it was introduced in [7520], doesn't appear to be documented anywhere, and is a pattern I've never seen used elsewhere in WordPress. It's not the worst idea to have taxonomy-specific query arguments (or post types, for that matter!), but perhaps we should advertise it better.

40496.diff appears to resolve the problem by restoring the argument-merging from pre-4.7. @danielbachhuber Care to have a look?

@boonebgorges
8 months ago

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


8 months ago

#4 @danielbachhuber
8 months ago

In my (lame) defense, I had no idea that this weird feature existed - it was introduced in [7520], doesn't appear to be documented anywhere, and is a pattern I've never seen used elsewhere in WordPress.

Undocumented features are the best features.

40496.diff​ appears to resolve the problem by restoring the argument-merging from pre-4.7. @danielbachhuber Care to have a look?

Good thing I tested! It fixes the problem for querying one taxonomy, but breaks for querying two taxonomies with wp_get_object_terms().

Here's the test that fails:

public function test_wp_get_object_terms_should_respect_taxonomy_orderby() {
	register_taxonomy( 'wptests_tax', 'post', array(
		'sort' => true,
		'args' => array(
			'orderby' => 'term_order',
		),
	) );
	$term_ids = self::factory()->term->create_many( 2, array(
		'taxonomy' => 'wptests_tax',
	) );
	$post_id = self::factory()->post->create();
	wp_set_object_terms( $post_id, array( $term_ids[0], $term_ids[1] ), 'wptests_tax' );
	$terms = wp_get_object_terms( $post_id, array( 'category', 'wptests_tax' ) );
	$this->assertEquals( array( $term_ids[0], $term_ids[1] ), wp_list_pluck( $terms, 'term_id' ) );
	// Flip the order
	wp_set_object_terms( $post_id, array( $term_ids[1], $term_ids[0] ), 'wptests_tax' );
	$terms = wp_get_object_terms( $post_id, array( 'category', 'wptests_tax' ) );
	$this->assertEquals( array( $term_ids[1], $term_ids[0] ), wp_list_pluck( $terms, 'term_id' ) );
}

The original code set $terms = array() early, then called wp_get_object_terms() for each taxonomy and merged the terms in.

The code in 4.7 sets $terms = get_terms( $args ); after the code you've ported over, so the handling for two taxonomies is discarded.

#5 @danielbachhuber
8 months ago

40496.2.diff includes the unit test for querying two taxonomies, and fixes the unit test by:

  1. Parsing $args and setting a default $terms = array(); before taxonomy args are processed.
  2. Merging $terms = array_merge( $terms, get_terms( $args ) ); after taxonomy args are processed, to ensure already queried terms aren't discarded.

#6 @boonebgorges
8 months ago

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

In 40513:

Restore support for taxonomy 'args' override when querying object terms.

[7520] introduced an undocumented feature whereby developers could
register a custom taxonomy with an 'args' parameter, consisting of
an array of config params that, when present, override corresponding
params in the $args array passed to wp_get_object_terms() when
using that function to query for terms in the specified taxonomy.

The wp_get_object_terms() refactor in [38667] failed to respect
this secret covenant, and the current changeset atones for the
transgression.

Props danielbachhuber.
Fixes #40496.

#7 @boonebgorges
8 months ago

In 40514:

Restore support for taxonomy 'args' override when querying object terms.

[7520] introduced an undocumented feature whereby developers could
register a custom taxonomy with an 'args' parameter, consisting of
an array of config params that, when present, override corresponding
params in the $args array passed to wp_get_object_terms() when
using that function to query for terms in the specified taxonomy.

The wp_get_object_terms() refactor in [38667] failed to respect
this secret covenant, and the current changeset atones for the
transgression.

Ports [40513] to the 4.7 branch.

Props danielbachhuber.
Fixes #40496.

#8 @GhostToast
5 months ago

@boonebgorges @danielbachhuber - I stumbled upon this trac ticket, when working with WP 4.8 I noticed I was getting duplicate terms (double the amount) when using get_the_terms() on a custom post type with custom taxonomies. I followed it along to the line where it does the array_merge() with a (seemingly) redundant call to get_terms():

$terms = array_merge( $terms, get_terms( $args ) );

This perplexed me for a while until I experimented enough to discover that if my custom post type had at least 1 taxonomy registered to it which _did not_ provide this in its setup:

'args' => [
	'orderby' => 'term_order',
],

then the problem was resolved!

I'm not sure if this is a new bug or a re-introduced one, but wanted to point it out.

To reproduce, try to view a custom post type's terms, fetched via get_the_terms(), where the custom post type has only taxonomies registered to it which include this manually-declared orderby. You should end up seeing twice as many terms (even if there is only 1 term to view).

#9 @boonebgorges
5 months ago

@GhostToast - Thanks for reporting this. I'm unable to reproduce, though it's likely that I'm not understanding the setup correctly. Here's the automated test I was using to try reproducing:

		register_post_type( 'wptests_pt' );
		register_taxonomy( 'wptests_tax2', 'wptests_pt', array(
			'args' => array(
				'orderby' => 'term_order',
			),
		) );

		$p = self::factory()->post->create( array(
			'post_type' => 'wptests_pt',
		) );

		$t = self::factory()->term->create( array(
			'taxonomy' => 'wptests_tax2',
		) );

		wp_set_object_terms( $p, $t, 'wptests_tax2' );

		$found = get_the_terms( $p, 'wptests_tax2' );
		print_r( $found );

If you're still having the issue, could you please open a new ticket that references this one, and contains more detailed instructions (ideally, with some code examples) to reproduce the issue? The current ticket was closed against 4.7.5, so it'd be nice to start fresh. Thanks!

Note: See TracTickets for help on using tickets.