Make WordPress Core

Opened 5 years ago

Closed 2 years ago

Last modified 2 years ago

#47911 closed enhancement (fixed)

redirect_guess_404_permalink does not consider public custom posts status

Reported by: goaroundagain's profile goaroundagain Owned by: peterwilsoncc's profile peterwilsoncc
Milestone: 6.0 Priority: normal
Severity: normal Version: 5.2.2
Component: Canonical Keywords: has-patch has-unit-tests early add-to-field-guide
Focuses: Cc:

Description

At the moment the redirect_guess_404_permalink function does only consider the default "publish" post status and there is no way around because "publish" ist hard coded:

$post_id = $wpdb->get_var( "SELECT ID FROM $wpdb->posts WHERE $where AND post_status = 'publish'" );

From register_post_status: https://codex.wordpress.org/Function_Reference/register_post_status
public: (bool) (optional) Whether posts of this status should be shown in the front end of the site.

For this reason, all post with a custom post status set to public will not be found and return an 404.

I think custom post status with public set to true should also be considered.

Attachments (4)

47911.diff (663 bytes) - added by goaroundagain 5 years ago.
Easy fix, just query all public post stati and change the query to IN linke already done with post types in https://core.trac.wordpress.org/browser/tags/5.2.2/src/wp-includes/canonical.php#L673
47911_htdat.diff (1.6 KB) - added by htdat 3 years ago.
Updated the code provided by @goaroundagain and added a unit test for this ticket
47911_htdat_2.diff (1.6 KB) - added by htdat 3 years ago.
The previous diff accidentally removed unnecessary blank link so I updated it again. Basically, this diff is still to update the code provided by @goaroundagain and add a unit test for this ticket
47911_htdat_3.diff (1.6 KB) - added by costdev 2 years ago.
Unit test updated with @covers tag and minor comment change for consistency.

Download all attachments as: .zip

Change History (29)

#2 @goaroundagain
5 years ago

  • Keywords close added

Never mind, just saw that this is fixed with 5.2.2 Great!

#3 @goaroundagain
5 years ago

  • Keywords close removed

Sorry for the confusion, its not fixed. Correct link to the linke in 5.2.2: https://core.trac.wordpress.org/browser/tags/5.2.2/src/wp-includes/canonical.php#L686

@goaroundagain
5 years ago

Easy fix, just query all public post stati and change the query to IN linke already done with post types in https://core.trac.wordpress.org/browser/tags/5.2.2/src/wp-includes/canonical.php#L673

#4 @goaroundagain
5 years ago

  • Keywords has-patch added

#5 @SergeyBiryukov
5 years ago

  • Milestone changed from Awaiting Review to 5.3
  • Owner set to SergeyBiryukov
  • Status changed from new to reviewing

Related: #47574

#6 @davidbaumwald
5 years ago

@SergeyBiryukov Is this one still on your radar for 5.3? If not, can me move it to a future milestone?

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


5 years ago

#8 @garrett-eclipse
5 years ago

  • Keywords needs-testing needs-unit-tests added
  • Milestone changed from 5.3 to Future Release

Moving this to a Future Release as we're now in beta3 for 5.3 and this change will need testing and possible unit tests.

#9 @SergeyBiryukov
3 years ago

  • Milestone changed from Future Release to 5.8

@htdat
3 years ago

Updated the code provided by @goaroundagain and added a unit test for this ticket

@htdat
3 years ago

The previous diff accidentally removed unnecessary blank link so I updated it again. Basically, this diff is still to update the code provided by @goaroundagain and add a unit test for this ticket

#10 @audrasjb
3 years ago

  • Keywords needs-testing removed

I tested the patch and it still applies cleanly against trunk. It looks good on my side: custom post statuses are took into account.

#11 @audrasjb
3 years ago

  • Keywords needs-unit-tests removed

Some unit tests were proposed, so let's remove this keyword.

#12 @hellofromTonya
3 years ago

  • Milestone changed from 5.8 to 5.9

Today is 5.8 Beta 1. As work continues on this ticket, punting to 5.9.

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


2 years ago

#14 @audrasjb
2 years ago

  • Keywords has-unit-tests added

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


2 years ago

#16 @audrasjb
2 years ago

  • Keywords early added
  • Milestone changed from 5.9 to 6.0
  • Type changed from defect (bug) to enhancement

As per today's bug scrub, this enhancement should be considered earlier in the release cycle. Moving to milestone 6.0 with the early workflow keyword.

@costdev
2 years ago

Unit test updated with @covers tag and minor comment change for consistency.

#17 @peterwilsoncc
2 years ago

  • Keywords needs-refresh added

The public attribute when registering post statuses is slightly odd. A post status can be registered as public while not actually been publicly queryable.

<?php
register_post_status(
  'guess404',
  [
    'public' => true,
    'publicly_queryable' => false,
    'internal' => true,
    'protected' => true,
  ]
);

I suggest the following:

<?php
$public_statuses = array_filter( get_post_stati(), 'is_post_status_viewable' );

The IN value will need to run through esc_sql() too. It's mostly a safe bet that the data will be in a form that doesn't need escaping, but for a single function call it's possible to be 100% sure, so let's do that.


This may need a dev note too.

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


2 years ago

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


2 years ago

#20 @chaion07
2 years ago

  • Keywords needs-dev-note added
  • Owner changed from SergeyBiryukov to peterwilsoncc

Thanks @goaroundagain for reporting this. We discussed this ticket during a recent bug-scrub session. Based on the feedback received from the team we're updating the ticket with the changes:

  1. Changing owner from @SergeyBiryukov to @peterwilsoncc since Peter volunteered to push up to a PR. He also wrote the code.
  2. Adding keyword needs-dev-note

Props to @peterwilsoncc

Cheers!

This ticket was mentioned in PR #2470 on WordPress/wordpress-develop by peterwilsoncc.


2 years ago
#21

  • Keywords needs-refresh removed

#22 @peterwilsoncc
2 years ago

In the linked pull request:

  • built upon the previous work in 47911_htdat_3.diff
  • now uses is_post_status_viewable() to determine public post statuses
  • added esc_sql to the SQL IN()
  • extended tests to include custom public, private, internal and protected statuses

Is pre_redirect_guess_404_permalink adequate in terms of filters or should a filter for the post statuses to include be added?

Naming suggestions are welcome if you think the filter is needed.

#23 @peterwilsoncc
2 years ago

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

In 53043:

Canonical: Include all public status in 404 redirects.

In redirect_guess_404_permalink() search for posts using all publicly queryable statuses rather than limiting options to the publish status.

Props goaroundagain, costdev, htdat, audrasjb, chaion07.
Fixes #47911.

#25 @milana_cap
2 years ago

  • Keywords add-to-field-guide added; needs-dev-note removed

Will be added in Field Guide rather than Dev note.

Note: See TracTickets for help on using tickets.