WordPress.org

Make WordPress Core

Opened 7 years ago

Closed 7 years ago

#16627 closed defect (bug) (fixed)

Rewrite query vars are stripped from custom taxonomy page URLs via redirect_canonical()

Reported by: gnoodl Owned by:
Milestone: 3.1.1 Priority: normal
Severity: normal Version: 3.1
Component: Canonical Keywords:
Focuses: Cc:

Description

In wp-includes/canonical.php, version 3.1 changed lines 155-177 from

if ( $term_count <= 1 && !empty($obj->term_id) && ( $tax_url = get_term_link((int)$obj->term_id, $obj->taxonomy) )
		&& !is_wp_error($tax_url) && $redirect['query'] ) {

    /* Not important */
    
	$tax_url = parse_url($tax_url);
	if ( ! empty($tax_url['query']) ) { // Custom taxonomies may only be accessable via ?taxonomy=..&term=..
		parse_str($tax_url['query'], $query_vars);
		$redirect['query'] = add_query_arg($query_vars, $redirect['query']);
	} else { // Taxonomy is accessable via a "pretty-URL"
		$redirect['path'] = $tax_url['path'];
	}
}

to (lines 153-174)

if ( $term_count <= 1 && !empty($obj->term_id) && ( $tax_url = get_term_link((int)$obj->term_id, $obj->taxonomy) ) && !is_wp_error($tax_url) ) {
	if ( !empty($redirect['query']) ) {

        /* Not important */
	    
	}
	$tax_url = parse_url($tax_url);
	if ( ! empty($tax_url['query']) ) { // Custom taxonomies may only be accessable via ?taxonomy=..&term=..
		parse_str($tax_url['query'], $query_vars);
		$redirect['query'] = add_query_arg($query_vars, $redirect['query']);
	} else { // Taxonomy is accessable via a "pretty-URL"
		$redirect['path'] = $tax_url['path'];
	}
}

The difference here is that in the new version, the $redirect['path'] is always set to the taxonomy path, eliminating any query vars added via custom rewrite rule, regardless of whether there are query vars in $redirect['query'] or not.

Attachments (1)

16627.diff (1.1 KB) - added by dd32 7 years ago.

Download all attachments as: .zip

Change History (23)

#1 @emartin24
7 years ago

  • Cc eric@… added

Just discovered this issue on my site as well. In my research, I found a few differences that seem to be causing the "issue".

1) In the code just above what @gnoodl mentioned, in 3.0.5, the following code results in a $term_count of 0:

$term_count = 0;
foreach ( array('category__in', 'category__not_in', 'category__and', 'post__in', 'post__not_in',
'tag__in', 'tag__not_in', 'tag__and', 'tag_slug__in', 'tag_slug__and') as $key )
	$term_count += count($wp_query->query_vars[$key]);

In 3.1, the new code results in a $term_count of 1:

$term_count = 0;
foreach ( $wp_query->tax_query->queries as $tax_query )
	$term_count += count( $tax_query['terms'] );

2) In the if condition that @gnoodl mentioned, 3.0.5 includes a check for $redirectquery?, 3.1 does not.


In my case, I'm using a custom query variable. Perhaps there is another solution to my problem, but it worked in 3.0.5 and this is the code that causes it to no longer work ;)

#2 @ryan
7 years ago

  • Milestone changed from Awaiting Review to 3.1.1

#3 @ryan
7 years ago

[15705] is where the conditional was altered.

#4 @ryan
7 years ago

A test case would be helpful here. What is an example URL that is failing? What does the registration code for the taxonomy look like?

#5 @emartin24
7 years ago

Here's what I am using. Worked in 3.0.5, not in 3.1:

// create the custom taxonomies
function btf_create_taxonomies() {
	register_taxonomy('textures', 'post', array('hierarchical' => false, 'label' => 'Texture Categories', 'query_var' => true, 'rewrite' => true));
	register_taxonomy('wallpapers', 'post', array('hierarchical' => false, 'label' => 'Wallpaper Categories', 'query_var' => true, 'rewrite' => true));
}
add_action('init', 'btf_create_taxonomies', 0);

// add the custom query var
function btf_query_vars($query_vars) {
	$query_vars[] = 'sort';
	return $query_vars;
}
add_filter('query_vars', 'btf_query_vars');

// I had some issues with custom query vars and paging, 
// so I wrote this to correct the query string values
function btf_request($query_string) {
	if (isset($query_string['name']) && $query_string['name'] === 'page'
			&& isset($query_string['page'])) {
		unset($query_string['name']);
		$query_string['paged'] = str_replace('/', '', $query_string['page']);
	}

	$sort = array('date', 'popularity', 'comments', 'downloads', 'rating', 'votes');
	if (isset($query_string['name']) && isset($query_string['category_name'])
			&& btf_ends_with('/sort', $query_string['category_name'])) {
		if (strpos($query_string['name'], '-') > 0) {
			$valid = explode('-', $query_string['name']);
			$s = $valid[0];
		}
		else {
			$s = $query_string['name'];
		}

		if (in_array($s, $sort)) {
			$parts = explode('/', $query_string['category_name']);
			$query_string['sort'] = $query_string['name'];
			$query_string['category_name'] = $parts[0];
			unset($query_string['name']);
		}
	}
	return $query_string;
}
add_filter('request', 'btf_request');

I hope the btf_request function doesn't over-complicate things. Basically, what I want is to be able to have:

  • mysite.com/custom-taxonomy/ - default listing
  • mysite.com/custom-taxonomy/sort/downloads - sort by downloads
  • mysite.com/custom-taxonomy/sort/downloads/page/2 - sort by downloads, page 2

The code I have above worked fine in 3.0.5. If there is a better way to achieve this, I'm happy to try a different approach.

#6 @gnoodl
7 years ago

Mine looks very similar to emartin's except I'm using rewrite rules.

Taxonomy code

register_taxonomy('my_taxonomy', 'custom_post_type', array(
    'label'     => __('My Taxonomy Name'),
    'public'    => true,
    'rewrite'   => array('slug' => 'my_slug_prefix'),
    'query_var' => 'my_taxonomy_var'
));

Added query vars (using the query_vars filter)

$queryVars[] = 'criteria';
return $queryVars;

Rewrite Rules (using the generate_rewrite_rules action hook)

$rules = array(
    'my_slug_prefix/(.+)/(.+)/(asc|desc)' => sprintf(
        'index.php?taxonomy=my_taxonomy&my_taxonomy_var=%s&criteria=%s&order=%s',
        $rewrite->preg_index(1), $rewrite->preg_index(2), $rewrite->preg_index(3)),
    'my_slug_prefix/(.+)/(.+)' => sprintf(
        'index.php?taxonomy=my_taxonomy&my_taxonomy_var=%s&criteria=%s',
        $rewrite->preg_index(1), $rewrite->preg_index(2))
);
$rewrite->rules = $rules + $rewrite->rules;

Like emartin's, mine worked fine in 3.0.5. Any matching URL is now redirected to the default taxonomy page via canonical.php line 172

$redirect['path'] = $tax_url['path'];

#7 @dd32
7 years ago

[15705] is where the conditional was altered.

That reverted [15462], which was hiding the issue by only processing requests with $_GET variables present.

This particular change is rooted in the fact that non-category, non-tag taxonomies got a canonicalisation treatment.

My initial thought pattern here, is that the function should check to see what $query_vars are present, ie. if anything other than taxonomy & $taxonomy_query_var are present in the rewrite, skip canonicalising based on the term path.

@dd32
7 years ago

#8 @dd32
7 years ago

My initial thought pattern here, is that the function should check to see what $query_vars are present, ie. if anything other than taxonomy & $taxonomy_query_var are present in the rewrite, skip canonicalising based on the term path.

attachment 16627.diff added

  • The above idea in code.

#9 @dd32
7 years ago

  • Component changed from Rewrite Rules to Canonical

#10 @dd32
7 years ago

The other option is to cripple the canonicalisation for taxonomies for 3.1.x by adding back in the check for $_GET variables present, that'll at least cause canonical to not apply to pure rewrite rules, but as soon as a $_GET variable is added to the mix, it'll kick in again.. as it should've done so in 3.0. and leave above patch for a 3.2 fix.

#11 @ryan
7 years ago

What's the other option look like, in patch form? :-)

#12 @scribu
7 years ago

How about doing a redirect only if there are $_GET variables present that are not registered with $wp->add_query_var() ?

#13 @dd32
7 years ago

The alternate to "cripple" it, is to re-instate [15462] - specifically, checking to see if a query is set. Which is a better option for 3.1.1, leaving a proper fix to 3.2

#14 @ryan
7 years ago

Putting $redirectquery? back for 3.1.1 sounds good to me.

#15 @automattor
7 years ago

(In [17549]) Reinstate [15462] for 3.1; Prevents canonical redirects for custom rewrite rules for taxonomies. See #16627

#16 @mikeschinkel
7 years ago

  • Cc mikeschinkel@… added

#17 @ryan
7 years ago

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

#18 @dd32
7 years ago

  • Milestone changed from 3.1.1 to 3.2
  • Resolution fixed deleted
  • Status changed from closed to reopened

This was never dealt with in trunk

#19 @scribu
7 years ago

Related: #17174

#20 @joehoyle
7 years ago

With the re-instated patch, term URLs are not canonicalized (though they probably weren't before). If you are using pretty permalinks, then the $redirect['query'] will never be set (looking at http://core.trac.wordpress.org/browser/tags/3.1.2/wp-includes/canonical.php#L153).

This means if the $tax_url is different to the current URL, it will not be redirected. E.g. /tag/Apple/ should redirect to /tag/apple/, but won't be. Removing the $redirect['query'] check fixes the canonical redirect in my case.

#21 @dd32
7 years ago

With the re-instated patch, term URLs are not canonicalized

Yep, For 3.1.x the decision was to change as little code as possible to restore the previous functionality. 3.2 should allow both, Patches on #17174 will probably be used to close this ticket.

#22 @dd32
7 years ago

  • Milestone changed from 3.2 to 3.1.1
  • Resolution set to fixed
  • Status changed from reopened to closed

Reclosing as fixed for 3.1.1.

See #17174 for 3.2

Note: See TracTickets for help on using tickets.