WordPress.org

Make WordPress Core

Opened 5 years ago

Closed 5 years ago

#28038 closed defect (bug) (fixed)

Only show the Edit Image option in the media modal if applicable

Reported by: Clorith Owned by: wonderboymusic
Milestone: 4.0 Priority: normal
Severity: normal Version: 3.9
Component: Media Keywords: has-patch
Focuses: ui, javascript, administration Cc:

Description

The general media screen filters out the edit option if there's no valid image processing library found. In the media modal this check is never performed so the Edit Image button is available regardless, and thus shows a broken image.

For consistency, I've added a check to not display the edit link if no library is found using wp_image_editor_supports.

I tested with a variety of file types, and couldn't find any other files that would trigger the Edit option in the modal to begin with, so I believe this should be a safe patch.

Attachments (4)

28038.patch (719 bytes) - added by Clorith 5 years ago.
28038.diff (1.1 KB) - added by nacin 5 years ago.
28038.2.diff (1.6 KB) - added by ericlewis 5 years ago.
28038.3.diff (1.6 KB) - added by ericlewis 5 years ago.

Download all attachments as: .zip

Change History (14)

@Clorith
5 years ago

#1 @DrewAPicture
5 years ago

  • Keywords has-patch added
  • Milestone changed from Awaiting Review to 3.9.1
  • Type changed from enhancement to defect (bug)

Since we've made the image editor quite a bit more discoverable via the modal, it seems to me that this bug might be more discoverable as well. Moving to 3.9.1 for consideration.

#2 @samuelsidler
5 years ago

  • Summary changed from Only show the Edit Image option in the media modal if aplicable to Only show the Edit Image option in the media modal if applicable

#3 @nacin
5 years ago

  • Keywords needs-patch added; has-patch removed

This patch will break things... The other nonces are used for any kind of type. It's specifically only the 'edit' nonce that needs to be restricted.

_wp_image_editor_choose() is not exactly cheap, so we should cache it inside that function so we're only running it once per mime type. We should also only run it when we're dealing with an image mime type.

#4 @Clorith
5 years ago

My experience with caching is limited to transients, would that be a sufficient approach in this situation (that would leave a DB query, I don't know how they compare in cost to _wp_image_editor_choose()).

Other than that, moving the media check around a little to account for other media types needing other nonces shouldn't be a problem at all.

@nacin
5 years ago

#5 @nacin
5 years ago

  • Milestone changed from 3.9.1 to 3.9.2

I don't love the static variable, but thinking something like 28038.diff.

@ericlewis
5 years ago

#6 @ericlewis
5 years ago

  • Focuses javascript added
  • Keywords has-patch added; needs-patch removed

Riffing a bit, attachment:28038.2.diff sets a property imageEditorSupports on the settings object, which is a flat array of image mime-types the system can handle. Then compare the attachments mime-type at microtemplate compile-time to decide whether to show the Edit Image link or not.

This ticket was mentioned in IRC in #wordpress-dev by ericandrewlewis. View the logs.


5 years ago

@ericlewis
5 years ago

#8 @ericlewis
5 years ago

attachment:28038.3.diff adds proper instantiation, in case no image editing support is found.

#9 @helen
5 years ago

  • Milestone changed from 3.9.2 to 4.0

#10 @wonderboymusic
5 years ago

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

In 29529:

Media Modal: don't show the "Edit Image" button for attachments that don't have data.sizes set.

Fixes #28038.

Note: See TracTickets for help on using tickets.