Opened 7 years ago
Last modified 8 weeks ago
#47988 new defect (bug)
Unexpected behaviour when draft post has the same page_name as published post
| Reported by: |
|
Owned by: | |
|---|---|---|---|
| Milestone: | Future Release | Priority: | normal |
| Severity: | normal | Version: | 2.8 |
| Component: | Posts, Post Types | Keywords: | good-first-bug has-test-info has-patch has-unit-tests needs-testing |
| Focuses: | Cc: |
Description
What steps should be taken to consistently reproduce the problem?
wp rewrite structure '/%postname%/' wp post create --post_title="Example post title" --post_status=publish --post_name=my-chosen-post-name wp post create --post_title="A draft post" --post_status=draft --post_name=my-chosen-post-name
Visit http://www.example.com/my-chosen-post-name
In case it's relevant to the ticket, what is the expected output or result?
We're expecting the published post to be displayed.
What did you see instead?
If you're authenticated and have permission to view drafts, the draft post will populate the Global $post object and be displayed.
Anonymous users will get a 404 page or the browser will throw a Too many redirects error.
Does the problem occur even when you deactivate all plugins and use the default theme?
Yes.
Please provide any additional information that you think we'd find useful. (OS and browser for UI defects, server environment for crashes, etc.)
The core behaviour of the WP Admin post edit screen doesn't allow us to get into this state because post_name values are not stored for a post until it transitions to the publish post status. When this transition does happen wp_unique_post_slug() ensures the post_name being saved unique.
We first encountered this issue via the Yoast SEO plugin metabox which allows a Slug to be saved for draft posts. As you can see with the WP-CLI commands above, however, there are other ways of getting to this state.
The draft post is loaded for authenticated requests because the default query vars order_by => post_date and order => DESC means the draft post created after the published post populates the WP_Query->post property.
Anonymous requests are not able to view the draft post, so before returning a 404, redirect_canonical() calls redirect_guess_404_permalink() which builds a query for a published post where page_name is LIKE the post name and finds the published post and redirects to it... and the loop continues.
Attachments (1)
Change History (10)
#2
@
6 months ago
- Keywords good-first-bug needs-unit-tests needs-patch has-test-info added
- Milestone changed from Awaiting Review to Future Release
- Version set to 2.8
Reproduction Report
Description
✅ This report validates that the issue can be reproduced.
Environment
- WordPress: 6.9-alpha-60093-src
- PHP: 8.2.29
- Server: nginx/1.29.1
- Database: mysqli (Server: 8.4.6 / Client: mysqlnd 8.2.29)
- Browser: Chrome 140.0.0.0
- OS: Windows 10/11
- Theme: Twenty Twenty-Five 1.3
- MU Plugins: None activated
- Plugins:
- Test Reports 1.2.0
Testing Instructions
Use the instructions provided in the OP
Actual Results
- ✅ Error condition occurs (reproduced).
Additional Notes
This can be tracked down to #9726/[11467]. We don't have a pinch of context of why it was decided to add this here. 16 years later, things have changed massively, and this has been left behind. Looking at the current unit tests, they were published in #5305/[32604]. Browsing through the ticket, I cannot really see a single reference to the particular unit test that is actually testing this: test_whitelisted_post_statuses_should_not_be_forced_to_be_unique
This brings me to a question: Why provisional post statuses (draft, auto-draft and pending) were meant to not be forced to be unique? Isn't this causing the conflict? I'm assuming that probably in the origin, they were expecting that permalinks were not considering provisional posts, hence not using the slug. That could be a fix in the permalinks section.
But there is another take, like @ajfleming suggests: when creating this through the admin interface, provisional posts are saved without a post_name. So I think we should expect the same behaviour in this particular case. On the provisional posts, since post_name is expected to be empty, Gutenberg picks the potential slug from the post, but this doesn't mean that the post_name gets stored.
Reviewing the idea of the patch 47988-20190906-1249.diff, looks good at first.
The problem is that this conditional encompasses several scenarios, not only the three provisional statuses, but also post types like "revision", which actually have a preset slug, and other exotic post types like user_request which in fact, this breaks the functionality if we force set the slug to empty.
So this little conditional has become a little of a hodgepodge that we must untangle.
My review for the current patch is simply, extracting the 3 provisional statuses, setting them to empty, and leaving the rest as-is.
Note that current unit tests must also be fixed, as they are only accounting for the provisional states, and expecting an untouched slug. So first we must introduce in the data provider the additional statuses. Also, the new expected result with an "empty" slug can be set in the same data provider.
If anyone builds the patch taking this information in mind, don't submit a .patch, but a GitHub PR. Moreover, after submitted to GitHub, please comment here after this so I can receive a notification (GitHub PR does not notify).
#3
@
6 months ago
Hello @SirLouen,
I tried implementing what you proposed and it led me to dig deeper into the subject of WordPress draft post status constraints. For now, I don't have a patch to submit because I think there may be a collective discussion and decision to be made on this subject.
Feel free to tell me if it is not ok to answer a ticket without patch or if the type of answer you'll find below should be made somewhere else (Slack, Github ..?).
About the proposed solution in your last answer
It does not seem desirable to implement it because there are already tests that expect certain behaviors (and probably third party plugins and themes):
- After being updated with wp_insert_post and without an explicit
post_nameinwp_insert_postargs, thepost_nameshouldn't be emptied (cf.test_wp_insert_post_for_customize_changeset_should_not_drop_post_name, linked to https://core.trac.wordpress.org/changeset/38810 and https://make.wordpress.org/core/2016/10/12/customize-changesets-technical-design-decisions/). - After being untrashed, a post have the
draftstatus. The expected behavior for thepost_nameduring this state change is that thepost_nameshould stay the same (cf.test_trashed_posts_original_post_name_should_be_reassigned_after_untrashing, linked to https://core.trac.wordpress.org/ticket/11863)
Based on these findings I searched for other uses of wp_unique_post_slug accross the WordPress API and found this interesting occurence :
If you check /wp-includes/rest-api/endpoints/class-wp-rest-posts-controller.php l.749 (search for "wp_unique_post_slug() returns the same slug for 'draft' or 'pending' posts."), you'll see that people implementing the REST post creation mechanics had a similar problem.
They opted for forcing the post_status to publish in the wp_unique_post_slug post_status args in order to get around this behavior.
Possible solutions
Based on this solution, I think we have several solutions to solve the problem raised by @ajflemming :
- introduce a similar exception as the one in the WP REST API into the WP CLI API;
- rethink the draft post status lifecycle and conditions in order to address this problem;
- not trying to untangle draft post_status functioning as the issue described here could be addressed in the function used to retrieve posts (see https://core.trac.wordpress.org/ticket/61996 PR proposal)
The first solution seems to me to be a quick and dirty fix.
Even though the second solution is more tedious, I think it's the best one. Choosing this solution would exclude this ticket from the #good-first-bug category as it would need a broad knowledge of WP codebase and its history.
The third solution seems to me to address the problem satisfactorily but superficially ( as it doesn't adresse the "draft shouldn't unique post_name" question).
What could be a good next step
Before rushing to a solution, it could be interesting to gather the experimented and codebase history connoisseurs in order to address this question : "Why shouldn't we assign a post_name to a post with a draft post_status ?". Indeed, as a WP (wannabe) beginner contributor, after putting time and effort digging in the code and Trac I was not able to give a satisfying answer to this question that seems to be the heart of the problem here.
For all of these reasons, I think the subject deserve a discussion amongst experienced WordPress codebase contributor.
What do you think about it ?
#5
@
6 months ago
Hello @SirLouen,
I'm not sure I fully understand your question.
I listed this ticket/PR as a possible solution to the problem if it is not possible / desirable to untangle the whole draft post status lifecycle and conditions.
If this ticket/PR does what it state it does the symptoms of the problems here (draft taking precedence other published posts) will also be addressed.
Thank for your answers
#6
@
6 months ago
@paulbonneau in that ticket, there is a PR. I have not tested yet and I wonder if you have done
Do you think that this PR solves the problem in this ticket also?
I have 3 tickets regarding post uniqueness that may collide in one or another point: #39942, #47552 and that one (#61996) and this one.
I have a task in my agenda to review them all and see if we can do a single patch to solve them, or if they need independent fixes. If you want to give it a check, I will try to follow along with you.
#7
@
8 weeks ago
I'd like to work on this ticket.
After reviewing the discussion between @SirLouen and @paulbonneau, I'll investigate the related tickets (#39942, #47552, #61996) to understand if there's a comprehensive solution that addresses all post uniqueness issues together.
I'll start by:
- Testing the reproduction steps to confirm the issue
- Reviewing the existing patch and the concerns raised
- Checking if the PR in #61996 resolves this issue as well
- Proposing a solution via GitHub PR
Will update here with my findings.
This ticket was mentioned in PR #10759 on WordPress/wordpress-develop by gsusI.
8 weeks ago
#8
- Keywords has-patch has-unit-tests added; needs-unit-tests needs-patch removed
## Summary
- Prefer published posts when resolving singular slug/path queries without explicit post_status.
- Add a unit test covering draft vs published slug conflicts.
## Testing
- ./vendor/bin/phpunit --filter test_singular_name_query_prefers_published_post_over_draft_with_same_slug tests/phpunit/tests/post/query.php
- ./vendor/bin/phpunit tests/phpunit/tests/post/query.php
#9
@
8 weeks ago
- Keywords needs-testing added
After investigating, I've chosen to implement solution 3) from @paulbonneau's analysis
(fixing at the query level rather than modifying wp_unique_post_slug).
Reasons for this approach:
- It doesn't break existing behaviours that plugins/themes rely on
- It doesn't modify how draft posts store their post_name
- It specifically fixes the symptom (wrong post returned) without changing the slug lifecycle that has been stable for many years
- It's less risky/invasive than modifying wp_unique_post_slug behavior
The PR modifies WP_Query to prefer published posts when resolving singular slug
queries without explicit post_status.
Regarding the related tickets @SirLouen mentioned:
- #61996: Has its own PR—#9675(https://github.com/WordPress/wordpress-develop/pull/9675). addressing get_page_by_path() - different function
- #47552: Race condition on insert - different problem, needs separate solution
- #39942: Slug stealing on restore - different problem, needs wp_unique_post_slug fix
I believe this ticket can be resolved independently with PR #10759(https://github.com/WordPress/wordpress-develop/pull/10759).
Does this approach work for you, @SirLouen, or would you prefer I pursue one of the other solutions?
A possible solution would
wp_unique_post_slug()returning an empty string for draft posts instead of the passed value.