Make WordPress Core

Opened 19 months ago

Last modified 3 months ago

#57300 new defect (bug)

parse_tax_query() does not handle array query param when taxonomy has rewrite set to hierarchical

Reported by: chamois_blanc's profile chamois_blanc Owned by:
Milestone: Awaiting Review Priority: normal
Severity: major Version: 6.1.1
Component: Taxonomy Keywords: has-patch
Focuses: Cc:

Description

Accessing the following URL on one of my sites create a PHP 8 type error:

/calendar/list/?tribe_events_cat[0]=advertised

tribe_events_cat is a taxonomy associated with the tribe_events custom post type. It is defined with rewrite as hierarchical.

The code in parse_tax_query() does not seem to handle this case properly. If rewrite is not set to hierarchical, the code checks whether the query term is an array and implodes it:

<?php
        $term = $q[ $t->query_var ];

        if ( is_array( $term ) ) {
          $term = implode( ',', $term );
        }

but if rewrite is set to hierarchical, the term is passed to wp_basename() directly. I don't know much about this code, but it seems like a bug.

Here is the code trace:

[ERROR] lvl:1 | msg:Uncaught TypeError: urlencode(): Argument #1 ($string) must be of type string, array given in .../public_html/wp-includes/formatting.php:5525
Stack trace:
#0 .../public_html/wp-includes/formatting.php(5525): urlencode()
#1 .../public_html/wp-includes/class-wp-query.php(1166): wp_basename()
#2 .../public_html/wp-includes/class-wp-query.php(936): WP_Query->parse_tax_query()
#3 .../public_html/wp-includes/class-wp-query.php(1820): WP_Query->parse_query()
#4 .../public_html/wp-includes/class-wp-query.php(3749): WP_Query->get_posts()
#5 .../public_html/wp-includes/class-wp.php(663): WP_Query->query()
#6 .../public_html/wp-includes/class-wp.php(783): WP->query_posts()
#7 .../public_html/wp-includes/functions.php(1332): WP->main()
#8 .../public_html/wp-blog-header.php(16): wp()
#9 .../public_html/index.php(17): require('...')
#10 {main}
  thrown | file:.../public_html/wp-includes/formatting.php | ln:5525

It does not appear to go through any theme/plugins code, so my guess is that it's a WP core bug. Let me know if I'm getting this wrong.

Change History (20)

#1 @chamois_blanc
19 months ago

I think the code should look like this instead:

<?php
        $term = $q[ $t->query_var ];

        if ( is_array( $term ) ) {
          $term = implode( ',', $term );
        }

        if ( ! empty( $t->rewrite['hierarchical'] ) ) {
          $term = wp_basename( $term );
        }

#2 @chamois_blanc
19 months ago

Or maybe like this:

<?php
        $term = $q[ $t->query_var ];

        if ( ! empty( $t->rewrite['hierarchical'] ) ) {
          if ( is_array( $term ) ) {
            foreach ( $term as $k => $v ) {
              $term[$k] = wp_basename( $v );
            }
          } else {
            $term = wp_basename( $term );
          }
        }

        if ( is_array( $term ) ) {
          $term = implode( ',', $term );
        }

#3 @chamois_blanc
16 months ago

Unfortunately this is not fixed in 6.2.

#4 @chamois_blanc
16 months ago

Here is a diff of a possible fix:

<?php
        // Comment out this code
        // if ( ! empty( $t->rewrite['hierarchical'] ) ) {
        //  $q[ $t->query_var ] = wp_basename( $q[ $t->query_var ] );
        // }

        $term = $q[ $t->query_var ];

        // BEGIN - Add this code
        if ( ! empty( $t->rewrite['hierarchical'] ) ) {
          if ( is_array( $term ) ) {
            foreach ( $term as $k => $v ) {
              $term[$k] = wp_basename( $v );
            }
          } else {
            $term = wp_basename( $term );
          }
        }
        // END

#5 @pskli
15 months ago

I'm having this issue as well.

When trying to filter archive pages on multiple taxonomy terms using array query args in the URL (such as ?product_cat[]=t-shirts&product_cat[]=accessories), a fatal error is triggered (only if the requested filter taxonomy rewrite is hierarchical).

For instance, this is an issue on the default WooCommerce shop page if trying to implement a custom filtering logic.

Last edited 15 months ago by pskli (previous) (diff)

This ticket was mentioned in PR #4396 on WordPress/wordpress-develop by pierre-dekode.


15 months ago
#6

  • Keywords has-patch added

When accessing a URL with a taxonomy slug URL parameter using array format (such as ?product_cat[]=accessories), a fatal error is triggered if the related taxonomy is hierarchical.
For example, visiting the default WooCommerce /shop page with multiple product_cat[]=… query args will trigger this error.

Trac ticket: [](https://core.trac.wordpress.org/ticket/57300)

---

This Pull Request is for code review only. Please keep all other discussion in the Trac ticket. Do not merge this Pull Request. See GitHub Pull Requests for Code Review in the Core Handbook for more details.

#7 @pskli
15 months ago

Steps to recreate on a fresh install

  • Register a new post type and taxonomy. Post type needs an archive (for testing purposes) and taxonomy needs rewrite.hierarchical set to true. Gist for that step available here: https://gist.github.com/pierre-dekode/c934284b88ebf0a31f4e433f265c4a64
  • Flush permalinks
  • Visit the post type archive with ?taxonomy[]=something query arg in URL. Example with Gist provided above: http://<yoursite.local>/my_custom_post_type/?my_taxonomy[]=something

This will trigger the fatal error.


Alternatively, do a fresh WordPress + WooCommerce installation and create the default WC pages. Then visit the shop page with a product_cat[] query arg, such as /shop?product_cat[]=something. This will also trigger the fatal error.

Last edited 15 months ago by pskli (previous) (diff)

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


15 months ago

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


15 months ago

#10 @oglekler
14 months ago

Hi! Even with instructions I was unable to reproduce this issue in several envs with WP 6.2 and trunk, PHP 7.4 and PHP 8.2, custom post type and hierarchical taxonomy and product_cat of WooCommerce 7.0.0.

But it looks like there is something, #58109 - related, ticket I was unable to reproduce either.

I wonder if something else is interfering with these cases. It will be good to know where this is coming from.

#11 @pskli
14 months ago

Thank you @oglekler!

As you can see in my previous comment (and provided Gist), the critical thing is to have the $args['rewrite']['hierarchical'] taxonomy registration argument set to true.

Not the $args['hierarchical'] argument :)

Can you let me know if that was the case? And maybe share the test code you've used?

Thank you!

#12 @oglekler
14 months ago

@pskli I still cannot reproduce it. I tried to change permalinks in case of they can interfere somehow, but didn't manage to get an error either. I wonder if we can get something from your deserialized rewrite_rules option value 🤔

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


14 months ago

#14 @pskli
14 months ago

Thanks for your help @oglekler

My rewrite rules (fresh 6.2 install, no other plugin except a custom one to load my custom CPT and taxonomy, see Gist above) are: https://gist.github.com/pierre-dekode/4ed610312680f7d6e67f56cf3b366081

On PHP 7.4, I get a PHP Warning displayed on the 404 page:
https://p156.p1.n0.cdn.getcloudapp.com/items/DOulOdyk/a284f76f-04e9-4402-979b-ea2a9d3ed69a.jpg

On PHP 8.1, I get a PHP Fatal error:
https://p156.p1.n0.cdn.getcloudapp.com/items/o0uYnGlW/982102eb-42ec-44d1-bf28-fb0cca2c1803.jpg


If I install and activate Query Monitor, I can see that the matching rewrite rule for this URL is 'my_custom_post_type/?$' => 'index.php?post_type=my_custom_post_type'
The tested URL being http://core.wp/my_custom_post_type/?my_taxonomy[]=test

Let me know how I can help!

Last edited 14 months ago by pskli (previous) (diff)

#15 @pskli
14 months ago

Actually, it seems that it is not necessary to access the custom post type rewrite rule to trigger the fatal error.

By visiting http://core.wp/?my_taxonomy[]=test — I get it too.

The my_taxonomy[] array query param is the culprit here, subsequently triggering the WP_Query->parse_tax_query method.

@oglekler can you share the custom taxonomy code you've created maybe?

#16 @pauldewouters
12 months ago

I was able to reproduce this error, and I wrote a phpunit test case for the patch
https://github.com/pierre-dekode/wordpress-develop/pull/1/files

#17 @chamois_blanc
9 months ago

Thank you @pauldewouters for providing a repro and unit test. Any chance to get this bug fixed soon-ish? Otherwise I have to patch WP manually every time there is a version update :/

#18 @petitphp
6 months ago

Test Report

This report validates that the indicated patch addresses the issue.

Patch tested: https://github.com/WordPress/wordpress-develop/pull/4396.diff

Steps used to reproduce the issue :

Environment

  • OS: macOS 13.6.1
  • Web Server: Nginx
  • PHP: 8.0.30
  • WordPress: 6.5-alpha-56966-src
  • Browser: Firefox 122b9
  • Theme: Twenty Twenty-Four
  • Active Plugins:
    • Query Monitor 3.15.0

Actual Results

  • ✅ Issue resolved with the patch

Supplemental Artifacts

Before patch 4396: https://i.postimg.cc/nrpkVJrC/before-patch-4396.png
After patch 4396: https://i.postimg.cc/2SVxSTkF/after-patch-4396.png

@lxt commented on PR #4396:


6 months ago
#19

Hey @pierre-dekode, love your solution for this. Let's hope this PR will be merged soon.

@lxt commented on PR #4396:


3 months ago
#20

Why is there still no progress? I need to manually fix this after every WP update which is kind of annoying :-(

Note: See TracTickets for help on using tickets.