Make WordPress Core

Opened 5 years ago

Last modified 4 years ago

#47719 reviewing defect (bug)

Consistency issue with `include` parameter set to "0" in `WP_Term_Query`

Reported by: audrasjb's profile audrasjb Owned by: sergeybiryukov's profile SergeyBiryukov
Milestone: Future Release Priority: normal
Severity: normal Version:
Component: Query Keywords: has-patch has-unit-tests dev-feedback
Focuses: Cc:

Description (last modified by audrasjb)

There is a consistency issue with include parameter when used in WP_Term_query.

If include is set to/contains [0], the query returns all terms.

This is not consistent with other query classes, like WP_query (using corresponding post_in parameter) and even more like WP_User_query (using the same include parameter).

Those doesn't return any result.

See reproductible examples below:

WP_Query

$args = array(
        'post_type' => 'post',
        'post__in' => [0],
);
$wp_query = new WP_Query( $args );
if ( $wp_query->have_posts() ) :
        while ( $wp_query->have_posts() ) :
                $wp_query->the_post();
                echo $post->ID;
        endwhile;
endif;

=> returns no post

WP_user_query

$args = array(
        'role' => '',
        'include' => [0],
);
$wp_user_query = new WP_User_Query( $args );
$users = $wp_user_query->get_results();
if ( ! empty( $users ) ) :
        foreach ( $users as $user ) :
                echo $user->ID;
        endforeach;
endif;

=> returns no user

WP_Term_Query

$args = array(
        'taxonomy' => 'category',
        'include' => [0],
);
$wp_term_query = new WP_Term_query( $args );
if ( ! empty( $wp_term_query->terms ) ) :
        foreach ( $wp_term_query->terms as $term ) :
                echo $term->term_id;
        endforeach;
endif;

=> returns all terms

Credits @loicblascos for the initial bug report.

Attachments (3)

47719.diff (510 bytes) - added by audrasjb 5 years ago.
Fix 47719 consistency issue with include parameter
47719.1.diff (646 bytes) - added by audrasjb 5 years ago.
Removing useless $inclusions var
47719.2.diff (642 bytes) - added by Hareesh Pillai 4 years ago.
Patch refreshed against trunk

Download all attachments as: .zip

Change History (14)

@audrasjb
5 years ago

Fix 47719 consistency issue with include parameter

#1 @audrasjb
5 years ago

  • Keywords has-patch needs-unit-tests added; needs-patch removed

47719.diff is fixing the issue on my side.

Credits: @loicblascos for the initial bug report on WP French Slack Team.

#2 @audrasjb
5 years ago

  • Description modified (diff)

@audrasjb
5 years ago

Removing useless $inclusions var

#3 @mukesh27
5 years ago

@audrasjb Patch 47719.1.diff working fine for me in the latest version

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


4 years ago

#5 @SergeyBiryukov
4 years ago

  • Milestone changed from Awaiting Review to 5.6
  • Owner set to SergeyBiryukov
  • Status changed from new to reviewing

@Hareesh Pillai
4 years ago

Patch refreshed against trunk

This ticket was mentioned in PR #679 on WordPress/wordpress-develop by hellofromtonya.


4 years ago
#6

  • Keywords has-unit-tests added; needs-unit-tests removed

Trac ticket: https://core.trac.wordpress.org/ticket/47719

  • Adds unit test
  • Validated issue, ie returns all terms when 'include' => array( 0 ),
  • Applies patch `47719.2.diff`
  • Tests pass, ie returns null

#7 @hellofromTonya
4 years ago

  • Keywords commit added; dev-feedback removed

PR 679 adds unit test for the patch.

Step 1: Validate issue

Before applying patch 47719.2.diff, added the unit test. Confirmed the query returns all of the terms when 'include' => [0]. Failing test results:

<?php
There was 1 failure:

1) Tests_Term_Query::test_should_return_null_when_include_0
Failed asserting that Array &0 (
    0 => WP_Term Object &000000003eeb693c000000003803654e (
        'term_id' => 2
        'name' => 'Term 0000018'
        'slug' => 'term-0000018'
        'term_group' => 0
        'term_taxonomy_id' => 2
        'taxonomy' => 'wptests_tax'
        'description' => 'Term description 0000018'
        'parent' => 0
        'count' => 0
        'filter' => 'raw'
    )
    1 => WP_Term Object &000000003eeb693d000000003803654e (
        'term_id' => 3
        'name' => 'Term 0000019'
        'slug' => 'term-0000019'
        'term_group' => 0
        'term_taxonomy_id' => 3
        'taxonomy' => 'wptests_tax'
        'description' => 'Term description 0000019'
        'parent' => 0
        'count' => 0
        'filter' => 'raw'
    )
    2 => WP_Term Object &000000003eeb693e000000003803654e (
        'term_id' => 4
        'name' => 'Term 0000020'
        'slug' => 'term-0000020'
        'term_group' => 0
        'term_taxonomy_id' => 4
        'taxonomy' => 'wptests_tax'
        'description' => 'Term description 0000020'
        'parent' => 0
        'count' => 0
        'filter' => 'raw'
    )
) is null.

/var/www/tests/phpunit/tests/term/query.php:867

Step 2: Apply patch. Revalidate.

Confirmed. Test passes, i.e. no terms are returned.

Last edited 4 years ago by hellofromTonya (previous) (diff)

#8 @hellofromTonya
4 years ago

  • Keywords dev-feedback added; commit removed

When applying the patch, the $terms property's value and data type changes: was array, now null. This will be a breaking change, especially when using the terms property in an iterator, such as foreach.

When no terms are found, this commit changes the terms property to an empty array before bailing out.

<?php
                if ( empty( $terms ) ) {
                        wp_cache_add( $cache_key, array(), 'terms', DAY_IN_SECONDS );
                        $this->terms = array();
                        return $this->terms;
                }

@audrasjb @SergeyBiryukov What do you foresee any side effects or impacts from this change?

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


4 years ago

#10 @hellofromTonya
4 years ago

  • Milestone changed from 5.6 to 5.7

With 5.6 RC1 tomorrow, it's too late in the cycle for a WP_Query change. Punting this ticket.

If any maintainer or committer feels this can be resolved in time, or wishes to assume ownership during a specific cycle, feel free to update the milestone accordingly.

#11 @lukecarbis
4 years ago

  • Milestone changed from 5.7 to Future Release

As per @hellofromTonya's comment above, we're too late into the 5.7 cycle to include this change. I'll update to Future Release for now.

Note: See TracTickets for help on using tickets.