Make WordPress Core

Opened 2 years ago

Closed 19 months ago

#55955 closed defect (bug) (fixed)

Setting a 'NOT EXISTS' tax query in 'pre_get_posts' action triggers a fatal error in wp-includes/canonical.php

Reported by: codesdnc's profile codesdnc Owned by: peterwilsoncc's profile peterwilsoncc
Milestone: 6.1.1 Priority: normal
Severity: normal Version: 6.0
Component: Canonical Keywords: php8 has-patch has-unit-tests commit fixed-major
Focuses: Cc:

Description

Under PHP8, the following code will fail on taxonomy archives: (is cut-down to its minimal form)

<?php
add_action( 'pre_get_posts', function( \WP_Query $query ){
  $query->set( 'tax_query', array(
    array(
      'taxonomy' => 'post_format',
      'operator' => 'NOT EXISTS',
    ),
  ) );
} );

Digging in the code, on line 334 of wp-includes/canonical.php, it assumes that for each element in WP_Tax_Query->queried_terms, there will be a 'terms' entry that is an (a) defined, and (b) an array. However, WP_Tax_Query::sanitize_query() (the only place I can see where queried_terms is set) does NOT guarantee this for cases like above -- it simply doesn't set a 'terms' entry for 'operator' => 'NOT EXISTS' queries (which makes sense). This is only a nuisance before PHP8, which makes passing non-arrays to count() throw an error.

Change History (16)

#1 @SergeyBiryukov
2 years ago

  • Keywords needs-patch needs-unit-tests php8 added

This ticket was mentioned in PR #3530 on WordPress/wordpress-develop by kadamwhite.


19 months ago
#2

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

As discussed in 55955, a NOT EXISTS tax_query causes a warning (PHP <= 7.4) or an error (PHP >= 8.0) in the redirect_canonical function because there is a count( $tax_query['terms'] ) check in that function that does not verify whether the terms property is present. It will not be present in NOT EXISTS queries because there is no purpose in specifying individual terms when we are querying for posts which have no terms at all.

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

#3 @kadamwhite
19 months ago

We ran into this one while upgrading a site to PHP 8 today, thank you @codesdnc for the clear ticket! It saved us a lot of time understanding why the feeds were failing.

I paired up with @miguelaxcar to debug this and create the linked PR.

I'm not certain whether there's a better way to set up the global $wp_query than in the unit test I added, but when we tried manipulating the global query directly the change didn't seem to trigger the error. We validated locally by setting LOCAL_PHP=8.0-fpm in our .env file when running unit tests.

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


19 months ago

#5 @SergeyBiryukov
19 months ago

  • Milestone changed from Awaiting Review to 6.1.1

#6 @kadamwhite
19 months ago

For anybody running into this between now and when the patch goes live, we've worked around it in our application by passing

    'terms' => array( 0 ),

within the tax_query array. Looking at how NOT EXISTS queries get parsed, it seems like this terms parameter is not used at all, so it seems to mitigate the PHP8 fatal without compromising application logic...

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

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


19 months ago

#8 @peterwilsoncc
19 months ago

  • Keywords commit added

I did a run of @kadamwhite's tests without the fix to ensure the tests are correct. They are.

I think this is ready for commit. I'll do so for trunk but leave it up to @desrosj and @jeffpaul to decide if they would like to put this in the minor release.

@peterwilsoncc commented on PR #3530:


19 months ago
#9

@MiguelAxcarHM are you able to link your WordPress.org account to your GitHub account so I can ensure you are given props correctly? This can be done in the GitHub username section of https://profiles.wordpress.org/me/profile/edit/group/1/

#10 @peterwilsoncc
19 months ago

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

In 54785:

Canonical: Protect against error for term not exists queries.

Prevent term NOT EXISTS queries causing redirect_canonical() to throw a fatal error in PHP 8 and above, or a warning in earlier versions.

This ensures the tax_query's terms property both exists and is countable before attempting to count it.

Props codesdnc, SergeyBiryukov, kadamwhite, costdev, miguelaxcar.
Fixes #55955.

@peterwilsoncc commented on PR #3530:


19 months ago
#11

Merged in https://core.trac.wordpress.org/changeset/54785 / b9304148f17278dbe9a4199a1143b083188f0d61

#12 @peterwilsoncc
19 months ago

  • Keywords fixed-major added
  • Resolution fixed deleted
  • Status changed from closed to reopened

Reopening for backport consideration.

If decided against, please move to the 6.2 milestone and reclose.

@miguelaxcar commented on PR #3530:


19 months ago
#13

Done @peterwilsoncc , thank you! :)

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


19 months ago

#15 @desrosj
19 months ago

I think this is worth including. Going to backport. Thanks everyone!

#16 @desrosj
19 months ago

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

In 54793:

Canonical: Protect against error for term not exists queries.

Prevent term NOT EXISTS queries causing redirect_canonical() to throw a fatal error in PHP 8 and above, or a warning in earlier versions.

This ensures the tax_query's terms property both exists and is countable before attempting to count it.

Props codesdnc, SergeyBiryukov, kadamwhite, costdev, miguelaxcar.
Merges [54785] to the 6.1 branch.
Fixes #55955.

Note: See TracTickets for help on using tickets.