Make WordPress Core

Opened 3 years ago

Closed 19 months ago

Last modified 18 months ago

#52422 closed defect (bug) (fixed)

Create a draft with the same slug as an existing post, the existing post will be 404.

Reported by: toro_unit's profile Toro_Unit Owned by: audrasjb's profile audrasjb
Milestone: 6.1 Priority: normal
Severity: normal Version:
Component: Posts, Post Types Keywords: has-patch has-unit-tests has-testing-info early dev-feedback commit
Focuses: Cc:

Description

Step-by-step reproduction instructions

  1. create custom post type. and set 'show_in_rest' => true
<?php
function custom_init() {
        $args = array(
                'public' => true,
                'label'  => 'Books',
                'show_in_rest' => true,
        );
        register_post_type( 'book', $args );
}
add_action( 'init', 'custom_init' );
  1. Publish new post of the post type.
  2. Create draft post of the post type with same slug.
  3. Go to the published post. but show draft post. if logged out, 404.

Using the block editor, a page can also create drafts with the same slug, which causes the same problem.

Attachments (3)

issue.gif (2.1 MB) - added by Toro_Unit 3 years ago.
A gif animation of the issue.
issue-on-page.gif (4.3 MB) - added by Toro_Unit 3 years ago.
reproduced on page.
52422.refresh-of-PR-1333.diff (3.6 KB) - added by costdev 23 months ago.
Refresh of PR 1333. Tidied up tests, added $message parameters. Tidied up source and improved the explanation.

Change History (44)

#1 @h2ham
3 years ago

It also happened in default posts.

  1. Set permalink to %postname%.
  2. Publish new post of default posts.
  3. Create draft post of the default post with same slug.
  4. Go to the published post. but show draft post. If logged out, 404.

#2 @peterwilsoncc
3 years ago

I've attempted to reproduce this on both 5.6 and trunk without success. I've tried with both a CPT per the original post and standard posts per the follow up comment.

Are either of you able to reproduce this with all plugins disabled and using a default theme such as Twenty Twenty-One?

#3 @Toro_Unit
3 years ago

@peterwilsoncc Thanks.

No plugins are used, and the theme is Twenty Twenty One. reproduced on WordPress 5.5 and 5.6.

When creating a post, please use the block editor.
Quick Edit or Classic Editor don't allow us to create a post with the same slug as an existing post.

@Toro_Unit
3 years ago

A gif animation of the issue.

@Toro_Unit
3 years ago

reproduced on page.

This ticket was mentioned in PR #1333 on WordPress/wordpress-develop by torounit.


3 years ago
#4

  • Keywords has-patch has-unit-tests added; needs-patch removed

Trac ticket: https://core.trac.wordpress.org/ticket/52422.

Using the block editor, a draft can have the same slug as a post that already exists.

There is a part in WP_Query that uses get_page_by_path, so it may load the wrong post.

A hack similar to wp_ajax_inline_save, fixed so that the slug is different.

https://i0.wp.com/user-images.githubusercontent.com/1908815/120768076-c2bda580-c556-11eb-9b2b-88333c6e3af6.gif

Shizumi commented on PR #1333:


3 years ago
#5

@torounit
I think the problem with this bug is not that it can be saved with the same slug, but that articles that are not published are called.

#6 @Toro_Unit
3 years ago

get_page_by_path is used in WP_Query.

  • Draft page ( id: 1, slug: foo, status: draft )
  • Published page ( id: 2, slug: foo, status: publish )

If the above two posts exist, get_page_by_path will get the draft page. That post will be set to queried_object.

https://github.com/WordPress/wordpress-develop/blob/5.7.2/src/wp-includes/class-wp-query.php#L1001

#7 @SergeyBiryukov
3 years ago

  • Component changed from General to Posts, Post Types

#8 @SergeyBiryukov
3 years ago

  • Milestone changed from Awaiting Review to 5.9

#9 @hellofromTonya
2 years ago

  • Keywords needs-refresh has-testing-info needs-testing added
  • Milestone changed from 5.9 to 6.0

Updating the keywords:

  • Marking as needs-refresh as there are merge conflicts
  • Marking as has-testing-info as there are step-by-step instructions
  • Marking as needs-testing as I too am unable to reproduce the reported problem

I'm not able to reproduce this problem either. It needs more investigation. As 5.9 Beta 1 is in 2 days, moving it to 6.0.

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


23 months ago

#11 @costdev
23 months ago

  • Keywords needs-testing removed

Test Report

Environment

  • Server: Apache (Linux)
  • WordPress: 6.0-alpha-52448-src
  • Browser: Chrome 99.0.4844.84
  • OS: Windows 10
  • Theme: Twenty Seventeen
  • Plugins: None activated

Steps to Reproduce

  1. Navigate to Settings > Permalinks.
  2. Ensure the permalink structure is set to Post name. Click Save Changes.
  3. Navigate to Posts > Add New.
  4. Name the post "Test post". Click Publish.
  5. Navigate to Posts > Add New.
  6. Name the post "Test post 2". Click Save draft.
  7. On the right hand side, scroll down and edit the slug from "test-post-2" to "test-post". Click Save draft.
  8. Navigate to Posts.
  9. Click View on "Test post". Notice that the title is "Test post 2". ✅
  10. Copy the link into a Private Browsing/Incognito window. Notice it 404s. ✅
  11. Apply 52422.refresh-of-PR-1333.diff (a refresh of PR 1333).
  12. Run npm run build:dev.
  13. Trash and permanently remove "Test post" and "Test post 2".
  14. Repeat steps 1-7. Notice that "test-post" changes to "test-post-2", preventing the use of an existing slug. ✅
  15. Continue to step 8 through 10. Notice that both issues in 9 and 10 no longer occur. ✅

Results

  1. Issue reproduced. ✅
  2. 52422.refresh-of-PR-1333.diff (a refresh of PR 1333) resolves the issue.
Last edited 20 months ago by costdev (previous) (diff)

@costdev
23 months ago

Refresh of PR 1333. Tidied up tests, added $message parameters. Tidied up source and improved the explanation.

#12 @costdev
22 months ago

  • Keywords commit added; needs-refresh removed

Patch still applies cleanly against trunk. Adding commit.

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


22 months ago

#14 follow-up: @costdev
22 months ago

  • Keywords early added
  • Milestone changed from 6.0 to 6.1

As 6.0 RC1 is starting in less than an hour, I'm moving this to the 6.1 milestone and adding the early keyword so that it has plenty of testing/soak time.

Committers: This can be committed to trunk after we branch 6.0.

To clarify: 52422.refresh-of-PR-1333.diff is the commit candidate.

Last edited 22 months ago by costdev (previous) (diff)

#15 @antonvlasenko
22 months ago

Test Report

Server: macOs 12.2
WordPress: 6.1-alpha-53344-src
Browser: Safari 15.4
OS: macOs 12.2
Theme: Twenty Twenty-One / Twenty Twenty-Two
Plugins: None activated

I'm able to reproduce the issue and the PR above fixed it in my tests.
Before the fix: watch the screen recodring
After the fix: watch the screen recording

Last edited 22 months ago by antonvlasenko (previous) (diff)

#16 in reply to: ↑ 14 @azaozz
22 months ago

Replying to costdev:

To clarify: 52422.refresh-of-PR-1333.diff is the commit candidate.

Probably missing something but why the $prepared_post->post_type = $this->post_type; is removed in the patches?

#17 @ironprogrammer
22 months ago

Thanks, @costdev!

Test Report

Patch 52422 (PR 1333 refresh via @costdev) 👍🏻

Env

  • WordPress 6.1-alpha-53344-src
  • Safari 15.4
  • Chrome 101.0.4951.54
  • macOS 12.3.1 (Monterey)
  • Theme: Twenty Twenty-Two
  • Gutenberg DISABLED 🔴

Steps to Test

  1. Ensure permalink structure is set to "Post name" in Settings > Permalinks.
  2. Under Posts > Add New create a new post with the title "Test Post" and Publish.
  3. In another tab, verify that the post is published at <your site url>/test-post/.
  4. Navigate to Posts > Add New and create another new post with the title "Test Post 2", and click Save draft (DO NOT click Publish).
  5. Find the "Permalink" metabox on the right-hand side of the post editor and update the "URL Slug" to test-post by removing the -2 -- this will match the slug of the first new post. Click Save draft again (DO NOT click Publish).
  6. In the second tab, refresh the page at <your site url>/test-post/.
  7. Observe that the post title displayed is now "Test Post 2", indicating that the draft post is overriding the original post's slug. 🐞
  8. Open the page in Private Browsing/Incognito mode and observe that a 404 error (not found) is returned, as the draft post is not published. 🐞
  9. Apply patch. 🛠
  10. From the main Posts page, "Move to Trash" and "Delete Permanently" both "Test Post" and "Test Post 2".
  11. Repeat Steps 2 through 5.
  12. Observe that the "Permalink" metabox in the post editor refreshes the "VIEW POST" link to <your site url>/test-post-2/. ✅
  13. In the second tab, refresh the page at <your site url>/test-post/.
  14. Observe that the post title remains "Test Post", indicating the draft has not overridden the published post's slug. ✅
  15. In Private Browsing/Incognito, observe that the originally published post is displayed (404 error has been mitigated). ✅

Expected Results

  • I was able to reproduce the reported issue (see Steps 7-8). ✅
  • The patch corrects the identified issue (see Steps 12-15). ✅

Additional Notes

  • The above reproduction steps do not apply to Pages, but only to Posts. Pages seem unaffected by this issue (i.e. a draft page does not override an existing published page slug).
Last edited 22 months ago by ironprogrammer (previous) (diff)

#18 @hellofromTonya
22 months ago

  • Keywords commit removed

As there is a valid question about why $prepared_post->post_type = $this->post_type; is removed in the patch, I'm removing the commit keyword until code review is finished. once finished, then commit can be applied to alert it's ready to be committed.

#19 @costdev
22 months ago

  • Keywords dev-feedback added

I'm not sure why $prepared_post->post_type = $this->post_type; was removed. There's no context in the original PR and 52422.refresh-of-PR-1333.diff is just a refresh with some improvements, source formatting and explanations.

Whether or not this line is redundant, the change doesn't appear to belong within this ticket's scope.

@Toro_Unit Can you provide any information about why this line was removed?

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


21 months ago

#21 @costdev
21 months ago

Following up on comment 16, $prepared_post->post_type = $this->post_type; doesn't appear to have any effect.

I added the line back in along with some logging before and after it. Results below.

Before: wp_block | After: wp_block
Before: wp_block | After: wp_block
Before: page | After: page
Before: page | After: page
Before: post | After: post
Before: post | After: post
Before: post | After: post

// ...continues with `post | post` for a while.

I believe this is because WP_REST_Posts_Controller::prepare_item_for_database() sets the post type at this location.

Whether this line is redundant is a matter for another ticket, and @rachelbaker might be able to give some insight on this.

Until then, I'll add the line back in and submit a PR to show CI passing.

#23 follow-up: @costdev
21 months ago

CI is passing on the PR. @hellofromTonya, @azaozz, is there anything else you think this needs before we can re-add commit?

#24 in reply to: ↑ 23 @azaozz
21 months ago

Replying to costdev:

Whether this line is redundant is a matter for another ticket...

Right, exactly. The last patch LGTM.

#25 @azaozz
21 months ago

  • Keywords commit added

#26 @Toro_Unit
21 months ago

@costdev Thank you update patch !!!

I can't recall why $prepared_post->post_type = $this->post_type; was removed.

I think it is a mistake. ( Or it could have been removed because it would not affect anything. )

I rolledback tha line and tested it and confirmed that the problem is fixed for both page and custom post type.

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


21 months ago

#28 @chaion07
21 months ago

Thanks @Toro_Unit for reporting this. We reviewed this ticket during a recent bug-scrub session. Based on the feedback received from the team we are confident that Colin's patch can potentially solve the problem.

Props to @Toro_Unit

Cheers!

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


21 months ago

#30 @audrasjb
21 months ago

  • Owner set to audrasjb
  • Status changed from new to accepted

As per today's bug scrub, self assigning for commit.

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


20 months ago

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


20 months ago

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


20 months ago

audrasjb commented on PR #2752:


20 months ago
#34

PHPUnit tests were failing after the last commit, so I restarted them to make sure this PR passes the tests before commit.

costdev commented on PR #2752:


20 months ago
#35

@audrasjb Seems the tests failed again. I rebased on trunk and pushed. Tests passed! 🎉

#36 @peterwilsoncc
19 months ago

  • Keywords commit removed

I added a note to the PR but it didn't come through here.

I think the PR should focus on the logic around when the REST API uses get_sample_permalink() rather than change the logic using wp_unique_post_slug().

This will ensure all the appropriate filters fire too.

#37 @Toro_Unit
19 months ago

@peterwilsoncc

https://github.com/WordPress/wordpress-develop/blob/6.0/src/wp-admin/includes/ajax-actions.php#L2082-L2086

This hack is also used for quick editing.

I think the best way to do this is to modify wp_insert_post and remove this hack.

#38 @peterwilsoncc
19 months ago

  • Keywords commit added

Thanks @Toro_Unit, let's go with what we have then.

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


19 months ago

#40 @audrasjb
19 months ago

  • Resolution set to fixed
  • Status changed from accepted to closed

In 53813:

Posts, Post Types: Force unique slugs for draft posts.

This fixes a behavior where a draft created with the same slug as an existing post would set the existing post to a 404.

wp_unique_post_slug() returns the same slug for 'draft' or 'pending' posts, so to ensure that a unique slug is generated, this changeset adds the post data with the 'publish' status to wp_unique_post_slug().

Props Toro_Unit, h2ham, peterwilsoncc, costdev, antonvlasenko, azaozz, ironprogrammer, audrasjb, hellofromTonya.
Fixes #52422.

#41 @SergeyBiryukov
18 months ago

In 54055:

Tests: Correct the @covers tag in a WP_REST_Posts_Controller test for unique post slugs.

This addresses a notice when generating the code coverage report:

"@covers WP_REST_Request::create_item" is invalid

The WP_REST_Request class does not have a create_item() method, WP_REST_Posts_Controller is the class being tested here.

Includes fixing a typo in the test method name.

Follow-up to [53813].

See #52422, #55652.

Note: See TracTickets for help on using tickets.