Make WordPress Core

Opened 7 years ago

Closed 6 months ago

Last modified 3 months ago

#36723 closed enhancement (fixed)

Add caching to wp_old_slug_redirect

Reported by: spacedmonkey's profile spacedmonkey Owned by: spacedmonkey's profile spacedmonkey
Milestone: 6.1 Priority: normal
Severity: normal Version: 2.1
Component: Posts, Post Types Keywords: has-patch dev-feedback has-unit-tests needs-refresh needs-dev-note
Focuses: performance Cc:

Description

The wp_old_slug_redirect function is called on every page. It perform a raw sql query everytime it is run. The result of that query should be cached for performance reasons.

Attachments (1)

36723.patch (899 bytes) - added by spacedmonkey 7 years ago.

Download all attachments as: .zip

Change History (21)

@spacedmonkey
7 years ago

#1 @spacedmonkey
7 years ago

  • Keywords has-patch dev-feedback needs-unit-tests added

#2 @dd32
7 years ago

The wp_old_slug_redirect() function only runs when the current pageload is marked as a 404, based on the is_404() check at the start of the function.

I don't think caching is the answer here, rather the custom code you're using to make a "fake" page needs to flag the page as not-a-404 to WordPress.

#3 follow-up: @spacedmonkey
7 years ago

Sorry @dd32 I am not sure what you mean by "fake" page. The patch just caches the response from sql. That is then invalided when the clean_post_cache function is called and the last_changed value changes. I know WordPress VIP are doing something similar with caching the result of this function. Still not sure why this needs to call sql on every time. You could easily have a popular url that is 404ing as you remove the post. This just increase traffic to sql unnecessarily.

#4 in reply to: ↑ 3 @dd32
7 years ago

Replying to spacedmonkey:

Sorry @dd32 I am not sure what you mean by "fake" page.

Sorry, it was an assumption on my part - When someone says that a function that has a is_404() check in it is running every page, it generally means they're running a plugin which injects a "fake" page (one that doesn't actually result in a proper WP_Query object being located).

What I mean, is that the only time that query should ever be run, is in the case where a singular page (a non-hierarchical one at that) is requested and cannot be found.
Caching the function in that case doesn't seem to be the best location for it - it seems like the redirect should be cached instead to me.

#5 @spacedmonkey
7 years ago

I miss spoke when I said it was on every page load. I meant it hooked on template redirect on every page load.

So a little back story on this. I run a large high traffic multisite. Lots of editorial teams. I also use object caching to help with performance. While looking into sql log I found a high number of calls to this sql query. We don't cache 404s in our cdn, so this function is results in a high amount of sql traffic.

I am not sure it is possible to cache the redirect it's self. After getting the post ID it calls get permalink. The value of the permalink can be dynamic, as either it is filtered or the home_url changes, (ssl or domain mapping). I think that just save the result of sql is the best option, as it lets those filters run and doesn't break backwards compatibility.

This ticket was mentioned in PR #2378 on WordPress/wordpress-develop by pbearne.


9 months ago
#6

  • Keywords has-unit-tests added; needs-unit-tests removed

Not working yet
the new tests are not work

Trac ticket: https://core.trac.wordpress.org/ticket/36723

spacedmonkey commented on PR #2378:


9 months ago
#7

There are php lint errors in this PR. Can you run composer lint / composer format locally to fix these.

#8 @spacedmonkey
9 months ago

Refreshed my patch as a PR #2448.

#9 @spacedmonkey
9 months ago

  • Milestone set to Future Release

spacedmonkey commented on PR #2378:


9 months ago
#11

I have own version of this patch at - https://github.com/WordPress/wordpress-develop/pull/2448

This includes unit tests.

spacedmonkey commented on PR #2448:


9 months ago
#12

CC @pbearne

#13 @spacedmonkey
9 months ago

  • Owner set to spacedmonkey
  • Status changed from new to assigned

#14 @mukesh27
6 months ago

  • Keywords needs-refresh added

The PR needs to update against the trunk.

@spacedmonkey Is this ticket is in your to-do list for the upcoming release?

#15 @spacedmonkey
6 months ago

  • Milestone changed from Future Release to 6.1

@mukesh27 Yes it is. Why do you want to take over it?

#16 @mukesh27
6 months ago

Thanks for the quick reply and set the milestone. I want to know if it lands on the upcoming release.

Happy to help in any way I can also test the PR when it is ready.

#17 @spacedmonkey
6 months ago

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

In 53549:

Posts, Post Types: Add caching to _find_post_by_old_slug and _find_post_by_old_date functions.

Cache result of database queries in _find_post_by_old_slug and _find_post_by_old_date functions. This means that repeated requests to a url where the page is not found, will result in hitting a cache for sites running persistent object caching.

Props Spacedmonkey, dd32, mukesh27, pbearne, flixos90.
Fixes #36723.

spacedmonkey commented on PR #2378:


6 months ago
#18

Committed.

spacedmonkey commented on PR #2448:


6 months ago
#19

Committed.

#20 @desrosj
3 months ago

  • Keywords needs-dev-note added

There are a lot of caching related changes in 6.1 and a dev note should be written collecting them all. Marking this for inclusion.

Note: See TracTickets for help on using tickets.