WordPress.org

Make WordPress Core

Opened 4 years ago

Closed 10 months ago

#21970 closed defect (bug) (fixed)

404 error when a post has the same slug as with a deleted (trash) page.

Reported by: janintia Owned by: SergeyBiryukov
Milestone: 4.4 Priority: normal
Severity: normal Version: 3.4.2
Component: Permalinks Keywords: 4.0-early has-patch commit
Focuses: Cc:

Description

I don't know if this is a bug/issue but I'll share this anyway.

A user creates a new Page, for example, with a title "New Garden". But decides to delete it/"Move to Trash" without deleting it permanently. Then creates a new Post with the same title "New Garden". Viewing that post will result to a "404" page not found.

Attachments (6)

21970.patch (808 bytes) - added by SergeyBiryukov 4 years ago.
21970.diff (812 bytes) - added by kovshenin 4 years ago.
Check the page post status before verifying a match
21970.2.diff (1.6 KB) - added by SergeyBiryukov 4 years ago.
21970.3.diff (1.6 KB) - added by SergeyBiryukov 3 years ago.
21970.tests.diff (1.1 KB) - added by igmoweb 20 months ago.
Unit test
21970.4.diff (1.2 KB) - added by SergeyBiryukov 10 months ago.

Download all attachments as: .zip

Change History (38)

#1 @SergeyBiryukov
4 years ago

  • Severity changed from trivial to normal

Reproduced with /%postname%/ permalink structure.

#2 @SergeyBiryukov
4 years ago

  • Component changed from General to Permalinks
  • Keywords needs-patch added

#3 @SergeyBiryukov
4 years ago

  • Keywords has-patch added; needs-patch removed

Related: #21298

Perhaps get_page_by_path() should not return trashed pages.

#4 @DrewAPicture
4 years ago

Related: #11863 - Trashed items interfere with page/post slug generation

#5 @SergeyBiryukov
4 years ago

#22210 was marked as a duplicate.

@kovshenin
4 years ago

Check the page post status before verifying a match

#6 @kovshenin
4 years ago

In 21970.diff, verifying that the page has a visible post status (public, private or protected). I first thought of checking against the read_post cap, but that might create a really confusing scenario, where a logged in user will see the page, while a logged out user will see the post.

#7 @SergeyBiryukov
4 years ago

  • Keywords needs-unit-tests added

21970.diff looks good to me.

21970.2.diff also handles the similar block in url_to_postid().

#8 @SergeyBiryukov
4 years ago

  • Milestone changed from Awaiting Review to 3.6

#10 @SergeyBiryukov
3 years ago

21970.3.diff makes the condition syntax a bit more obvious.

#11 follow-up: @sc0ttkclark
3 years ago

A similar issue happens with get_page_by_title() (pulls from trash), I may be submitting a patch for that separately as well depending on dev feedback on whether core is open to the adjustment.

#12 @sc0ttkclark
3 years ago

  • Cc lol@… added

#13 in reply to: ↑ 11 @SergeyBiryukov
3 years ago

Replying to sc0ttkclark:

A similar issue happens with get_page_by_title() (pulls from trash)

Related: #20350

#14 @markjaquith
3 years ago

  • Milestone changed from 3.6 to Future Release

Punting this as we're focusing on 3.6 blockers now.

#15 @SergeyBiryukov
3 years ago

#24817 was marked as a duplicate.

#16 @SergeyBiryukov
2 years ago

  • Milestone changed from Future Release to 3.9

#17 @nacin
2 years ago

  • Milestone changed from 3.9 to Future Release

This needs unit tests like whoa.

#18 @samuelsidler
2 years ago

  • Keywords 4.0-early added

#19 @SergeyBiryukov
22 months ago

#30047 was marked as a duplicate.

@igmoweb
20 months ago

Unit test

#20 @igmoweb
20 months ago

Added Unit Tests.

I didn't know where to place this one so I decided for tests/post/objects.php. Now that I read it again sounds weird for me :).

#21 @SergeyBiryukov
12 months ago

#33329 was marked as a duplicate.

#22 @SergeyBiryukov
11 months ago

  • Milestone changed from Future Release to 4.4

#24 @SergeyBiryukov
10 months ago

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

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


10 months ago

#26 @SergeyBiryukov
10 months ago

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

In 35195:

In WP::parse_request() and url_to_postid(), if a post slug clashes with a trashed page, return the post instead of the page.

Props kovshenin, SergeyBiryukov, igmoweb.
Fixes #21970.

#27 @ocean90
10 months ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

[35195] produces a unit test failure:

$ phpunit --group rewrite

1) Tests_Query_VerbosePageRules::test_post_attachment
is_single, is_attachment, is_singular should be true. is_404 should be false.
Failed asserting that false is true.

/srv/www/wp-develop/svn/tests/phpunit/includes/testcase.php:550
/srv/www/wp-develop/svn/tests/phpunit/tests/query/conditionals.php:599

#28 @SergeyBiryukov
10 months ago

[35195] broke pretty permalinks for attachments (#1914), though accidentally fixed #24612 as a side effect.

#29 @SergeyBiryukov
10 months ago

  • Keywords needs-patch added; has-patch removed

#30 @SergeyBiryukov
10 months ago

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

21970.4.diff fixes it for me.

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


10 months ago

#32 @SergeyBiryukov
10 months ago

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

In 35205:

In WP::parse_request() and url_to_postid(), don't skip objects that have a post status with 'exclude_from_search' => false, e.g. inherit.

This fixes pretty permalinks for attachments, broken in [35195].

Fixes #21970.

Note: See TracTickets for help on using tickets.