Make WordPress Core

Opened 11 years ago

Closed 11 years ago

#22538 closed enhancement (fixed)

'wp_image_editor_class' vs. 'wp_image_editors'

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

Description

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

Attachments (3)

22538.diff (634 bytes) - added by scribu 11 years ago.
'wp_image_editor_instance'
22538.2.diff (1.0 KB) - added by scribu 11 years ago.
'image_editors' and 'image_editor_instance'
22538.3.diff (999 bytes) - added by scribu 11 years ago.
'image_editors' only

Download all attachments as: .zip

Change History (46)

#1 @macbrink
11 years ago

#22539 was marked as a duplicate.

#2 @scribu
11 years ago

  • Version set to trunk

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.

#3 @markoheijnen
11 years ago

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

#4 follow-up: @macbrink
11 years ago

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.

#5 in reply to: ↑ 4 @markoheijnen
11 years ago

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.

#6 follow-up: @scribu
11 years ago

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' );

#7 @markoheijnen
11 years ago

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

#8 @scribu
11 years ago

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.

#9 follow-ups: @scribu
11 years ago

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

#10 in reply to: ↑ 6 @macbrink
11 years ago

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.

#11 in reply to: ↑ 9 @macbrink
11 years ago

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?

#12 follow-up: @markoheijnen
11 years ago

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
11 years ago

'wp_image_editor_instance'

#13 in reply to: ↑ 12 @macbrink
11 years ago

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?

Last edited 11 years ago by macbrink (previous) (diff)

#14 follow-up: @markoheijnen
11 years ago

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

#15 @scribu
11 years ago

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

#16 @scribu
11 years ago

  • Keywords has-patch added; dev-feedback removed
  • Type changed from defect (bug) to enhancement

#17 in reply to: ↑ 14 @macbrink
11 years ago

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

#18 @markoheijnen
11 years ago

You have filter priority and array_unshift()

#19 in reply to: ↑ 9 @macbrink
11 years ago

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

#20 @markoheijnen
11 years ago

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

#21 @macbrink
11 years ago

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.

#22 follow-up: @markoheijnen
11 years ago

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

#23 in reply to: ↑ 22 @macbrink
11 years ago

Replying to markoheijnen:

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

Maybe, but when you want to use the WordPress media uploader, save() is not called with the required filename.

Version 0, edited 11 years ago by macbrink (next)

#24 @scribu
11 years ago

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

#25 @scribu
11 years ago

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
11 years ago

'image_editors' and 'image_editor_instance'

#26 @nacin
11 years ago

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.

#27 @scribu
11 years ago

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.

#28 @scribu
11 years ago

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

#29 @macbrink
11 years ago

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

#30 @scribu
11 years ago

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.

#31 @markoheijnen
11 years ago

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.

#32 @scribu
11 years ago

We still need 'image_editor_instance' for unit tests.

#33 @markoheijnen
11 years ago

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.

#34 @scribu
11 years ago

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.

#36 @markoheijnen
11 years ago

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

#37 @scribu
11 years ago

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

#38 @scribu
11 years ago

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.

#39 @scribu
11 years ago

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
11 years ago

'image_editors' only

#40 @scribu
11 years ago

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

#42 @nacin
11 years ago

<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

#43 @nacin
11 years ago

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

In 22859:

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

Note: See TracTickets for help on using tickets.