Make WordPress Core

Opened 12 months ago

Closed 12 months ago

Last modified 12 months ago

#59040 closed defect (bug) (wontfix)

Usage of new filter `image_edit_thumbnails_separately` in image editing logic breaks backward compatibility

Reported by: flixos90's profile flixos90 Owned by:
Milestone: Priority: normal
Severity: normal Version: 6.3
Component: Media Keywords:
Focuses: Cc:

Description

Follow-up to #57685 / [55935]: Unfortunately, [55935] is technically a breaking change that for example broke the image editing support of the WebP module in Performance Lab (https://github.com/WordPress/performance/pull/796).

Maybe that's intended since intentionally the UI was disabled, but I personally find it an odd choice to "hide" even the underlying logic behind the new filter, which effectively disables logic that previously would have worked: Passing thumbnail or nothumb as image editing "target" is now pointless, while it would have worked before.

Wouldn't it have been safer to simply hide the UI for this via filter (so that via WordPress UI you wouldn't be able to specify values other than all, but leave the underlying logic untouched?

Since this is a very low-level function, I'm not sure it's worth a fix, but I wanted to flag it in any case. Noting though that the original ticket also has a needs-dev-note keyword, but it seems that a dev note was never published? For this kind of change, I think we should have one due to the above implications.

cc @azaozz @peterwilsoncc @costdev

Change History (2)

#1 @costdev
12 months ago

This was included in the Modified Action/Filter Hooks section of the Field Guide and had the add-to-field-guide keyword, but as it had the needs-dev-note keyword, you're right that it should've had its own dev note / been included in the Misc dev note to draw attention to potential breakages.

I'm not sure (50/50) on whether we should make a change to only hide the UI and maintain the underlying logic. Need to
make sure any reversal doesn't cause as much disruption as the deprecation did, but as 6.3 is only out, that'll be minimal I'd hope.

Ozz, Peter, what do you think about a change here?

#2 @azaozz
12 months ago

  • Milestone Awaiting Review deleted
  • Resolution set to wontfix
  • Status changed from new to closed

Uhh, I wish we could have found this sooner...

Unfortunately, [55935] is technically a breaking change

Not sure what exactly is broken there, but you're right probably could have made it work.
Looking at https://github.com/WordPress/performance/pull/796/files#diff-1aa4f22388bb7b78b82086cd9baaa60f9a240bb72d4b5a4e9d9bb49fa71c5991, seems it is mostly a copy from the core code? That's fine, however such copies would need to be maintained when the core code changes.

Maybe that's intended

Yes both hiding the UI and disabling the specific image editing functionality was intended. The reason is that it produces "not-best-practices" results.

I'm not sure it's worth a fix...

I'm not sure (50/50) on whether we should make a change to only hide the UI...

Not sure either. But seems it can be broken in very specific cases only (copied core code?), and since the intent was to disable the behavior thinking it may be best to leave it as-is for now.

Closing as wontfix. Please feel free to reopen with examples of other places where this change introduces errors.

Last edited 12 months ago by azaozz (previous) (diff)
Note: See TracTickets for help on using tickets.