WordPress.org

Make WordPress Core

Opened 4 years ago

Last modified 4 months ago

#20419 reviewing defect (bug)

get_sample_permalink() passes the wrong post_status to wp_unique_post_slug()

Reported by: mintindeed Owned by: boonebgorges
Milestone: Future Release Priority: normal
Severity: normal Version: 3.3
Component: Permalinks Keywords: has-patch
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 (12)

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 4 years ago.
updated version of original patch
20419-override-post_status.patch (1.3 KB) - added by mintindeed 4 years ago.
new patch - preferred
20419-post-obj.patch (5.3 KB) - added by mintindeed 4 years ago.
replace $post_ID with $post object
20419-russian-reversal.patch (1.2 KB) - added by mintindeed 4 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 4 years ago.
20419.diff (9.0 KB) - added by wonderboymusic 3 years ago.
20419.2.diff (1.2 KB) - added by boonebgorges 10 months ago.
20419.3.diff (22.0 KB) - added by mintindeed 10 months ago.
20419.4.diff (18.2 KB) - added by boonebgorges 4 months ago.
20419.5.diff (2.4 KB) - added by boonebgorges 4 months ago.

Download all attachments as: .zip

Change History (33)

@mintindeed
4 years ago

example screenshot

#1 @ryan
4 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
4 years ago

  • Milestone changed from Awaiting Review to 3.5

@mintindeed
4 years ago

updated version of original patch

@mintindeed
4 years ago

new patch - preferred

#3 @mintindeed
4 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
4 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
4 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;

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 4 years ago by mintindeed (next)

@mintindeed
4 years ago

replace $post_ID with $post object

@mintindeed
4 years ago

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

#6 @mintindeed
4 years ago

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

Fixed some issues with the previous patch(es).

#7 @nacin
4 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.

@wonderboymusic
3 years ago

#9 @wonderboymusic
3 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
3 years ago

  • Milestone changed from 3.7 to 3.8

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

#11 @wonderboymusic
3 years ago

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

#12 @chriscct7
12 months ago

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

@wonderboymusic did you want to continue pursuing this?

#13 @mintindeed
10 months ago

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.

#14 @DrewAPicture
10 months ago

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

@mintindeed
10 months ago

#15 @mintindeed
10 months ago

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

This ticket was mentioned in Slack in #core by chriscct7. View the logs.


8 months ago

#17 @chriscct7
8 months ago

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

#18 @boonebgorges
8 months ago

  • Owner set to boonebgorges
  • Status changed from new to reviewing

#19 @boonebgorges
4 months ago

In 37443:

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

Props mintindeed.
See #20419.

#20 @boonebgorges
4 months ago

  • Milestone changed from Future Release to 4.6

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.

#21 @boonebgorges
4 months ago

  • Milestone changed from 4.6 to Future Release

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.

Note: See TracTickets for help on using tickets.