WordPress.org

Make WordPress Core

Opened 14 months ago

Last modified 6 weeks ago

#20419 new defect (bug)

get_sample_permalink() passes the wrong post_status to wp_unique_post_slug()

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

Description

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 (7)

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

Download all attachments as: .zip

Change History (15)

mintindeed14 months ago

example screenshot

mintindeed14 months ago

comment:1 ryan11 months ago

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

comment:2 ryan11 months ago

  • Milestone changed from Awaiting Review to 3.5

mintindeed11 months ago

updated version of original patch

mintindeed11 months ago

new patch - preferred

comment:3 mintindeed11 months 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 nacin11 months 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 mintindeed11 months 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 11 months ago by mintindeed (previous) (diff)

mintindeed11 months ago

replace $post_ID with $post object

mintindeed11 months ago

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

comment:6 mintindeed11 months ago

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

Fixed some issues with the previous patch(es).

comment:7 nacin7 months 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.

Note: See TracTickets for help on using tickets.