WordPress.org

Make WordPress Core

Opened 4 years ago

Closed 4 years ago

Last modified 4 years ago

#33920 closed defect (bug) (fixed)

`wp_old_slug_redirect` ignores rewrite endpoints

Reported by: swissspidy Owned by: pento
Milestone: 4.4 Priority: normal
Severity: normal Version: 2.1
Component: Query Keywords: has-patch
Focuses: Cc:
PR Number:

Description

For the oEmbed feature plugin we're adding an embed rewrite endpoint to posts, to have URLs like http://example.com/my-post/embed/

However, when the slug changes from my-post to new-post, wp_old_slug_redirect redirects to http://example.com/new-post/, ignoring the endpoint.

The function should check for such query vars / endpoints in the process.

Additionally, a filter for the resulting URL in this function would be helpful for developers.

Attachments (6)

33920.diff (1.4 KB) - added by swissspidy 4 years ago.
33920.2.diff (3.9 KB) - added by swissspidy 4 years ago.
33920.3.diff (4.7 KB) - added by swissspidy 4 years ago.
33920.4.diff (4.4 KB) - added by swissspidy 4 years ago.
33920.5.diff (5.0 KB) - added by swissspidy 4 years ago.
33920.6.diff (6.5 KB) - added by swissspidy 4 years ago.

Download all attachments as: .zip

Change History (22)

#1 @swissspidy
4 years ago

  • Keywords has-patch added; needs-patch removed
  • Milestone changed from Awaiting Review to 4.4

I just added a patch that adds missing endpoints to the URL.

@swissspidy
4 years ago

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


4 years ago

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


4 years ago

#4 @pento
4 years ago

  • Keywords needs-unit-tests added

I like it.

While it's kind of hacky, we could add unit tests using the new old_slug_redirect_url filter - hook into the filter, verify that the URL is correct, then return false to short circuit before the wp_redirect().

@swissspidy
4 years ago

#5 @swissspidy
4 years ago

  • Keywords needs-refresh added

The new patch adds unit tests using the method you suggested.

While testing I noticed that WordPress doesn't properly redirect feed URLs, even with the patch applied.

/old-slug/feed/ doesn't redirect to /new-slug/feed/ but instead shows a feed for a non-existent post.

This is because feeds and trackbacks are pretty much hardcoded and not added via add_rewrite_endpoint.

Last edited 4 years ago by swissspidy (previous) (diff)

@swissspidy
4 years ago

#6 @swissspidy
4 years ago

  • Keywords needs-unit-tests needs-refresh removed

New patch also fixes feeds.

#7 @pento
4 years ago

Unit tests are passing when they're run by themselves, but not when they're run as part of the full suite.

3) Tests_Rewrite_OldSlugRedirect::test_old_slug_redirect
is_404 should be true. is_single, is_singular should be false.
Failed asserting that false is true.

/srv/www/wordpress-develop/tests/phpunit/includes/testcase.php:492
/srv/www/wordpress-develop/tests/phpunit/tests/rewrite/oldSlugRedirect.php:62

4) Tests_Rewrite_OldSlugRedirect::test_old_slug_redirect_endpoint
is_404 should be true. is_single, is_singular should be false.
Failed asserting that false is true.

/srv/www/wordpress-develop/tests/phpunit/includes/testcase.php:492
/srv/www/wordpress-develop/tests/phpunit/tests/rewrite/oldSlugRedirect.php:80

5) Tests_Rewrite_OldSlugRedirect::test_old_slug_redirect_feed
Failed asserting that null matches expected 'http://example.org/?p=1087/feed/'.

/srv/www/wordpress-develop/tests/phpunit/tests/rewrite/oldSlugRedirect.php:99

@swissspidy
4 years ago

#8 @swissspidy
4 years ago

Thanks.

Fixed in 33920.4.diff by setting up everything in setUp / tearDown.

@swissspidy
4 years ago

#9 @swissspidy
4 years ago

33920.5.diff now properly respects custom endpoints with a different query var.

#10 @pento
4 years ago

I haven't tested it, what happens with attachment URLs?

Either way, it'd be nice to have a unit test for it.

@swissspidy
4 years ago

#11 @swissspidy
4 years ago

This doesn't change behaviour for attachments, as they already seem to work properly. Perhaps because of redirect_canonical.

In WP_Query name is set to the attachment slug, so wp_old_slug_redirect has no effect.

I added a test for it in 33920.6.diff anyway, and also tested this with paginated posts.

#12 @swissspidy
4 years ago

See also #34043 for implementing old slug redirects for attachments.

#13 @pento
4 years ago

  • Owner set to pento
  • Resolution set to fixed
  • Status changed from new to closed

In 34659:

Rewrite: When redirecting old slugs, include URL endpoints.

Historically, wp_old_slug_redirect() has only ever redirected the old slug of posts, it hasn't included URL endpoints, or worked with comment feed URLs. By adding support for these, we ensure a greater range of URLs aren't killed when the slug changes.

Props swissspdy.

Fixes #33920.

#14 @ocean90
4 years ago

@pento: What about the unit test from 33920.6.diff?

#15 @pento
4 years ago

They all look like they're there to me? (Changeset view doesn't show new files in the diff, only in the file list.)

https://core.trac.wordpress.org/browser/trunk/tests/phpunit/tests/rewrite/oldSlugRedirect.php?rev=34659

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


4 years ago

Note: See TracTickets for help on using tickets.