WordPress.org

Make WordPress Core

Opened 4 years ago

Closed 3 years ago

#36602 closed enhancement (fixed)

Improvement to redirect_canonical with category permalink

Reported by: spacedmonkey Owned by: boonebgorges
Milestone: 4.6 Priority: normal
Severity: normal Version: 3.0
Component: Canonical Keywords: has-patch commit dev-reviewed
Focuses: Cc:
PR Number:

Description

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.

Attachments (2)

36602.patch (1.0 KB) - added by spacedmonkey 4 years ago.
updated.36602.patch (1.2 KB) - added by cmillerdev 3 years ago.
Alternate patch

Download all attachments as: .zip

Change History (15)

@spacedmonkey
4 years ago

#1 @boonebgorges
4 years ago

  • Milestone changed from Awaiting Review to 4.6
  • Owner set to boonebgorges
  • Status changed from new to accepted

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.

#2 @boonebgorges
4 years ago

In 37260:

Add tests for permastruct containing /%category%/.

See #36602.

#3 @boonebgorges
4 years ago

In 37261:

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

This ensures no leakage between tests of fixture IDs.

See #36602.

#4 @boonebgorges
4 years ago

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

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.

#5 @cmillerdev
3 years ago

@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.

Last edited 3 years ago by cmillerdev (previous) (diff)

@cmillerdev
3 years ago

Alternate patch

#6 @boonebgorges
3 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

@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.

#7 @cmillerdev
3 years ago

@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

#8 @boonebgorges
3 years ago

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

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.

#9 @boonebgorges
3 years ago

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

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

#10 @wonderboymusic
3 years ago

this looks good to me

#11 @ocean90
3 years ago

  • Keywords has-patch commit dev-reviewed added; dev-feedback removed

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


3 years ago

#13 @boonebgorges
3 years ago

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

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.

Note: See TracTickets for help on using tickets.