Make WordPress Core

Opened 4 years ago

Last modified 10 days ago

#20419 new defect (bug)

get_sample_permalink() passes the wrong post_status to wp_unique_post_slug()

Reported by: mintindeed Owned by:
Milestone: Future Release Priority: normal
Severity: normal Version: 3.3
Component: Permalinks Keywords: has-patch
Focuses: Cc:


get_sample_permalink() fakes a 'publish' post_status so that it can create a pseudo-permalink for drafts and scheduled posts. Also wp_unique_post_slug() bails when the post_status is 'draft', 'pending', or 'auto-draft', so faking a 'publish' post_status helps there too.

However, that means the incorrect post_status is passed to the 'wp_unique_post_slug' filter at times, creating unexpected behaviour -- namely sometimes the $post_status parameter of the wp_unique_post_slug filter will report a non-published post is published.

The best approach here seems to be to let the wp_unique_post_slug() function know when a fake $post_status is being passed to it, what that status is so it can be passed to the wp_unique_post_slug filter. That way anybody using the filter will always get the actual post status all the time without having to do anything special.

Related: #14111

Attachments (8)

Screen Shot 2012-04-11 at 2.30.21 PM.png (42.7 KB) - added by mintindeed 4 years ago.
example screenshot
ticket.20419.patch (1.8 KB) - added by mintindeed 4 years ago.
20419-extra-param.patch (1.8 KB) - added by mintindeed 3 years ago.
updated version of original patch
20419-override-post_status.patch (1.3 KB) - added by mintindeed 3 years ago.
new patch - preferred
20419-post-obj.patch (5.3 KB) - added by mintindeed 3 years ago.
replace $post_ID with $post object
20419-russian-reversal.patch (1.2 KB) - added by mintindeed 3 years ago.
Only check for "publish" because that's what we actually care about
20419.2012-08-05.patch (8.5 KB) - added by mintindeed 3 years ago.
20419.diff (9.0 KB) - added by wonderboymusic 2 years ago.

Download all attachments as: .zip

Change History (20)

4 years ago

example screenshot

#1 @ryan
3 years ago

Alternatively, pass a post_status of 'sample' or some other dummy to wp_unique_post_slug() and add handling for it.

#2 @ryan
3 years ago

  • Milestone changed from Awaiting Review to 3.5

3 years ago

updated version of original patch

3 years ago

new patch - preferred

#3 @mintindeed
3 years ago

One reason for submitting the original/real status is so that plugins can act differently based on the status of the post, e.g. for custom statuses. I ran into this bug when implementing an SEO feature that allowed an "seo slug" to override the default permalink based on title. If someone was using custom statuses etc, having access to the original status (vs a placeholder status) could be useful.

I've uploaded two patches made against svn r21284 --

20419-extra-param.patch is an updated version of my original patch

20419-override-post_status.patch based on Ryan's suggestion attempts to override the existing status parameter while still passing the actual post status. I kinda like this approach better because I think it's a bit janky to add new parameters.

#4 @nacin
3 years ago

wp_unique_post_slug() takes a $post_ID, but what if we turned that into a $post object? Then we could pass in the object we already have, which will have filter->sample.

#5 @mintindeed
3 years ago

Not sure it makes sense to do that. It would be more efficient, but it would kinda deprecate post_status and introduce a bunch of (unnecessary?) changes.

In order to not break backwards compatibility *too* much it would look something like:

function wp_unique_post_slug( $slug, $post_ID, $post_status, $post_type, $post_parent ) {
	if ( is_object($post_status) )
		$post_status = $post->post_status;
	elseif ( in_array( $post_status, array( 'draft', 'pending', 'auto-draft' ) ) )
		return $slug;

but that elseif really should be an "else" in order for it to work as expected when passing an object -- otherwise you would be changing the API where "version 1" is a string and only allows certain statuses, and "version 2" is an object and allows any status. I guess you would put in a piece of branching logic to determing whether you're using "version 1 (string)" or "version 2 (object)" and if your'e using an obj, allow any string as value assuming that the user is overriding the behaviour via plugin? That seems a little dangerous.

Maybe turn it on its head, and just make sure the status isn't "publish":

function wp_unique_post_slug( $slug, $post_ID, $post_status, $post_type, $post_parent ) {
	if ( isset($post_ID->post_status) )
		$post_status = $post_ID->post_status;
	if ( 'publish' === $post_status )
		return $slug;

I kinda like this, included ad "20419-russian-reversal.patch"

20419-post-obj.patch -- This changes the behaviour a little bit, because instead of allowing a whitelist of post_status it's allowing anything that's not "publish". That's OK IMO because really the only status I know about that matters w/r/t post_status is "publish" because that's the "it's public, therefore it's permanent" status. Including a patch with that scenario: 20419-russian-reversal.patch -- I don't like this nearly as much, and if you see the diff you'll understand why. Unless I'm being dense and missing something, there are a lot of maybe-edge-cases it doesn't account for -- namely if it's falling back to faking a $post object when an ID-only is passed to it, it could break things. Also it has to add an extra param to the "wp_unique_post_slug" filter in order to not break things, and allow plugins to take advantage of the new functionality.

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

3 years ago

replace $post_ID with $post object

3 years ago

Only check for "publish" because that's what we actually care about

#6 @mintindeed
3 years ago

Updated patch for latest version: 20419.2012-08-05.patch

Fixed some issues with the previous patch(es).

#7 @nacin
3 years ago

  • Keywords 3.6-early added
  • Milestone changed from 3.5 to Future Release

Lots of churn here. Let's work with this early 3.6.

2 years ago

#9 @wonderboymusic
2 years ago

  • Milestone changed from Future Release to 3.7

Patch was exploding, I refreshed it and fixed a few places where $post_parent should have been $post->post_parent

#10 @nacin
2 years ago

  • Milestone changed from 3.7 to 3.8

20419.diff is a good start, could use some cleanup and simplification.

#11 @wonderboymusic
23 months ago

  • Keywords 3.9-early added; 3.6-early removed
  • Milestone changed from 3.8 to Future Release

#12 @chriscct7
10 days ago

  • Keywords 3.9-early removed
  • Severity changed from minor to normal

@wonderboymusic did you want to continue pursuing this?

Note: See TracTickets for help on using tickets.