Make WordPress Core

Opened 12 years ago

Closed 6 years ago

Last modified 6 years ago

#21112 closed enhancement (fixed)

Add pre_wp_unique_post_slug to override post slug handling

Reported by: coffee2code's profile coffee2code Owned by: pento's profile 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 12 years ago.
21112.2.diff (2.1 KB) - added by coffee2code 10 years ago.
Refreshed the 21112.diff patch. See associated comment for details.
21112.3.diff (2.3 KB) - added by javorszky 7 years ago.
Refreshed state of 21112.2.diff
21112.4.diff (2.3 KB) - added by javorszky 6 years ago.
Rebased on latest master (50cd98012e), and changed @since tag to 5.0
21112.5.diff (2.3 KB) - added by iCaleb 6 years ago.
Refreshed the patch per the above comments, along with some additional documentation tweaks.

Download all attachments as: .zip

Change History (36)

@coffee2code
12 years ago

#1 @johnbillion
12 years ago

  • Cc johnbillion added

#2 @wycks
11 years ago

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

@coffee2code
10 years ago

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

#3 @coffee2code
10 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
10 years ago

  • Keywords reporter-feedback added

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

#5 @coffee2code
10 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
9 years ago

  • Keywords needs-docs added

The unit test functions need doc blocs

#7 @javorszky
9 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 9 years ago by javorszky (previous) (diff)

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


8 years ago

#9 @johnbillion
8 years ago

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

#10 @CabGfx
8 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
8 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
7 years ago

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

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

Version 1, edited 7 years ago by negrusti (previous) (next) (diff)

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


7 years ago

#15 @slaFFik
7 years ago

Recently bumped into this as well.

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

#16 @javorszky
7 years ago

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

@javorszky
7 years ago

Refreshed state of 21112.2.diff

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


7 years ago

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


7 years ago

#19 @javorszky
7 years ago

  • Keywords needs-docs removed

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

#20 @SergeyBiryukov
7 years ago

  • Milestone changed from Future Release to 5.0

@javorszky
6 years 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.


6 years ago

#22 @swissspidy
6 years ago

Some notes on the patch and inline docs:

#23 @netweb
6 years 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
6 years ago

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

#24 @swissspidy
6 years ago

  • Keywords needs-refresh removed

Looks good to me!

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

#25 @netweb
6 years ago

  • Owner changed from johnbillion to swissspidy

Reassigning to @swissspidy to move this along for 5.1

#26 @pento
6 years ago

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

#27 @pento
6 years 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 years ago

#45855 was marked as a duplicate.

#29 @lkraav
6 years ago

Any chance of this getting backported to 4.9-branch?

#30 @pento
6 years ago

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

Note: See TracTickets for help on using tickets.