Media: Redirect slug-based attachment URLs when attachment pages are disabled#11816
Media: Redirect slug-based attachment URLs when attachment pages are disabled#11816dd32 wants to merge 6 commits into
Conversation
redirect_canonical() fails to redirect attachment pages accessed via pretty permalinks (e.g. /my-image-jpg/) when wp_attachment_pages_enabled is 0. get_query_var( 'attachment_id' ) is only populated for ?attachment_id=123 URLs, not slug-based URLs. Falls back to get_queried_object_id() when the query var is empty.
assertCanonical() compares against the path component of the redirect URL, not the full URL. Use parse_url() to extract the path from wp_get_attachment_url().
Cover the privacy guard at canonical.php line ~800: attachments on private posts should not redirect for anonymous users (would leak the file URL), but should redirect for authorized users. Also cover attachments on draft posts and the pages-enabled no-redirect case.
Use create_and_get() directly into self:: properties to avoid intermediate variables that trigger MultipleStatementAlignment warnings.
Child attachment URLs with pretty permalinks take the form /parent-slug/attachment-slug/, not just /attachment-slug/. Fix test URLs to include the parent post slug. Break consecutive self:: assignments with blank lines to avoid PHPCS MultipleStatementAlignment warnings between differently-named properties.
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the Unlinked AccountsThe following contributors have not linked their GitHub and WordPress.org accounts: @claude@local. Contributors, please read how to link your accounts to ensure your work is properly credited in WordPress releases. Core Committers: Use this line as a base for the props when committing in SVN: To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
Test using WordPress PlaygroundThe changes in this pull request can previewed and tested using a WordPress Playground instance. WordPress Playground is an experimental project that creates a full WordPress instance entirely within the browser. Some things to be aware of
For more details about these limitations and more, check out the Limitations page in the WordPress Playground documentation. |
| * populated for ?attachment_id=123 URLs, not slug-based URLs. The fix falls back | ||
| * to get_queried_object_id(). | ||
| */ | ||
| public function test_unattached_slug_url_redirects_when_pages_disabled() { |
There was a problem hiding this comment.
Add @ticket annotation for all the newly added tests
2 participants
When
wp_attachment_pages_enabledis0and pretty permalinks are active, accessing an attachment via its slug URL (e.g./my-image-jpg/) currently returns a 200 with an attachment page rendered, instead of redirecting to the underlying file. The query-string variant (?attachment_id=123) for the same attachment correctly redirects.The cause is in
redirect_canonical()(src/wp-includes/canonical.phparound line 553): the code retrieves the attachment ID viaget_query_var( attachment_id ), which is only populated for?attachment_id=NURLs. For slug-based attachment URLs the query var is empty, soget_post()receives an empty value, returnsnull, and the redirect is silently skipped.This change falls back to
get_queried_object_id()when theattachment_idquery var is empty, so the attachment ID resolves correctly for both URL forms.Trac ticket: https://core.trac.wordpress.org/ticket/65230
Root Cause Analysis
The bug originates from the initial implementation of
wp_attachment_pages_enabledin r56657 (WordPress 6.4, Trac #57913). The redirect logic has always usedget_query_var( attachment_id )to resolve the attachment, which is only populated for?attachment_id=123URLs — never for slug-based pretty permalink URLs like/my-image-jpg/.Trac History
#57913 — The original ticket's intent was unambiguous: "setting [the toggle] to off should disable the attachment URLs entirely, redirecting existing attachment URLs to their parent post." The announcement post described the feature as "attachment pages for new WordPress installations are fully disabled."
#59866 — The follow-up ticket reported that the redirect didn't work for logged-out users (due to a
current_user_cancheck). During testing, @joppuyo explicitly demonstrated in comment:3 that slug-based URLs also don't redirect:They noted:
In comment:5, @afercia acknowledged the slug-based case but expressed uncertainty about whether it was in scope:
@chesio pushed back in comment:6:
The fix that landed in r57357 removed the
current_user_cangate (fixing the logged-out user issue) but kept the sameget_query_var( attachment_id )call, leaving slug-based URLs still broken.This PR
Falls back to
get_queried_object_id()whenget_query_var( attachment_id )is empty. WordPress already resolves the attachment via the slug rewrite rule and sets the queried object — we just weren't reading it.Test Coverage
Expanded the test file to cover the parent-post visibility guard at
canonical.phpline ~800. When an attachment is attached to a post,$redirect_objis set to the parent post, and the later visibility check determines whether to allow or suppress the redirect.test_unattached_slug_url_redirects_when_pages_disabled/unattached-image/test_unattached_query_var_url_redirects_when_pages_disabled?attachment_id=IDtest_attached_to_public_post_slug_url_redirectstest_attached_to_private_post_no_redirect_for_anonymoustest_attached_to_private_post_redirects_for_authorized_usertest_attached_to_draft_post_no_redirect_for_anonymoustest_no_redirect_when_attachment_pages_enabledwp_attachment_pages_enabled = 1The first two tests are the core regression tests for the
get_query_var(attachment_id)fix. The remaining five verify that the existing privacy logic (lines 800-820 ofcanonical.php) works correctly now that$attachment_idis properly resolved for slug-based URLs — particularly that we don't accidentally leak file URLs for attachments on private or draft posts.This Pull Request is for code review only. Please keep all other discussion in the Trac ticket. Do not merge this Pull Request. See GitHub Pull Requests for Code Review in the Core Handbook for more details.