Make WordPress Core

Opened 12 years ago

Closed 12 years ago

#22356 closed enhancement (fixed)

Deprecate gd_edit_image_support() and replace it with a better method

Reported by: markoheijnen's profile markoheijnen Owned by: kirasong's profile kirasong
Milestone: 3.5 Priority: normal
Severity: normal Version: 3.5
Component: Media Keywords: has-patch
Focuses: Cc:

Description

Due to the changes of 3.5 we want that WordPress can work without GD and that it should work with only Imagemagick installed for example. That is not the case because of gd_edit_image_suppor() that checks only for GD support

Attachments (14)

22356.diff (6.7 KB) - added by kirasong 12 years ago.
Introduce WP_Image_Editor::supports(); Harden supports_mime_type in Imagick and GD; Check mime while choosing implementation; DOC Fixes. (created in collaboration with markoheijnen)
22356.2.diff (9.0 KB) - added by kirasong 12 years ago.
Same as previous, but also deprecates gd_edit_image_support()
22356.3.diff (10.0 KB) - added by kirasong 12 years ago.
Simplify parameters with $args all around. Still accept string for 'path' on public functions. [edit:proper PHPDoc on supports()]
22356.3.2.diff (10.0 KB) - added by kirasong 12 years ago.
Make mime_type checks actually work, and additional PHPDoc Fix.
22356.4.diff (10.7 KB) - added by scribu 12 years ago.
Separate $path parameter; test() -> exists()
22356.5.diff (30.7 KB) - added by scribu 12 years ago.
WP_Image_Editor_Base
22356.5.tests.diff (1.3 KB) - added by scribu 12 years ago.
22356.5.2.diff (32.2 KB) - added by scribu 12 years ago.
re-order the static methods, to be closer together
22356.6.diff (9.7 KB) - added by kirasong 12 years ago.
22356.3.2.diff​, with get_instance() now accepting a discrete $path argument as previously.
22356.7.diff (32.0 KB) - added by scribu 12 years ago.
optional supports_mime_type()
22356.8.diff (18.2 KB) - added by scribu 12 years ago.
wp_get_image_editor()
22356.8.tests.diff (4.2 KB) - added by scribu 12 years ago.
22356.9.diff (20.0 KB) - added by kirasong 12 years ago.
An incremental pass at turning the factory into functions. Also returns supports_mime_type, and call within _wp_image_editor_choose(). Moves back to scribu's suggestion to only check extension of file from factory function.
22356.9.tests.diff (6.9 KB) - added by kirasong 12 years ago.
Make tests pass with new factory functions

Download all attachments as: .zip

Change History (42)

#1 @nacin
12 years ago

  • Milestone changed from Awaiting Review to 3.5

#2 @nacin
12 years ago

Let's just focus on removing uses of gd_edit_image_support(), specifically in edit_form_image_editor(), which is used on post.php. The usage in get_media_item() is less important, as that code ideally does not fire in 3.5.

#3 @nacin
12 years ago

I think this function highlights three issues with WP_Image_Editor:

  • WP_Image_Editor_GD does not check if, individually, PNG, GIF, and JPEG can be supported. It assumes all can be supported. gd_edit_image_support() does not make that assumption. Is this a safe assumption to make and we were just being hyper-sensitive previously?
  • There doesn't appear to be a good way to check if a mime-type is supported by any of the image editors, which is what gd_edit_image_support() did, without actually loading the image.
  • It looks like supports_mime_type() isn't even considered in test(). I assumed it was.
Last edited 12 years ago by nacin (previous) (diff)

#4 @nacin
12 years ago

  • Owner set to DH-Shredder
  • Status changed from new to assigned

@kirasong
12 years ago

Introduce WP_Image_Editor::supports(); Harden supports_mime_type in Imagick and GD; Check mime while choosing implementation; DOC Fixes. (created in collaboration with markoheijnen)

#5 @kirasong
12 years ago

Attached 22356.diff.

  • Introduce WP_Image_Editor::supports() to test mime-types and required methods without creating an instance of WP_Image_Editor.
  • Harden supports_mime_type in Imagick and GD.
  • Check mime-type (using filename) while choosing implementation.
  • DOC Fixes.

It passes current unit tests, but could use some of its own.

This patch brought to you by DH-Shredder and markoheijnen.

@kirasong
12 years ago

Same as previous, but also deprecates gd_edit_image_support()

#6 follow-up: @ryan
12 years ago

Change supports(), get_instance(), and choose_implementation() to accept one $args associative array? Maybe allow a two arg style for passing key and value? Turn supports into a generic supports/capabilities method like post_type_supports(), current_theme_supports() and wpdb::has_cap()?

Last edited 12 years ago by ryan (previous) (diff)

#7 in reply to: ↑ 6 @kirasong
12 years ago

Replying to ryan:

Change supports(), get_instance(), and choose_implementation() to accept one $args associative array? Maybe allow a two arg style for passing key and value? Turn supports into a generic supports/capabilities method like post_type_supports(), current_theme_supports() and wpdb::has_cap()?

In general I like this, since it would simplify things a bit.

The only hesitation I have is that in the case of get_instance(). Most of the time (probably per the 80/90% rule), a user will want to pass just a filename, since that's its base use-case, and it's a required parameter all the time. Changing the others to either just $args (as "supports cases") would be fine, as for supports() and choose_implementation(), path isn't a required parameter.

#8 @nacin
12 years ago

get_instance() could either accept a string (filename) or an array ($args) as the first argument, or that could be the exception.

As I expressed in IRC yesterday, I hope it can be simplified, as it feels a bit off. Tons of crossover for those three methods.

#9 @kirasong
12 years ago

At the very least, I'll do a simplification run on those functions, and see if anything else can be combined.

Indeed, I don't think this is currently optimal either, it's just complicated if we want to avoid code duplication.

If it's the general consensus that it'd be more straight forward, we can also do a base WP_Image_Editor::supports_mime_type() (rather than supports).

It'd just have the issues mentioned in IRC regarding confusion if an Editor author doesn't override (since it could return true, but for a different Editor), and also would still leave authors without a way to check if there is an Editor that both supports the mime and the methods (without just running get_instance() and checking for failure).

#11 @kirasong
12 years ago

Next step in 22356.3.diff​​.

  • Simplifies parameters by using $args all around.
  • Returns WP_Error if no path is specified on get_instance.
  • Accepts string for 'path' on public functions, so that developers can both easily spawn an image editor and pass the same data to supports() and get_instance(), if they want to do a pre-check.


Last edited 12 years ago by kirasong (previous) (diff)

@kirasong
12 years ago

Simplify parameters with $args all around. Still accept string for 'path' on public functions. [edit:proper PHPDoc on supports()]

@kirasong
12 years ago

Make mime_type checks actually work, and additional PHPDoc Fix.

#12 @ryan
12 years ago

Could supports_mime_type() just be supports() for each implementation. Pass it $args and it can decide what to care about.

#13 @markoheijnen
12 years ago

I rather don't have functionality of WP_Image_Editor merged with an implementation. It whats Mike said in comment:9.
What happens when an implementation doesn't implement supports(). You get weird things then. And now supports_mime_type() by default return false. So an developer really is forced to implement.

Last edited 12 years ago by markoheijnen (previous) (diff)

#14 @scribu
12 years ago

  • Cc scribu added

#15 @scribu
12 years ago

WP_Editor::get_instance() doesn't make sense without a filename, so it should look like this:

function get_instance( $filename, $args = array() )

There's no point whatsoever in mixing the $filename with $args, which can be passed directly to supports().

It might turn out that we just need to make the constructor public, but I'll dig into it some more.

#16 @scribu
12 years ago

I think some of the perceived complexity is due to naming. test(), for example, can mean anything.

Another issue is that there are two kinds of static methods in WP_Image_Editor. Examples:

  • supports(): is there any implementation that does X?
  • test(): each implementation must implement a test() method

I think we should make the first type of static methods stand-alone functions.

Last edited 12 years ago by scribu (previous) (diff)

#17 @scribu
12 years ago

Or have a WP_Image_Editor_Base class, as we did a while ago.

If I was the one that suggested that we merge them, mea culpa.

#18 @nacin
12 years ago

Perhaps wp_http_supports( $capabilities = array(), $url = null ) could be used as a pattern to follow.

@scribu
12 years ago

Separate $path parameter; test() -> exists()

#19 @scribu
12 years ago

  • Keywords has-patch added

22356.4.diff:

  • make get_instance() have a mandatory $path parameter
  • rename test() to exists()

#20 @scribu
12 years ago

Here's what we have:

  • get_instance( $path, $args ): calls supports() and load()
  • supports( $args ): can _any_ implementation handle $args? calls exists() and supports_mime_type()
  • exists(): is X implementation even loaded?
  • supports_mime_type( $mime_type ): can X implementation handle that type?

It's quite DRY.

Last edited 12 years ago by scribu (previous) (diff)

#21 @kirasong
12 years ago

Replying to scribu:

WP_Editor::get_instance() doesn't make sense without a filename, so it should look like this:

function get_instance( $filename, $args = array() )

There's no point whatsoever in mixing the $filename with $args, which can be passed directly to supports().

This is fine, if it's where consensus lies.
I went the $args route all around because it was the simplest to follow, and seemed preferred by most. Since a filename is always required, I have no problem with that.

As far as the test() method, if we're going to rename it, I'd suggest something that indicates what it actually does, which is:
"Is this editor compatible with the current environment." exists() probably isn't quite what we're looking for. Going to think about a better name.

In terms of the base class, I'm a fan of the idea, but unsure if it's a good idea to change that at this juncture. Let's chat about this further.

rboren:
If we are going to consider a supports() that's everywhere, that'd mean that all editors would need to add additional boilerplate code to handle checking of their own methods existence, which seems unnecessary. That's the reason that we didn't add method checks and mime checks to test(). It would also still have the issue that markoheijnen mentions, with the purpose of supports() being unclear when it's unimplemented by an editor.

nacin:
That's fine, if that's what consensus is. We're pretty close to that signature at the moment, anyway, with $args being the equivalent of $capabilities

#22 @nacin
12 years ago

test(), I imagine, was modeled after WP_HTTP. I do think test() is fine as-is.

Isn't supports() basically (bool) get_instance()? In that case, it's basically get_instance() without load(), right?

What's the point of supports_mime_type() if we have supports()? Can't WP_Image_Editor::supports() be one, while an individual implementation's supports() be the other?

@scribu
12 years ago

WP_Image_Editor_Base

#23 @scribu
12 years ago

Both from the comments on this ticket and on IRC, it's apparent that confusion abounds.

22356.5.diff introduces the WP_Image_Editor_Base class. WP_Image_Editor is now a factory, basically.

22356.5.tests.diff contains the changes to the test suite.

@scribu
12 years ago

re-order the static methods, to be closer together

#24 @scribu
12 years ago

I think I'm going to stop at 22356.5.2.diff. Most of that patch is just moving code from A to B, but it does have a point:

It makes it clearer which static methods you're free to call (those in WP_Image_Editor) and which methods are meant for implementations to overwrite (those in WP_Image_Editor_Base).

Dismantling WP_Image_Editor into stand-alone functions would cause even more noise and, like nacin said, we can do later.

@kirasong
12 years ago

22356.3.2.diff​, with get_instance() now accepting a discrete $path argument as previously.

#25 @scribu
12 years ago

We're approaching convergence. 22356.7.diff is almost identical to 22356.6.diff, except for two things:

  • WP_Image_Editor_Base
  • supports_mime_type() is now optional; implementations can perform such a check in their test() methods

PS: The github branch I'm exporting my patches from: https://github.com/scribu/WordPress/compare/image-support;22356

Last edited 12 years ago by scribu (previous) (diff)

@scribu
12 years ago

optional supports_mime_type()

@scribu
12 years ago

wp_get_image_editor()

#26 @scribu
12 years ago

22356.8.diff transforms the factory methods into standalone functions, stored in media.php

Advantages:

  • we can load the base and implementation classes on demand. Yay memory savings!
  • we can keep WP_Image_Editor as the base class name, which saves a lot of churn

@kirasong
12 years ago

An incremental pass at turning the factory into functions. Also returns supports_mime_type, and call within _wp_image_editor_choose(). Moves back to scribu's suggestion to only check extension of file from factory function.

@kirasong
12 years ago

Make tests pass with new factory functions

#27 @kirasong
12 years ago

Added 22356.9.diff​​ and 22356.9.tests.diff​, which are incremental passes at scribu's factory transformation and tests.

  • The return of supports_mime_type(); gets called within _wp_image_editor_choose() (scribu noted he cared less about this than some of the other changes, so went ahead and did so)
  • Unit Tests Now Pass
  • Only check extension of file from factory function, per scribu's previous patch.
  • Replace all instances of WP_Image_Editor:get_instance() with wp_get_image_editor(), both within core and tests.

#28 @nacin
12 years ago

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

In 22817:

WP_Image_Editor: the last stand.

  • Have wp_get_image_editor() rather than WP_Image_Editor::get_instance(). Having static factory methods would be less confusing if there weren't also static methods tied to individual editor implementations.
  • Lazy-load the WP_Image_Editor base class and editor implementations.
  • Have WP_Image_Editor_GD::supports_mime_type() actually check which types it supports.
  • Deprecate gd_edit_image_support() in favor of wp_image_editor_supports().

props DH-Shredder, scribu, markoheijnen. fixes #22356. see #6821.

Note: See TracTickets for help on using tickets.