'wp_image_editor_class' vs. 'wp_image_editors'

[scribu]

I don't think there's a clear explanation anywhere for why we have two similar hooks related to the image editor classes.

We have 'wp_image_editors', which passes an array of editor classes.

And then we also have 'wp_image_editor_class', which passes the final class, chosen for a particular set of arguments.

In #6821 it was suggested that we pass $args to 'wp_image_editor_class'.

[macbrink]

#22539 was marked as a duplicate.

[scribu]

Besides the patch for $args, I guess what I'm after is an example of what you can do with one filter that you can't do with the other.

[markoheijnen]

'wp_image_editors' is the primary filter developers should use. 'wp_image_editor_class' was needed till we dropped the caching stuff.

I guess the need for 'wp_image_editor_class' is only there for the unit-tests.

[macbrink]

If $args is passed, you could do with just one.

Imho the class selection could be more elegant. It seems the reasoning behind this was to select between Imagick en GD, however, if a plugin developer wants to add or change functionality, he has to extend both WP_Image_Editor_GD and WP_Image_Editor_Imagick. I would rather have one image editor class which internally checks the image library to use.

[markoheijnen]

Replying to macbrink:

If $args is passed, you could do with just one.

Imho the class selection could be more elegant. It seems the reasoning behind this was to select between Imagick en GD, however, if a plugin developer wants to add or change functionality, he has to extend both WP_Image_Editor_GD and WP_Image_Editor_Imagick. I would rather have one image editor class which internally checks the image library to use.

I disagree with this. You can't mix GD and Imagick. Or at least you shouldn't so the current way of doing is best. Also there are more then two image libraries like jpegtran and gmagick.

[scribu]

Actually, it doesn't make sense to pass $args to either filter, since $args should be checked by the test() method of your image editor.

The 'wp_image_editors' filter is just for registering the class as a candidate:

function my_image_editor_init( $editors ) {

	class My_Image_Editor extends WP_Image_Editor_Imagick {

		static function test( $args = array() ) {

			// this is where $args should be checked

			return parent::test( $args );
		}
	}

	array_unshift( $editors, 'My_Image_Editor' );

	return $editors;
}
add_filter( 'wp_image_editors', 'my_image_editor_init' );

[markoheijnen]

Yes, so the only thing is left for this ticket is if we do need the filter 'wp_image_editor_class'

[scribu]

if a plugin developer wants to add or change functionality, he has to extend both WP_Image_Editor_GD and WP_Image_Editor_Imagick.

You could avoid that duplication by creating a ​decorator that contains the image editor instance chosen by WP.

However, that's not possible with the 'wp_image_editor_class' hook. We'd need something like 'wp_image_editor_instance' instead. Looking into it.

[scribu]

Here's what I had in mind:

class My_Image_Editor_Decorator {

	function __construct( $editor ) {
		$this->editor = $editor;
	}

	// Proxy all methods we're not interested in changing
	function __call( $method, $args ) {
		return call_user_func_array( array( $this->editor, $method ), $args );
	}

	function resize( $max_w, $max_h, $crop ) {

		// Custom functionality goes here

		return $this->editor->resize( $max_w, $max_h, $crop );
	}
}

function my_image_editor_decorate( $instance, $args ) {
	return new My_Image_Editor_Decorator( $instance );
}
add_filter( 'wp_image_editor_instance', 'my_image_editor_init', 10, 2 );

Now to see if we could use 'wp_image_editor_instance' in the unit tests instead of 'wp_image_editor_class'.

[macbrink]

Replying to scribu:

Actually, it doesn't make sense to pass $args to either filter, since $args should be checked by the test() method of your image editor.

The 'wp_image_editors' filter is just for registering the class as a candidate:

so the candidate classes test() should check args, and check for GD or Imagick functionality. This class then should be selected against other classes which could return true for test(), but would not handle the image as required by the plugin.
Because this is a filter, other plugins could add candidate class names too. So without passing $args in the filters (preferably 'wp_image_editor_class'), the plugin has no control over the actual class being selected.

[macbrink]

function my_image_editor_decorate( $instance, $args ) {

return new My_Image_Editor_Decorator( $instance );

}
add_filter( 'wp_image_editor_instance', 'my_image_editor_init', 10, 2 );
}}}

Now to see if we could use 'wp_image_editor_instance' in the unit tests instead of 'wp_image_editor_class'.

Err, so you want a new function with a new filter where you pass the editor instance and $args? Why not pass args to (one of) the existing filters?

[markoheijnen]

If the plugin wants to load an editor itself on a certain place in the code of the plugin then you don't need a filter. The plugin can manipulate 'wp_image_editors'.
The filters don't need args because at that point the factory has the control and not the implementation.

I guess scribu means that the filter will be renamed with a little different way of handeling.

[scribu]

'wp_image_editor_instance'

[macbrink]

The filters don't need args because at that point the factory has the control and not the implementation.

Manipulation of 'wp_image_editors' is done through a filter.
Because it is a filter, other plugins can manipulate 'wp_image_editors' too, how to be sure the right editor is chosen?

[markoheijnen]

The right editor is the first in the array that supports the given mime type and is enabled on the server.

[scribu]

22538.diff​ replaces 'wp_image_editor_class' with 'wp_image_editor_instance'. Still need to fix the unit tests.

[macbrink]

Replying to markoheijnen:

The right editor is the first in the array that supports the given mime type and is enabled on the server.

my argument is that with this filter, there is no way the plugin's editor is first in the array

[markoheijnen]

You have filter priority and array_unshift()

[macbrink]

Replying to scribu:

Here's what I had in mind:

class My_Image_Editor_Decorator {

	function __construct( $editor ) {
		$this->editor = $editor;
	}

This decorator won't work. If you want to override the generate_file_name() method, which is called by _save(), the editor instance will never call the method defined in the decorator class

[markoheijnen]

Why do you want to override generate_file_name(). That doesn't make sense for me.

[macbrink]

It makes sense if you have a plugin that manages images not in wp_upload and that does not store resized images in filename-150x150 format but in subdirectories.

[markoheijnen]

Thats why you can pass parameters to save(). By default it stores images in the same folder as where the image is in.

[scribu]

It turns out that 'wp_image_editor_class' was essential for unit testing only because WP_Image_Editor::__construct() was protected. Since [22817] made it public, implementations can be instantiated directly.

I had no trouble making the unit tests work with just 'wp_image_editor_instance':

​https://github.com/scribu/wp-tests/compare/image-editor-hooks;22538

which makes sense, since the entire point is that 'wp_image_editor_instance' gives you more control over the process than 'wp_image_editor_class'.

[scribu]

Since we're on the subject, what's the point of the 'wp_' prefix?

'wp_editors' was renamed to 'wp_image_editors', but I think 'image_editors' is clear enough.

Similarly, 'image_editor_instance' instead of 'wp_image_editor_instance'.

[scribu]

'image_editors' and 'image_editor_instance'

[nacin]

I'd really like to see what we can do to model this after WP_Http as much as possible, as it has served us well, and it is good to have consistency.

Check it out:

	public static function test( $args = array() ) {
		if ( ! function_exists( 'curl_init' ) || ! function_exists( 'curl_exec' ) )
			return false;

		$is_ssl = isset( $args['ssl'] ) && $args['ssl'];

		if ( $is_ssl ) {
			$curl_version = curl_version();
			if ( ! (CURL_VERSION_SSL & $curl_version['features']) ) // Does this cURL version support SSL requests?
				return false;
		}

		return apply_filters( 'use_curl_transport', true, $args );
	}

Note how SSL is tested, based on $args. This would be equivalent to checking mime types in test(). In our case, the filter outside test() does make more sense.

I actually like our existing filters. I would not tweak much, given the chance. But:

• image_editors should be a simple array of available editors. $args should not be desired here, as it allows us to be able to always figure out all registered editors, regardless of conditionals. Better for forwards compatibility.

• We should probably have a variable filter on the result of test(), which *should* receive $args. Think about the use case of *removing* a single editor from the rotation based on a certain situation. You don't want to force a particular one, you just want to remove it. Also great for unit testing.

• A image_editor_class filter probably makes more sense as the return value of _wp_image_editor_choose(), not in wp_get_image_editor(), so supports() uses that information to know whether an editor is available. Alternatively, there should be a filter on supports().

• image_editor_class should be on the class name — I don't really see the reason for it to be on the object. If anything, we should fire an *action* with the instantiated object to allow triggers to happen then.

[scribu]

image_editor_class should be on the class name — I don't really see the reason for it to be on the object. If anything, we should fire an *action* with the instantiated object to allow triggers to happen then.

An action wouldn't allow to wrap the instance in a decorator. I explained the use case for that earlier.

[scribu]

We should probably have a variable filter on the result of test(), which *should* receive $args. Think about the use case of *removing* a single editor from the rotation based on a certain situation. You don't want to force a particular one, you just want to remove it. Also great for unit testing.

How would that be great for unit testing? And I really can't think of a use case where you would want to prevent an implementation from being used only some times. If you want to remove it completely, you have 'image_editors'.

[macbrink]

What if more than one plugin add image editor classes? Only one of them will be selected in the wp_image_editors filter.

[scribu]

Yes, and it's the site owner's responsibility to install only the plugin that they actually want to use.

Also, with the decorator method I illustrated, you could actually have multiple plugins customizing the various operations at the same time.

[markoheijnen]

Is it an idea to only have the 'wp_image_editors' filter for 3.5 and revisit the needs for 3.6? Specially since we already have our first RC.

[scribu]

We still need 'image_editor_instance' for unit tests.

[markoheijnen]

I don't think we need that anymore. And removing it seems the best way for doing.

If unit test needs it we can remove it before release and added after it. What is ugly but it's the best option.

[scribu]

I don't know. When someone says "let's leave this out of release X; we'll take care of it in release X+1." what usually happens is it doesn't ever get taken care of, or it happens in release X+n, where n > 3.

[wonderboymusic]

Confirmed:

​http://core.trac.wordpress.org/query?status=accepted&status=assigned&status=new&status=reopened&status=reviewing&keywords=~3.2-early&col=id&col=summary&col=status&col=owner&col=type&col=priority&col=milestone&order=priority

[markoheijnen]

Obviously we suck in that but that is not a reason to try to tackle this 9 days before a release.

Also I would love to have 3.6 for only to be cleaning up those tickets. To much feature creeping instead of making WordPress more robust

[scribu]

Fair enough. I'll see what the unit tests look like with only the 'image_editors' filter.

[scribu]

Without either 'image_editor_class' or 'image_editor_instance', the whole tests/image/resize.php file would need to be removed, because it calls image_resize(), which then calls wp_get_image_editor().

And I'm sure there are other examples.

[scribu]

So, given that a 3.6-early tag is almost meaningless, I think removing 'wp_image_editor_class', instead of replacing it with a more powerful hook, is the worst possible solution.

[scribu]

'image_editors' only

[scribu]

I was wrong. Unit tests can do with only 'image_editors'.

So, 22538.diff​ renames 'wp_image_editors' to 'image_editors' and removes 'wp_image_editor_class'.

[scribu]

​[UT1157]

[nacin]

<nacin>  scribu: how strong do we feel about renaming wp_image_editors?
<scribu> 'wp_' is redundant
<scribu> and if we don't remove it now, it's set in stone
<nacin>  We frequently have wp_* starting a filter name when it is the same as
         (or very similar to) the name of the function
<nacin>  It's actually quite common.
<nacin>  I count 120+ filters starting with wp_* alone
<nacin>  And since we're in RC, I'm inclined to leave it.
<scribu> fine

[nacin]

In 22859:

Remove wp_image_editor_class filter. wp_image_editors is sufficient. props scribu. fixes #22538.