WordPress.org

Make WordPress Core

Opened 6 years ago

Last modified 2 months ago

#21112 reviewing enhancement

Add pre_wp_unique_post_slug to override post slug handling

Reported by: coffee2code Owned by: johnbillion
Milestone: 5.0 Priority: normal
Severity: normal Version: 3.4
Component: Permalinks Keywords: has-patch
Focuses: Cc:

Description

As originally proposed in the comments of #20480 (and in fact further back in a comment for #14111), a pre_wp_unique_post_slug filter early in wp_unique_post_slug()could be useful and justified for some use cases rather than relying on the existing and late-firing wp_unique_post_slug filter for a couple reasons:

  • wp_unique_post_slug() (and thus this new filter) is not high use, so won't impact performance
  • wp_unique_post_slug() performs database queries (perhaps repeatedly until a unique slug is found), fires other filters, etc, all of which may be rendered moot if a hooking function's intent is to fully implement its own slug generation/handling logic. The pre slug could short-circuit that unnecessary effort.

The attached patch introduces the new filter in addition to adding related phpDocs.

Attachments (4)

21112.diff (1007 bytes) - added by coffee2code 6 years ago.
21112.2.diff (2.1 KB) - added by coffee2code 4 years ago.
Refreshed the 21112.diff patch. See associated comment for details.
21112.3.diff (2.3 KB) - added by javorszky 2 months ago.
Refreshed state of 21112.2.diff
21112.4.diff (2.3 KB) - added by javorszky 4 weeks ago.
Rebased on latest master (50cd98012e), and changed @since tag to 5.0

Download all attachments as: .zip

Change History (24)

@coffee2code
6 years ago

#1 @johnbillion
6 years ago

  • Cc johnbillion added

#2 @wycks
5 years ago

  • Cc bob........ellison@… added

@coffee2code
4 years ago

Refreshed the 21112.diff patch. See associated comment for details.

#3 @coffee2code
4 years ago

Added 21112.2.diff as a refresh of 21112.diff in order to:

  • Add inline docs for filter
  • Add unit test
  • Add braces
  • Remove unnecessary additions to function's docblock

#4 @johnbillion
4 years ago

  • Keywords reporter-feedback added

Out of interest, can you give a use case for this filter?

#5 @coffee2code
4 years ago

  • Keywords reporter-feedback removed

Well, likely most uses for the existing wp_unique_post_slug filter would be better served using the proposed pre_wp_unique_post_slug filter for the simple reason that if you are mucking around with unique slug detection (and regeneration due to a conflict), you're abandoning the built-in work being performed by wp_unique_post_slug().

Being early-firing (pre_wp_unique_post_slug) versus late-firing (wp_unique_post_slug) means you can skip the queries, filters firing, and code execution that you know will always be disregarded.

Some examples (geared towards custom-crafting the slugs the site uses) include:

  • changing of certain terms when appearing in slugs:
    e.g. "wp-is-fun" => "wordpress-is-fun", "wp-cant-be-shuttered" => "wp-cannot-be-shuttered"
  • removal of certain terms/phrases from appearing in slugs:
    e.g. "the-one-where-i-go-camping" => "where-i-go-camping"
  • force prepending of word(s) to all, or contextually to some, slugs:
    e.g. "wp-is-fun" => "mysite-wp-is-fun"
  • custom slug truncation length
    (Core has a 200 char limit, and while the internally used _truncate_post_slug() supports customizing that length via an argument, it is not otherwise filterable or customizable)
  • custom duplicate slug suffix
    (core attempts "-N" where N is a number tried until a unique slug is found)
    e.g. "the-weekly-report-15" => "the-weekly-report-fifteen"
  • custom slug rules per post type

In the end, yes, all this can be achieved with the existing wp_unique_post_slug filter, but it's a waste to run the 1(+n) queries and code execution in situations where custom handling will negate it all.

#6 @chriscct7
3 years ago

  • Keywords needs-docs added

The unit test functions need doc blocs

#7 @javorszky
3 years ago

Just to add a bit more emphasis on this: a library we're using creates posts to keep track of scheduled events (wp's cron is not good enough). By default the post_name is not populated, but even when it is, because WordPress would do it on ajax, there's no authenticated user, and the post is pending. wp_insert_post strips post_name even if one was passed through here.

Because there's no way to bypass this, on a large site when there are 60,000+ entries with the same empty post_name, the server just comes to a grinding halt. It's also not great with hosts like WPEngine, that have a hard limit for script execution. It's 60 seconds in WPEngine's case.

Currently the only way for us to do this is to essentially copy the entire wp_insert_post function, and remove all the things that would hit wp_unique_post_slug and call our version of it from within the library. It's not very WordPress way to be honest.

Being able to short circuit the function would be AMAZING.

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

This ticket was mentioned in Slack in #docs by morganestes. View the logs.


21 months ago

#9 @johnbillion
21 months ago

  • Milestone changed from Awaiting Review to Future Release
  • Owner set to johnbillion
  • Status changed from new to reviewing

#10 @CabGfx
19 months ago

Chiming in here, to basically echo what @javorszky mentioned above, as this:

[…] on a large site when there are 60,000+ entries with the same empty post_name, the server just comes to a grinding halt. It's also not great with hosts like WPEngine, that have a hard limit for script execution. It's 60 seconds in WPEngine's case.

is exactly what we're experiencing at the moment :[

#11 @johnjamesjacoby
16 months ago

My thoughts:

  • Needs seems genuine
  • Patch seems reasonable
  • Approach is similar to others we use in core
  • This could go in as-is, or the patch could be refreshed to include phpdoc in the tests like @chriscct7 mentioned

Paging @johnbillion, who marked himself as owner. Able to see this through?

#12 @negrusti
8 months ago

Chiming in as well, 190000+ entries, 17 actions from scheduler generate ~190000 MySQL queries:

https://fastserver.io/images/callgraph.png

Last edited 8 months ago by negrusti (previous) (diff)

#13 @javorszky
8 months ago

I hotfixed this on a huge site with Subscriptions on. The result was that renewals went from around ~200 / hr to ~980 / hr.

Can we please get this into WP Core officially?

<puppy eyes>

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


8 months ago

#15 @slaFFik
7 months ago

Recently bumped into this as well.

@johnbillion, can you please consider to move this ticket forward?

#16 @javorszky
2 months ago

Attaching a refreshed diff. Test passes as of writing this comment.

@javorszky
2 months ago

Refreshed state of 21112.2.diff

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


2 months ago

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


2 months ago

#19 @javorszky
2 months ago

  • Keywords needs-docs removed

Removing needs-docs, as test in 21112.2.diff has inline documentation.

#20 @SergeyBiryukov
2 months ago

  • Milestone changed from Future Release to 5.0

@javorszky
4 weeks ago

Rebased on latest master (50cd98012e), and changed @since tag to 5.0

Note: See TracTickets for help on using tickets.