get_sample_permalink() passes the wrong post_status to wp_unique_post_slug()

[mintindeed]

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

[mintindeed]

example screenshot

[ryan]

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

[mintindeed]

updated version of original patch

[mintindeed]

new patch - preferred

[mintindeed]

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.

[nacin]

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.

[mintindeed]

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.

[mintindeed]

replace $post_ID with $post object

[mintindeed]

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

[mintindeed]

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

Fixed some issues with the previous patch(es).

[nacin]

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

[SergeyBiryukov]

Related: #22902

[wonderboymusic]

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

[nacin]

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

[chriscct7]

@wonderboymusic did you want to continue pursuing this?

[mintindeed]

Worked on this during WordCamp US 2015 contributor day. Seemed to work OK, but thanks to unit tests I found a couple regressions...now I'm in a black hole of regression testing. I hope to give a more thorough update in the next day or two.

[DrewAPicture]

@mintindeed Thanks for your work so far. If you have any questions or need guidance on how to move forward, let us know :-)

[mintindeed]

Thanks. There was one test that wasn't passing and was driving me nuts, finally sussed it and now I think this is good to go.

@wonderboymusic's patch changed the method signature for wp_unique_post_slug() so that it only contained a WP_Post object. After reviewing this ticket with @boonebgorges, Boone suggested leaving the first param ($slug), converting the second ($post_ID) to be either a WP_Post object or post ID, and deprecating the remaining params. The rationale is that there may be plugins or themes out there which use the wp_unique_post_slug() function on its own, maybe even as a utility function. This patch retains that utility, and it also means the changes to wp_unique_post_slug() are reduced (another good thing).

So in the end, here are the changes made by the latest patch (20419.3.diff):

• get_sample_permalink() sets WP_Post->filter = 'sample', and the method signature for wp_unique_post_slug() is modified to allow passing a WP_Post object. This is great because the WP_Post object contains all the meta information we need to generate a unique slug, and also lets use pass along that this is a sample object and therefore we can safely bypass some of the prior post status hacks (#22902, #30910).
• This gets rid of the need for the hack for #22902 (r24206) and the similar hack in get_sample_permalink() which appears to date back to its inception. I tested for regression and it appeared fine, I could not reproduce using the steps in the ticket.
• Added new unit tests for this bug.
• Added this ticket number to existing unit tests for #30910, for regression testing.
• Added this ticket number to existing unit tests for #18306, for regression testing. #18306 isn't really a related issue, but when testing my patch I found some scenarios where I accidentally regressed these changes.
• #22902 doesn't have unit tests; I didn't add any because I'm pretty sure the existing+new tests for get_sample_permalink() and wp_unique_post_slug() cover everything.
• I updated existing unit tests which called wp_unique_post_slug() to use the new function signature, with the exception of a new test which verifies backwards compatibility of the params. This seemed like the right way to do it, rather than adding @expectedDeprecated wp_unique_post_slug to all the existing tests. I still added @expectedDeprecated wp_unique_post_slug to a few existing tests for other tickets, just because changing it to the new params would change the nature of the test in a somewhat significant way.

Unit Tests

phpunit --group 30910
Passed

phpunit --group 20419
Passed

phpunit --group post
Passed

phpunit --group admin
Passed

phpunit --group ajax
Passed

[chriscct7]

Needs a @since with what/when was deprecated on the wp_unique_post_slug function

[boonebgorges]

In 37443:

Tests: Use factory method to generate fixtures for wp_unique_post_slug() tests.

Props mintindeed.
See #20419.

[boonebgorges]

I spent some time with this ticket tonight and here's where we are :-D

20419.4.diff​ contains the changes necessary to change the function signature of wp_unique_post_slug() so that the second parameter is $post. This includes the changes to wp_unique_post_slug() itself, changes to the places in core where we use the function, and modifications to our unit tests to avoid notices.

My one misgiving about the change in 20419.4.diff​ is that I don't want to start throwing zillions of deprecated notices for plugins using wp_unique_post_slug() in the old way. However, I'm halfway through crawling the plugins repo for uses of wp_unique_post_slug(), and it looks like it's not actually used very often at all.

Other than that, I think the changes in 20419.4.diff​ are unproblematic, and should go into 4.6 either way.

The second attachment is 20419.5.diff​. This contains what's left of the patch: the change to wp_unique_post_slug() so that 'draft' and 'pending' are no longer excluded from the uniqueness checks; the changes to get_sample_permalink(); and the necessary changes to unit tests. I'll need some more time to think about how and whether these changes are the right kind of fix for the issue described in this ticket.

There was some stuff in 20419.3.diff​ that I left out. Most notably, there was a modification related to #30910. Not sure if that was supposed to be there, but it's not directly related to this ticket, so I've left it out.

@mintindeed - Could I ask you to do the following:

1. Look over 20419.4.diff​ to make sure I've covered all the bases related to changing the function signature
2. Now that 20419.5.diff​ has distilled your fixes to a couple of easy-to-read lines, let me know if you still think they're the right approach (it's possible that I've missed something out of the larger earlier patch, too).

Putting this into the 4.6 milestone for further review.

[boonebgorges]

In the interest of making sure that at least part of this ticket can be achieved for 4.6, I've opened #37068 to handle the function signature change.