WordPress.org

Make WordPress Core

Opened 7 years ago

Closed 6 weeks ago

Last modified 4 weeks ago

#21112 closed enhancement (fixed)

Add pre_wp_unique_post_slug to override post slug handling

Reported by: coffee2code Owned by: pento
Milestone: 5.1 Priority: normal
Severity: normal Version: 3.4
Component: Permalinks Keywords: has-patch has-dev-note
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 (5)

21112.diff (1007 bytes) - added by coffee2code 7 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 10 months ago.
Refreshed state of 21112.2.diff
21112.4.diff (2.3 KB) - added by javorszky 9 months ago.
Rebased on latest master (50cd98012e), and changed @since tag to 5.0
21112.5.diff (2.3 KB) - added by iCaleb 2 months ago.
Refreshed the patch per the above comments, along with some additional documentation tweaks.

Download all attachments as: .zip

Change History (36)

@coffee2code
7 years ago

#1 @johnbillion
6 years ago

  • Cc johnbillion added

#2 @wycks
6 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.


2 years ago

#9 @johnbillion
2 years ago

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

#10 @CabGfx
2 years 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
2 years 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
16 months ago

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

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

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

#13 @javorszky
16 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.


16 months ago

#15 @slaFFik
15 months ago

Recently bumped into this as well.

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

#16 @javorszky
10 months ago

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

@javorszky
10 months ago

Refreshed state of 21112.2.diff

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


10 months ago

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


10 months ago

#19 @javorszky
10 months ago

  • Keywords needs-docs removed

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

#20 @SergeyBiryukov
10 months ago

  • Milestone changed from Future Release to 5.0

@javorszky
9 months ago

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

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


4 months ago

#22 @swissspidy
4 months ago

Some notes on the patch and inline docs:

#23 @netweb
4 months ago

  • Keywords needs-refresh added
  • Milestone changed from 5.0 to 5.1

Per https://make.wordpress.org/core/2018/10/08/wordpress-5-0-for-contributors-and-committers/

See also Slack discussion here https://wordpress.slack.com/archives/C02RQBWTW/p1538986854000100

The 21112.4.diff will also need refreshing to updatethe PHPDoc @since block to 5.1 and updating per @swissspidy's comment above

@iCaleb
2 months ago

Refreshed the patch per the above comments, along with some additional documentation tweaks.

#24 @swissspidy
2 months ago

  • Keywords needs-refresh removed

Looks good to me!

I don't see anything that would hold this back.

#25 @netweb
7 weeks ago

  • Owner changed from johnbillion to swissspidy

Reassigning to @swissspidy to move this along for 5.1

#26 @pento
6 weeks ago

  • Owner changed from swissspidy to pento
  • Status changed from reviewing to accepted

#27 @pento
6 weeks ago

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

In 44454:

Permalinks: Add a pre_wp_unique_post_slug filter.

Returning a non-null value on this fillter will cause wp_unique_post_slug() to return early with that value, skipping potentially expensive database queries on some sites.

Props coffee2code, javorszky, iCaleb.
Fixes #21112.

#28 @ocean90
6 weeks ago

#45855 was marked as a duplicate.

#29 @lkraav
5 weeks ago

Any chance of this getting backported to 4.9-branch?

#30 @pento
5 weeks ago

No, only security fixes are backported to branches older than the current major version.

Note: See TracTickets for help on using tickets.