Make WordPress Core

Opened 7 years ago

Closed 5 years ago

Last modified 4 years ago

#47911 closed enhancement (fixed)

redirect_guess_404_permalink does not consider public custom posts status

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

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 7 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 6 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 6 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 5 years ago.
Unit test updated with @covers tag and minor comment change for consistency.

Download all attachments as: .zip

Change History (29)

#2 @goaroundagain
7 years ago

  • Keywords close added

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

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

  • Keywords has-patch added

#5 @SergeyBiryukov
7 years ago

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

Related: #47574

#6 @davidbaumwald
7 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.


7 years ago

#8 @garrett-eclipse
7 years ago

  • Keywords needs-testing needs-unit-tests added
  • Milestone 5.3 → 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
6 years ago

  • Milestone Future Release → 5.8

@htdat
6 years ago

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

@htdat
6 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
5 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
5 years ago

  • Keywords needs-unit-tests removed

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

#12 @hellofromTonya
5 years ago

  • Milestone 5.8 → 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.


5 years ago

#14 @audrasjb
5 years ago

  • Keywords has-unit-tests added

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


5 years ago

#16 @audrasjb
5 years ago

  • Keywords early added
  • Milestone 5.9 → 6.0
  • Type defect (bug) → 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
5 years ago

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

#17 @peterwilsoncc
5 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.


5 years ago

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


5 years ago

#20 @chaion07
5 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.


5 years ago
#21

  • Keywords needs-refresh removed

#22 @peterwilsoncc
5 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
5 years ago

  • Resolution → fixed
  • Status reviewing → 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
4 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.