Make WordPress Core

Opened 3 years ago

Last modified 6 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 3 years ago.
example screenshot
ticket.20419.patch (1.8 KB) - added by mintindeed 3 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)

@mintindeed3 years ago

example screenshot

@mintindeed3 years ago

comment:1 @ryan3 years ago

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

comment:2 @ryan3 years ago

  • Milestone changed from Awaiting Review to 3.5

@mintindeed3 years ago

updated version of original patch

@mintindeed3 years ago

new patch - preferred

comment:3 @mintindeed3 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.

comment:4 @nacin3 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.

comment:5 @mintindeed3 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)

@mintindeed3 years ago

replace $post_ID with $post object

@mintindeed3 years ago

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

comment:6 @mintindeed3 years ago

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

Fixed some issues with the previous patch(es).

comment:7 @nacin3 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.

@wonderboymusic2 years ago

comment:9 @wonderboymusic2 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

comment:10 @nacin2 years ago

  • Milestone changed from 3.7 to 3.8

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

comment:11 @wonderboymusic23 months ago

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

comment:12 @chriscct76 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.