Link to media images now get a rel=attachment

[csschris]

Previously only images that link to attachment pages got wrapped in achors with the rel=attachment attribute.

Now images with link to media are also getting wrapped in anchors with rel=attachment.

Is this a bug or is it here to stay? I have lightbox logic that needs to be able to discriminate between links to media and links to attachments. Checking the urls for image extensions isn't ideal.

[SergeyBiryukov]

Seeing some reports about lightbox plugins breakage too.

Appears to be an unintended result of [34259] and [34260], see also [35760].

[jorbin]

@azaozz @joemcgill - You guys seem like you might know the image code enough to dig in. Seems like something we should fix.

[joemcgill]

I believe 36084.diff​ is what was intended in r34259 so that we use the $rel parameter in get_image_send_to_editor() if there is one, and don't add a rel attribute if not.

[joemcgill]

Actually, we probably need to update image_media_send_to_editor() as well, so it becomes responsible for generating the rel, instead of relying on get_image_send_to_editor(). 36084.2.diff​ uses the same logic as wp_ajax_send_attachment_to_editor() for building the $rel attribute.

[SergeyBiryukov]

#35595 was marked as a duplicate.

[joemcgill]

36084.3.diff​ adds unit tests to cover a few different uses of get_image_send_to_editor(). The patch in 36084.2.diff​ fixes the breakage in the third test, Tests_Media::test_get_image_send_to_editor_defaults_no_caption_no_rel().

In my tests, this restores the previous behavior for the rel attributes on links to media. However, I would caution that changes to wp_ajax_send_attachment_to_editor() could reintroduce this issue by changing the $rel value being passed to get_image_send_to_editor(). From what I can tell, the fact that rel attributes were only being applied on links to attachment pages was not intentional (but I don't know the full history), so relying on that attribute for plugin behavior should be considered fragile from my opinion.

[azaozz]

I wouldn't mind reverting [34259] and [34260]. They don't fix any bugs. The tickets they fix are "Fix documentation..." (#32074), and "Move a code block for clarity..." (#32072). However both patches introduce regressions.

There may be some value in keeping the possibility to pass $rel as string in get_image_send_to_editor(), but 36084.3.diff changes the original behaviour too. Keep in mind these are functions specifically for use with the editor and they seem to have all the filters they need so plugins can customize the output.

[joemcgill]

I'm not opposed to reverting, but we should update the docs as well so they're accurate.

36084.3.diff​ is my suggestion if we want the behavior to match the current documentation and keep the previous behavior as it relates to rel attributes.

[azaozz]

How about 36084.4.diff. It reverts most of [34259] and [34260], fix inline docs for $rel being boolean, and tweaks the tests from 36084.3.diff to apply for that.

It also brings back the original behaviour for $rel: insert attachment wp-att-#### then true.

Another place that may need fixing is inside the editor. Currently when an image wrapped in a link is edited, the link's rel attribute is not changed/removed/added.

[azaozz]

Thinking more about this: we've supported a string for $rel in get_image_send_to_editor() for a while. Removing it now may eventually break some plugin...

I'll be pretty easy to continue supporting a string there but not use it in core. New patch coming up :)

[azaozz]

In 36084.5.diff: same as 36084.4.diff but also support string for $rel (see above).

[joemcgill]

Replying to azaozz:

Thinking more about this: we've supported a string for $rel in get_image_send_to_editor() for a while. Removing it now may eventually break some plugin...

The main advantage I see in 36084.5.diff​ over 36084.3.diff​ is the validation that $rel is a string, otherwise, I don't see much benefit over 36084.3.diff​ since functionally, we're building an identical default $rel attribute in wp_ajax_send_attachment_to_editor() before converting to a (bool) when calling get_image_send_to_editor().

If we needed to support a fallback case when someone passes true to the $rel param in get_image_send_to_editor() I suspect we would have heard about it when changed that behavior in [34259].

My preference currently would be to keep the changes introduced last cycle, and do 36084.3.diff​ with an added sanity check to make sure the value is a string. The logic seems more straightforward to me and removes the confusing redundancy that existed before [34259] (et al.).

[azaozz]

Replying to joemcgill:

My preference currently would be to keep the changes introduced last cycle, and do 36084.3.diff​ with an added sanity check to make sure the value is a string. The logic seems more straightforward to me and removes the confusing redundancy that existed before [34259] (et al.).

Yeah, it's similar to 36084.3.diff.

I'm 50/50 on whether we need to support string there. It was added for no good reason (to match the docs?) and is very doubtful anything is using it.

However we should not change how that parameter works, only revert. The bug is that in [34259] the boolean was reversed. However $rel has been a boolean for many years. There is no good reason to change it instead of fix the bug.

[azaozz]

36084.6.diff is same as 36084.5.diff except it doesn't set $rel to a string in wp_ajax_send_attachment_to_editor() before it's needed.

[joemcgill]

Point taken. I'm starting to lean towards reverting as well, except that I suspect #32074 was reported because someone was attempting to pass a different string to it. 36084.7.diff​ is identical to 36084.6.diff​ but moves setting $rel after $url is set to avoid errors.

[azaozz]

In 37035:

Media: fix erroneously inserting a rel attribute in get_image_send_to_editor(). Reverts most of [34259] and [34260] and adds a unit test.

Props joemcgill, azaozz.
Fixes #36084.