WordPress.org

Make WordPress Core

#23719 closed defect (bug) (fixed)

Twenty Thirteen: Attachment URLs for unattached images cause redirect loop

Reported by: trepmal Owned by: lancewillett
Milestone: 3.6 Priority: normal
Severity: major Version: 3.6
Component: Bundled Theme Keywords: has-patch close
Focuses: Cc:

Description (last modified by lancewillett)

In Twenty Thirteen, the attachment url for an unattached image (e.g. http://local.dev/trunk/?attachment_id=5252#main) results in an infinite redirect loop.

See #23543 for original improvements.

Attachment urls for attached images (e.g. http://local.dev/trunk/sample-page/hd-traver-4/#main) are fine.

Commenting out add_filter( 'attachment_link', 'twentythirteen_attachment_link' ); in functions.php is the quick fix.

Attachments (2)

23719.diff (432 bytes) - added by trepmal 16 months ago.
23719.2.diff (970 bytes) - added by trepmal 16 months ago.

Download all attachments as: .zip

Change History (13)

trepmal16 months ago

comment:1 trepmal16 months ago

  • Keywords needs-testing added

Attached patch resolves the issue for me, but I'm not too familiar with all the canonical stuff, so not sure what sort of side-effects there might be.

comment:2 follow-up: obenland16 months ago

  • Milestone changed from Awaiting Review to 3.6

trepmal, do you want to prepare a patch that uses _s' approach?

trepmal16 months ago

comment:3 trepmal16 months ago

Ah, very nice. Thanks obenland.

comment:4 in reply to: ↑ 2 ; follow-up: lancewillett16 months ago

Replying to obenland:

trepmal, do you want to prepare a patch that uses _s' approach?

Why does the attachment have to be an image? If I upload a PDF file, I might still want to link to the parent, right?

comment:5 in reply to: ↑ 4 obenland16 months ago

  • Keywords has-patch added; needs-patch removed

Replying to lancewillett:

Why does the attachment have to be an image? If I upload a PDF file, I might still want to link to the parent, right?

The purpose of twentythirteen_attachment_link() is to reposition the screen on load so the user does not have to scroll down to have a comfortable view of the image. I don't think we need to do that for other attachement types since the vast majority of them will be links to a file.

comment:6 follow-up: nacin16 months ago

I think this is a situation where a theme is trying to be far too clever. I'd maybe let it slide in another theme but not in a default theme that should be setting an example of simple. -100 to twentythirteen_attachment_link().

comment:7 in reply to: ↑ 6 lancewillett16 months ago

Replying to nacin:

I think this is a situation where a theme is trying to be far too clever. I'd maybe let it slide in another theme but not in a default theme that should be setting an example of simple. -100 to twentythirteen_attachment_link().

So you are voting for simpler code in functions.php over a better user experience? I'm leaning your way, but want to be clear on the trade-offs.

comment:8 nacin16 months ago

Yes.

comment:9 lancewillett16 months ago

  • Keywords close added; needs-testing removed

I don't think we need to fix unattached image URLs, since they shouldn't be revealed in a public view.

I agree with Nacin it's a bit too much complexity for this theme, and the tradeoff is a simpler codebase.

comment:10 lancewillett16 months ago

  • Description modified (diff)

comment:11 lancewillett16 months ago

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

In 23677:

Twenty Thirteen: revert addition of twentythirteen_attachment_link() in r23523 -- as discussed in the following ticket comments, the tradeoff means simpler code instead of a minor UX enhancement. Closes #23543 and #23719.

Note: See TracTickets for help on using tickets.