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 | Owned by: | 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)
Change History (42)
#3
@
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.
@
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
@
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.
#6
follow-up:
↓ 7
@
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()?
#7
in reply to:
↑ 6
@
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
@
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
@
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).
#10
@
12 years ago
For anyone who missed the conversation in IRC:
https://irclogs.wordpress.org/chanlog.php?channel=wordpress-dev&day=2012-11-20&sort=asc#m493613
#11
@
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.
@
12 years ago
Simplify parameters with $args all around. Still accept string for 'path' on public functions. [edit:proper PHPDoc on supports()]
#12
@
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
@
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.
#15
@
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
@
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.
#17
@
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
@
12 years ago
Perhaps wp_http_supports( $capabilities = array(), $url = null )
could be used as a pattern to follow.
#19
@
12 years ago
- Keywords has-patch added
- make get_instance() have a mandatory $path parameter
- rename test() to exists()
#20
@
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.
#21
@
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
@
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?
#23
@
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.
#24
@
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.
@
12 years ago
22356.3.2.diff, with get_instance() now accepting a discrete $path argument as previously.
#25
@
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
#26
@
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
@
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.
#27
@
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.
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.