Deprecate gd_edit_image_support() and replace it with a better method

[markoheijnen]

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

[nacin]

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.

[nacin]

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.

[kirasong]

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)

[kirasong]

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]

Same as previous, but also deprecates gd_edit_image_support()

[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()?

[kirasong]

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.

[nacin]

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.

[kirasong]

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

[kirasong]

For anyone who missed the conversation in IRC:
​https://irclogs.wordpress.org/chanlog.php?channel=wordpress-dev&day=2012-11-20&sort=asc#m493613

[kirasong]

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.

[kirasong]

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

[kirasong]

Make mime_type checks actually work, and additional PHPDoc Fix.

[ryan]

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

[markoheijnen]

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.

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

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

[scribu]

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.

[scribu]

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.

[nacin]

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

[scribu]

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

[scribu]

22356.4.diff​:

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

[scribu]

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.

[kirasong]

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

[nacin]

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]

WP_Image_Editor_Base

[scribu]

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]

re-order the static methods, to be closer together

[scribu]

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]

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

[scribu]

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

[scribu]

optional supports_mime_type()

[scribu]

wp_get_image_editor()

[scribu]

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]

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]

Make tests pass with new factory functions

[kirasong]

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.

[nacin]

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.