WordPress.org

Make WordPress Core

Opened 11 years ago

Closed 11 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 11 years ago.

Download all attachments as: .zip

Change History (23)

#1 @emartin24
11 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
11 years ago

  • Milestone changed from Awaiting Review to 3.1.1

#3 @ryan
11 years ago

[15705] is where the conditional was altered.

#4 @ryan
11 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
11 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
11 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
11 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
11 years ago

#8 @dd32
11 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
11 years ago

  • Component changed from Rewrite Rules to Canonical

#10 @dd32
11 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
11 years ago

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

#12 @scribu
11 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
11 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
11 years ago

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

#15 @automattor
11 years ago

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

#16 @mikeschinkel
11 years ago

  • Cc mikeschinkel@… added

#17 @ryan
11 years ago

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

#18 @dd32
11 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
11 years ago

Related: #17174

#20 @joehoyle
11 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
11 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
11 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.