Improvement to redirect_canonical with category permalink

[spacedmonkey]

If you are using category in permalink, the redirect_canonical calls wp_get_object_terms get categories on the current post. As wp_get_object_terms is not cached, it results in an sql query. This is a performance hit for sites using object caching. get_the_terms should be used instead.

[boonebgorges]

Thanks for the patch. This looks like a good fix. We have zero test coverage for this kind of rewrite, so I'm going add it before making the change.

[boonebgorges]

In 37260:

Add tests for permastruct containing /%category%/.

See #36602.

[boonebgorges]

In 37261:

Tests: After [37260], use WP's setUpBeforeClass() wrappers.

This ensures no leakage between tests of fixture IDs.

See #36602.

[boonebgorges]

In 37262:

Canonical: Use get_the_terms() to verify that a post belongs to the requested %category%.

The get_the_terms() wrapper provides cache support, and saves a database hit
on sites with a persistent cache backend.

Props spacedmonkey.
Fixes #36602.

[cmillerdev]

@boonebgorges @spacedmonkey recommend you back this change out. The in_array will always be false. This has a breaking impact on /embed/ endpoints when sites have %category% in their permalink structure.

Alternative patch attached if you want to use it to fix the original issue.

[cmillerdev]

Alternate patch

[boonebgorges]

@cmillerdev Can you explain why the in_array() check fails? Does this have something to do with object identity?

Your fix seems OK to me (has_term() falls back to is_object_in_term(), which checks the object cache) but we need a test to demonstrate the failure.

[cmillerdev]

@boonebgorges yes it's because it's checking for an object in an array of objects. You could change it to check for the term by ID etc, but has_term() seemed much simpler. Test should be easy enough, I'm not writing it though, I want nothing to do with that horrendous function! I'm just reporting the issue. Feel free to use that patch, or not, up to you.

Cheers

[boonebgorges]

In 38216:

Improve category check in redirect_canonical() when permastruct contains category slug.

[37262] changed a check in redirect_canonical() so that it checked
categories in the object cache rather than querying the database. However,
the check was based on the identity of WP_Term objects, which in
certain cases can be augmented by the main WP query routine, causing
failures of the in_array() check. This caused unnecessary redirects
for URLs where is_single() is true, but the URL is different from the
post permalink, such as the embed endpoint.

has_term() also checks the cache, but does not sufer from this bug.

Props cmillerdev.
Fixes #36602.

[boonebgorges]

Reopening for 4.6. Can I get a review of [37262] from @ocean90, @wonderboymusic, or @dd32 ?

[wonderboymusic]

this looks good to me

[boonebgorges]

In 38220:

Improve category check in redirect_canonical() when permastruct contains category slug.

[37262] changed a check in redirect_canonical() so that it checked
categories in the object cache rather than querying the database. However,
the check was based on the identity of WP_Term objects, which in
certain cases can be augmented by the main WP query routine, causing
failures of the in_array() check. This caused unnecessary redirects
for URLs where is_single() is true, but the URL is different from the
post permalink, such as the embed endpoint.

has_term() also checks the cache, but does not sufer from this bug.

Merges [38216] to the 4.6 branch.

Props cmillerdev.
Fixes #36602.