WordPress.org

Make WordPress Core

Opened 4 years ago

Closed 4 years ago

Last modified 4 years ago

#36084 closed defect (bug) (fixed)

Link to media images now get a rel=attachment

Reported by: csschris Owned by: joemcgill
Milestone: 4.5 Priority: normal
Severity: normal Version: 4.4.2
Component: Media Keywords: has-patch has-unit-tests
Focuses: Cc:

Description

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.

Attachments (7)

36084.diff (613 bytes) - added by joemcgill 4 years ago.
36084.2.diff (1.3 KB) - added by joemcgill 4 years ago.
36084.3.diff (3.9 KB) - added by joemcgill 4 years ago.
36084.4.diff (5.8 KB) - added by azaozz 4 years ago.
36084.5.diff (6.5 KB) - added by azaozz 4 years ago.
36084.6.diff (6.2 KB) - added by azaozz 4 years ago.
36084.7.diff (6.3 KB) - added by joemcgill 4 years ago.

Download all attachments as: .zip

Change History (30)

#1 @SergeyBiryukov
4 years ago

  • Keywords needs-patch added
  • Milestone changed from Awaiting Review to 4.5

Seeing some reports about lightbox plugins breakage too.

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

#2 @jorbin
4 years ago

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

@joemcgill
4 years ago

#3 @joemcgill
4 years ago

  • Keywords has-patch needs-unit-tests added; needs-patch removed
  • Owner set to joemcgill
  • Status changed from new to accepted

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
4 years ago

#4 @joemcgill
4 years ago

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.

#5 @SergeyBiryukov
4 years ago

#35595 was marked as a duplicate.

#6 @SergeyBiryukov
4 years ago

  • Summary changed from Link to media images now get a rel=attachement to Link to media images now get a rel=attachment

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


4 years ago

This ticket was mentioned in Slack in #core-images by joemcgill. View the logs.


4 years ago

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


4 years ago

@joemcgill
4 years ago

#10 @joemcgill
4 years ago

  • Keywords has-unit-tests added; needs-unit-tests removed

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.

This ticket was mentioned in Slack in #core-images by joemcgill. View the logs.


4 years ago

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


4 years ago

#13 @azaozz
4 years ago

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.

#14 @joemcgill
4 years ago

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
4 years ago

#15 @azaozz
4 years ago

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.

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

#16 follow-up: @azaozz
4 years ago

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
4 years ago

#17 @azaozz
4 years ago

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

#18 in reply to: ↑ 16 ; follow-up: @joemcgill
4 years ago

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

#19 in reply to: ↑ 18 @azaozz
4 years ago

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.

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

@azaozz
4 years ago

#20 @azaozz
4 years ago

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
4 years ago

#21 @joemcgill
4 years ago

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.

#22 @azaozz
4 years ago

  • Resolution set to fixed
  • Status changed from accepted to closed

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.

This ticket was mentioned in Slack in #core-images by joemcgill. View the logs.


4 years ago

Note: See TracTickets for help on using tickets.