Twenty Thirteen: Attachment URLs for unattached images cause redirect loop

[trepmal]

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.

[trepmal]

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.

[obenland]

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

[trepmal]

Ah, very nice. Thanks obenland.

[lancewillett]

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?

[obenland]

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.

[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().

[lancewillett]

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.

[nacin]

Yes.

[lancewillett]

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.

[lancewillett]

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.