WordPress.org

Make WordPress Core

Opened 2 years ago

Last modified 5 months 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: minor Version: 3.3
Component: Permalinks Keywords: has-patch 3.9-early
Focuses: 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 (8)

Screen Shot 2012-04-11 at 2.30.21 PM.png (42.7 KB) - added by mintindeed 2 years ago.
example screenshot
ticket.20419.patch (1.8 KB) - added by mintindeed 2 years ago.
20419-extra-param.patch (1.8 KB) - added by mintindeed 21 months ago.
updated version of original patch
20419-override-post_status.patch (1.3 KB) - added by mintindeed 21 months ago.
new patch - preferred
20419-post-obj.patch (5.3 KB) - added by mintindeed 21 months ago.
replace $post_ID with $post object
20419-russian-reversal.patch (1.2 KB) - added by mintindeed 21 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 21 months ago.
20419.diff (9.0 KB) - added by wonderboymusic 9 months ago.

Download all attachments as: .zip

Change History (19)

mintindeed2 years ago

example screenshot

mintindeed2 years ago

comment:1 ryan21 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 ryan21 months ago

  • Milestone changed from Awaiting Review to 3.5

mintindeed21 months ago

updated version of original patch

mintindeed21 months ago

new patch - preferred

comment:3 mintindeed21 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 nacin21 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 mintindeed21 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;

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.

Version 0, edited 21 months ago by mintindeed (next)

mintindeed21 months ago

replace $post_ID with $post object

mintindeed21 months ago

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

comment:6 mintindeed21 months ago

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

Fixed some issues with the previous patch(es).

comment:7 nacin18 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.

wonderboymusic9 months ago

comment:9 wonderboymusic9 months 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 nacin7 months ago

  • Milestone changed from 3.7 to 3.8

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

comment:11 wonderboymusic5 months ago

  • Keywords 3.9-early added; 3.6-early removed
  • Milestone changed from 3.8 to Future Release
Note: See TracTickets for help on using tickets.