WordPress.org

Make WordPress Core

Opened 2 months ago

Closed 2 months ago

Last modified 2 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 2 months ago.
40496.2.diff (3.3 KB) - added by danielbachhuber 2 months ago.

Download all attachments as: .zip

Change History (9)

#1 @johnbillion
2 months ago

Broken in [38667].

cc @boonebgorges

#2 @boonebgorges
2 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
2 months ago

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


2 months ago

#4 @danielbachhuber
2 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
2 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
2 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
2 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.

Note: See TracTickets for help on using tickets.