Per #1664822: Revert image effect and cache system conversions to plugins, we decided to revert image effects and cache as our initial plugin use cases. But we still need a use case, and moshe suggested image toolkits. This is a great use case because:

- There's only 1 implemented in core (GD), so should be not too tedious to convert and minimizes patch bloat.
- There's an existing UI for selecting which toolkit to use.
- The 'available' key is based on non-static info, so this demonstrates a use case for Derivatives.
- It's a use case that includes a configuration setting.
- image_toolkit_invoke() is just silly, replacing it with OOP will be satisfying.

Ideally, I think we should build this use case with chx's AnnotatedClassDiscovery rather than HookDiscovery, because I think merlin's point about keeping everything about a plugin in one place (instead of an info hook plus a class) is an important one to reinforce to people who will be seeing this plugin system for the first time. It also helps explain what derivatives are for (which is a concept I've been finding impossible to explain when info hooks are in use, since info hooks can contain runtime logic).

CommentFileSizeAuthor
#92 90-92-interdiff.txt674 bytesalexpott
#92 1664844.image-toolkits-as-plugins.92.patch87.39 KBalexpott
#90 84-90-interdiff.txt47.79 KBalexpott
#90 1664844.image-toolkits-as-plugins.90.patch87.38 KBalexpott
#85 82-84-interdiff.txt595 bytesalexpott
#85 1664844.image-toolkits-as-plugins.84.patch87.69 KBalexpott
#83 79-82-interdiff.txt3.82 KBalexpott
#83 1664844.image-toolkits-as-plugins.82.patch86.92 KBalexpott
#81 image-toolkit-plugin-1664844-81.patch149.01 KBACF
#79 image-toolkit-plugin-1664844-79.patch85.93 KBACF
#77 image-toolkit-plugin-1664844-77.patch85.89 KBBerdir
#77 image-toolkit-plugin-1664844-77-interdiff.txt4.63 KBBerdir
#75 1664844-74-image_toolkits_as_plugins.patch85.2 KBeojthebrave
#73 1664844-73-image_toolkits_as_plugins.patch85.2 KBeojthebrave
#71 68-71-interdiff.patch34.08 KBalexpott
#71 1664844-image-toolkits-as-plugins-71.patch85.22 KBalexpott
#68 1664844-image-toolkits-as-plugins-68.patch70.39 KBgdd
#66 1664844-image-toolkits-as-plugins-66.patch70.39 KBgdd
#58 1664844_58.drupal8.image-toolkits.patch69.3 KBalexpott
#58 56-58-interdiff.txt2.25 KBalexpott
#56 55-56-interdiff.txt325 bytesalexpott
#56 1664844_56.drupal8.image-toolkits.patch69.3 KBalexpott
#54 50-55-interdiff.txt22.2 KBalexpott
#54 1664844_55.drupal8.image-toolkits.patch69.29 KBalexpott
#50 1664844_50.drupal8.image-toolkits.patch55.31 KBcweagans
#48 1664844_48.drupal8.image-toolkits.patch55.31 KBcweagans
#41 38-41-interdiff.txt13.13 KBalexpott
#41 1664844-41.drupal8.image-toolkits.patch55.36 KBalexpott
#37 interdiff-using-diff.txt3.12 KBalexpott
#37 1664844-38.drupal8.image-toolkits.patch55.15 KBalexpott
#35 32-35-interdiff.txt9.21 KBalexpott
#35 1664844-35.drupal8.image-toolkits.patch55.09 KBalexpott
#32 1664844-32.drupal8.image-toolkits.patch54.64 KBalexpott
#32 29-32-interdiff.txt8.1 KBalexpott
#29 1664844-29.drupal8.image-toolkits.patch54.59 KBalexpott
#29 24-29-interdiff.txt2.73 KBalexpott
#24 22-24-interdiff.txt3.61 KBalexpott
#24 1664844-24.drupal8.image-toolkits.patch54.52 KBalexpott
#22 18-22-interdiff.txt8.37 KBalexpott
#22 1664844-22.drupal8.image-toolkits.patch54.53 KBalexpott
#19 1664844-18.drupal8.image-toolkits.patch54.64 KBalexpott
#17 15-17-interdiff.txt7.01 KBalexpott
#17 1664844-17.drupal8.image-toolkits.patch54.02 KBalexpott
#15 14-15-interdiff.txt55.67 KBalexpott
#15 1664844-15.drupal8.image-toolkits.patch53.97 KBalexpott
#14 13-14-interdiff.patch2.84 KBalexpott
#14 1664844-14.drupal8.image-toolkits.patch48.33 KBalexpott
#13 1664844.drupal8.image-toolkits.patch48.71 KBalexpott
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

EclipseGc’s picture

Project: Drupal 8 Blocks Everywhere » Drupal core
Issue summary: View changes

moshe actually suggested this, not merlin

sun’s picture

Project: Drupal core » Drupal 8 Blocks Everywhere

The only potential "conflict" with this could/might be #1561832: Replace Image toolkit API with Imagine

However, that's still very far from turning into reality anytime soon, so I don't want to hinder progress here. ;)

neclimdul’s picture

Played with this some. Getting things into PSR-0 wasn't terribly hard. I'll push that up somewhere. Apparently image toolkits use a plugin feature I'd been hoping to address in a follow up which is requirements checking. This is actually a common thing so I guess we'll have to deal with it now. Think a module wanting to provide a plugin that's dependent on say comments module being enabled.

I've been thinking about it all morning and I'm hesitant to keep throwing things in decorators because we're already getting to the point where ordering is complicated. If you're having to stack 4 or 5 discovery classes for the common plugin case we're doing something wrong. That said, I don't know where it should live yet. Maybe just a static method on the plugin the type uses for filtering? That or a base discovery that provides some common code for filtering? Best ideas I have at the moment.

effulgentsia’s picture

Maybe just a static method on the plugin the type uses for filtering?

That sounds good to me, at least in the image toolkit case where requirements aren't based on something requiring dependency injection into an object constructor. The question then is what should call this static method? The plugin type's process(Plugin?)Definition() method or something in a reusable discovery class/decorator? I'd be fine with whatever gets us to a solution fastest, and then we can refine in follow-ups after the initial patch is in.

neclimdul’s picture

Assigned: Unassigned » neclimdul

process(Plugin?)Definition() sounds perfect. lets do that. Going to claim this while I'm playing with it today.

effulgentsia’s picture

Assigned: neclimdul » Unassigned

If you're having to stack 4 or 5 discovery classes for the common plugin case we're doing something wrong.

One thing I've been bouncing around a bit in my head is a EventDispatchDecorator to which the plugin type could add (Symfony style) listeners, and these listeners could include the plugin type's processPluginDefinition(), as well as a drupal_alter() step (since we'll want that for chx's AnnotatedClassDiscovery, not just for HookDiscovery), and perhaps filters like requirements checking. Not something for the initial patch, but just putting it out there as a way we can extend the system as we run into use cases like this without needing a new decorator each time (instead, just needing a new listener, which I think is ok).

effulgentsia’s picture

x-post. sorry. I don't have permissions to re-assign to you.

neclimdul’s picture

Assigned: Unassigned » neclimdul

no worries. Sounds like some cool follow ups. Could probably solve a lot of the decorator use cases. In fact we might be able to get rid of all the decorators at some point.

neclimdul’s picture

I'm beginning to think that finishing the Effects conversion or using a different system might be a better choice. Because of the way image_toolkit_invoke() works and interacts with the $image->toolkit property we're either going to have a fairly inefficient system of loading toolkit objects over and over(possible) or we have to do a giant refactor probably converting $image objects from stdClass to a bespoke class along the way so we can do some DI with the toolkit.

I'm happy to keep banging on it though and I'll post a patch that converts it to at least work with the globally set toolkit this afternoon so we can review and maybe someone will see a more elegant middle ground.

sun’s picture

The major image toolkit subsystem refactoring and its consequences on consuming code is basically the problem I mentioned to @effulgentsia already when he updated me due to my question in #1664822: Revert image effect and cache system conversions to plugins -- I'm not sure whether @effulgentsia transferred what I said to anyone else though ;)

I don't know what @Dries said or decided in the conf call about the plugins system you guys said, but meanwhile, I'd rather want to see this land ASAP (without any conversions if that is needed to make everyone happy), so all of the follow-up work gets unblocked, and we move any possibly required follow-up changes to the plugins system into the core queue. That is, because refactoring an entire subsystem like image toolkits will add a considerable amount of noise to the plugins patch/branch/issue, which needs to be carefully reviewed on top of the plugin system code... :-/

neclimdul’s picture

yeah, the complexity is making me leary of this implementation for sure. I don't think I heard the concerns you raised.

The action point from the meeting was this:
"Core patch: Do a complete conversion of a subsystem front to back (image libraries e.g. gd/imagemagick), remove cache and image effects."
This was built from the general feeling of people wanting to see what something in core would look like. We chose image toolkits because on the surface it seemed like a fairly simple thing. Convert to PSR-0 and call methods instead dynamically built functions. All of us on the call fell into that trap. Maybe we keep with this, maybe there's a better option or maybe as you suggested we just go with no implementation. I don't know the correct answer right now.

effulgentsia’s picture

Status: Active » Postponed

Once #1497366: Introduce Plugin System to Core lands in core, this issue can get moved to the core queue.

neclimdul’s picture

Project: Drupal 8 Blocks Everywhere » Drupal core
Version: » 8.x-dev
Component: Plugins » image system
Status: Postponed » Active

moving to core per #11

alexpott’s picture

Status: Active » Needs review
FileSize
48.71 KB

I've done some initial work to move Image Toolkit's into objects - I was looking at this issue from the CMI perspective see #1712258: Simplify image toolkit form logic, removing unneeded checks. - some of the vagaries of the image toolkit system are making that more difficult than it should be too which was I why I started looking at this.

I'm currently looking at how to convert this work to using the new plugin system.

The attached patch loads toolkits using PSR-0 and uses CMI for the configuration. It also makes some changes to the image toolkit selection form it order to rationalise the way in which toolkits add settings forms to it and save their configuration.

alexpott’s picture

Attached patch tidies up the code around system_image_toolkit_settings() and it's submit handler (so that it actually submits for all available toolkits).

alexpott’s picture

Based on what I've seen in the aggregator module I've converted toolkits to the plugin system.

Status: Needs review » Needs work

The last submitted patch, 1664844-15.drupal8.image-toolkits.patch, failed testing.

alexpott’s picture

Status: Needs work » Needs review
FileSize
54.02 KB
7.01 KB

Fix test and some code commentary.

sun’s picture

Title: Convert image toolkit system to plugin based as a demonstration use case » Convert image toolkits into plugins
Issue tags: +API change, +Configuration system, +Plugin system

Clarifying/shortening title and tagging.

alexpott’s picture

Title: Convert image toolkits into plugins » Convert image toolkit system to plugin based as a demonstration use case
Issue tags: -API change, -Configuration system, -Plugin system
FileSize
54.64 KB

Rebased patch on head and converted it to use annotations.

This resulted in two discoveries for me.
1. It's not possible to create a plugin in core/lib/Drupal because drupal_classloader()->getNamespaces() does not include the namespace fallback of 'Drupal' => DRUPAL_ROOT . '/core/lib'.

2. PHPDoc comments cause annotation errors. For example, * @addtogroup image causes the error:

Doctrine\Common\Annotations\AnnotationException: [Semantical Error] The annotation "@addtogroup" in class Drupal\system\Plugin\system\imagetoolkit\GDToolkit was never imported. Did you maybe forget to add a "use" statement for this annotation? in Doctrine\Common\Annotations\AnnotationException::semanticalError() (line 52 of core/vendor/doctrine/common/lib/Doctrine/Common/Annotations/AnnotationException.php).

Interdiff here would be huge and not that useful because lots of file have move due to issue (1).

alexpott’s picture

xpost :(

alexpott’s picture

Title: Convert image toolkit system to plugin based as a demonstration use case » Convert image toolkits into plugins
Issue tags: +API change, +Configuration system, +Plugin system

Doh! There was more... and I got the tags wrong.

alexpott’s picture

Clean up some code documentation and remove an unnecessary function.

aspilicious’s picture

Status: Needs review » Needs work
+use Drupal\system\Plugin\ImageToolkitInterface;
+use \stdClass;

use stdClass with the first backslash. And would be nice te sort the use statements when you're on it.

alexpott’s picture

Status: Needs work » Needs review
FileSize
54.52 KB
3.61 KB

Thanks for the review.

Made suggested corrections from #23

aspilicious’s picture

image_rotate and others are using a stdClass as param. Better to typehint with "ImageToolkitInterface ".

alexpott’s picture

I think that's because $image is an object that contains a toolkit object but it itself is not a toolkit.

+++ b/core/includes/image.incundefined
@@ -134,7 +124,7 @@ function image_get_info($filepath, $toolkit = FALSE) {
     $image = new stdClass();
     $image->source = $filepath;
     $image->toolkit = $toolkit;

I think that creating a PSR-0 class for images that can be manipulated by toolkits is possibly a separate issue.

sun’s picture

Status: Needs review » Needs work
Issue tags: +API change, +Configuration system, +Plugin system

The last submitted patch, 1664844-24.drupal8.image-toolkits.patch, failed testing.

alexpott’s picture

Re-rolled patch against latest HEAD.

Fixed comment in code.

Interdiff seems to suggest that the patch is re-implementing system_image_toolkits() - it's not - interdiffs are always a little weird after a reroll :) but its useful as it shows the two other changes... the system_update_N number and the code comment fix.

alexpott’s picture

Status: Needs work » Needs review
sun’s picture

Great stuff. Overall, this looks really good! :)

+++ b/core/modules/system/lib/Drupal/system/Plugin/ImageToolkitInterface.php
@@ -0,0 +1,156 @@
+interface ImageToolkitInterface {
...
+  function resize(stdClass $image, $width, $height);
...
+  function rotate(stdClass $image, $degrees, $background = NULL);
...
+  function crop(stdClass $image, $x, $y, $width, $height);
...
+  function desaturate(stdClass $image);

I'm not 100% sure, but I think there's an architectural flaw with the idea of putting the operation methods onto the toolkit class itself:

Image toolkits can provide many more methods -- have a look at the imagemagick_advanced sub-module of ImageMagick...

Of course, the toolkit can happily define additional methods that are not in the interface, but then again, are the methods on the interface actually required for a toolkit? I doubt it. E.g., a toolkit that doesn't support e.g. desaturate() is still a valid toolkit, right?

I'd like to quickly move forward here, so I'm happy to punt on this for now -- especially since the idea of switching entirely to Imagine isn't really ruled out yet (just a lack of someone doing it, I guess).

+++ b/core/modules/system/lib/Drupal/system/Plugin/system/imagetoolkit/GDToolkit.php
@@ -0,0 +1,301 @@
+class GDToolkit implements ImageToolkitInterface {
...
+  function settingsForm() {
+    if ($this->checkAvailable()) {
...
+    else {
+      form_set_error('image_toolkit', t('The GD image toolkit requires that the GD module for PHP be installed and configured properly. For more information see <a href="@url">PHP\'s image documentation</a>.', array('@url' => 'http://php.net/image')));
+      return FALSE;
+    }

AFAICS, the settings form for all toolkits is triggered on the image toolkit settings form, so in case my server has ImageMagick installed but no GD library compiled in, I actually have a working environment, but the toolkit settings form will show me an error and state that the GD image toolkit requires GD to be installed...?

In other words, I think that the availability check needs to be moved out of the settings form and be performed in some other location (probably the system image toolkit settings form or somewhere else).

+++ b/core/modules/system/lib/Drupal/system/Plugin/system/imagetoolkit/GDToolkit.php
@@ -0,0 +1,301 @@
+  public static function checkAvailable() {

Let's rename this to isAvailable()?

alexpott’s picture

Renamed checkAvailable() to isAvailable()

@sun you're right about the availability check in settingsForm - in fact it's never called. So I've switched things round so that if system_image_toolkit_settings() doesn't find any available toolkits it outputs a helpful form error.

EclipseGc’s picture

I could totally see image operations actually being plugins of their own... this leads to weird stuff pretty quickly though since we allow for configuration based upon essentially these in image styles, and that you'd need to swap which ones are being used based upon which image toolkit you are using. That seems weird, probably an indicator that image operations aren't actually plugins, but rather just consistently named classes that are some how associated with the toolkit. I don't actually know, just throwing spaghetti on the wall to see what sticks. If everyone is happy with what's happening in this patch, feel free to ignore me :-)

Thanks Alex for doing this awesome work. It definitely needed doing.

Eclipse

yched’s picture

Status: Needs review » Needs work
+++ b/core/includes/image.incundefined
@@ -39,15 +41,16 @@
+function image_get_available_toolkits($key = NULL) {
+  // Use plugin system to get list of available toolkits.
+  $manager = new ImageToolkitManager();

The patch re-instanciates a new Manager object on each call (here and in image_get_toolkit()). The current preferred way would be to use a functional image_get_toolkit_manager() factory that creates one instance and caches it in drupal_static().
The "future" way would be to register the Manager class directly in the DIC by implementing a Bundle, but the test suite is not completely up for that yet.

+++ b/core/modules/image/image.effects.incundefined
@@ -79,7 +79,7 @@ function image_image_effect_info() {
+    watchdog('image', 'Image resize failed using the %toolkit toolkit on %path (%mimetype, %dimensions)', array('%toolkit' => get_class($image->toolkit), '%path' => $image->source, '%mimetype' => $image->info['mime_type'], '%dimensions' => $image->info['width'] . 'x' . $image->info['height']), WATCHDOG_ERROR);

If toolkit classes extended PluginBase, you could print the machine name instead of the class with $image->getPluginId();
Not critical, but given that the configuration is made by specifying the id of an image toolkit in a config file somewhere, the plugin id is more relevant than the class name in error messages. That's probably a common case across Plugins uses : refer to plugin name rather than class name in UI / error messages.

Also, not a blocker for an initial patch to get in, but I'm not sure the current pattern of each toolkit plugin class being responsible for writing / reading its settings in the config system is the best approach. For instance, the code in GDToolkit::settingsFormSubmit() relies on $form_state['values']['gd'], which is tightly coupled to the the shape of the UI form defined outside the class.
I'd like to promote a standardized approach for "plugin settings" where settings are injected in the plugin object at construct time (and thus saving / reading settings is handled outside the plugin class) - see #1764380: Merge PluginSettingsInterface into ConfigurableInterface. Again, not a blocker, but the case of image toolkits is a typical and fairly straightforward example, and I'm a bit worried to see each and every "X as plugin" effort reinventing its own specific ways of handling plugin settings.

23 days to next Drupal core point release.

alexpott’s picture

Status: Needs work » Needs review
FileSize
55.09 KB
9.21 KB

Created new image_get_toolkit_manager() function to create manager object only once per request.

Image toolkits now extend pluginbase and use getPluginId() in the error messages.

Thnaks for the review @yched.

I totally agree with your points about decoupling the UI form from the class - makes loads of sense.

aspilicious’s picture

Status: Needs review » Needs work

Some small doc cleanups I would like to get in with this patch.

+++ b/core/modules/system/lib/Drupal/system/Plugin/ImageToolkitInterface.phpundefined
@@ -0,0 +1,156 @@
+namespace Drupal\system\Plugin;
+use stdClass;

Newline needed between namespace and use lines

+++ b/core/modules/system/lib/Drupal/system/Plugin/ImageToolkitInterface.phpundefined
@@ -0,0 +1,156 @@
+   * Retrieve toolkit's settings form.

Retrieves.

In general each function in this interface should start with a 3th person verb

+++ b/core/modules/system/lib/Drupal/system/Plugin/ImageToolkitInterface.phpundefined
@@ -0,0 +1,156 @@
+   * Form submission handler for toolkit's settings form.

Should start with a 3th person verb

+++ b/core/modules/system/lib/Drupal/system/Plugin/ImageToolkitManager.phpundefined
@@ -0,0 +1,24 @@
+  public function __construct() {

"Constructs a ... object" docblock needed. See node 1354 for more info.

+++ b/core/modules/system/lib/Drupal/system/Plugin/system/imagetoolkit/GDToolkit.phpundefined
@@ -0,0 +1,296 @@
+   * Create a truecolor image preserving transparency from a provided image.

Creates

+++ b/core/modules/system/lib/Drupal/system/Plugin/system/imagetoolkit/GDToolkit.phpundefined
@@ -0,0 +1,296 @@
+   *   The new height of the new image, in pixels.
+   * @return
+   *   A GD image handle.

space between @param and @return blocks

+++ b/core/modules/system/system.admin.incundefined
@@ -1871,39 +1872,68 @@ function system_file_system_settings() {
+  // If we have avialable toolkits allow the user to select the image toolkit to

avialable

+++ b/core/modules/system/tests/modules/image_test/lib/Drupal/image_test/Plugin/system/imagetoolkit/BrokenToolkit.phpundefined
@@ -0,0 +1,29 @@
+  /**

Newline needed between class declaration and first function

+++ b/core/modules/system/tests/modules/image_test/lib/Drupal/image_test/Plugin/system/imagetoolkit/TestToolkit.phpundefined
@@ -0,0 +1,122 @@
+   * Store the values passed to a toolkit call.

Stores

alexpott’s picture

Thanks @aspilicious.

Rerolled patch as it no longer applied and made suggested changes.

Unfortunately due to the re-roll interdiff was unhappy so I've provided a diff of the patches using diff... horrible but at least you can see what's happening.

sun’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 1664844-38.drupal8.image-toolkits.patch, failed testing.

sun’s picture

We're getting there! :)

+++ b/core/includes/image.inc
@@ -39,15 +41,16 @@
+function image_get_available_toolkits($key = NULL) {
...
+  $manager = image_get_toolkit_manager();
+  $toolkits = $manager->getDefinitions();

@@ -55,49 +58,49 @@ function image_get_available_toolkits() {
+function image_get_toolkit_manager() {
+  $manager = &drupal_static(__FUNCTION__);
...
+    $manager = new ImageToolkitManager();
...
+function image_get_toolkit($toolkit_id = NULL) {
+  static $toolkit = array();
...
     $toolkits = image_get_available_toolkits();
...
+      $manager = image_get_toolkit_manager();
+      $toolkit[$toolkit_id] = $manager->createInstance($toolkit_id);

I'm a bit confused by the various procedural helper functions.

For one, ImageToolkitManager appears to be hard-coded currently, so the helper function image_get_toolkit_manager() doesn't really make sense, because the manager class could simply retain what it sets up in static properties already. However, any kind of statics are not good, so I wonder whether the ImageToolkitManager shouldn't actually be a service on the DIC; i.e.,

drupal_container()->get('image.toolkit.manager');

Second, the phpDoc of image_get_toolkit() & co should be updated accordingly. Let's also replace the static with a drupal_static(), please - otherwise, the instantiated toolkits will leak into tests :)

+++ b/core/includes/image.inc
@@ -55,49 +58,49 @@ function image_get_available_toolkits() {
+  if (!is_object($manager)) {
...
+  if (empty($toolkit_id)) {

Let's use !isset() here.

+++ b/core/modules/system/lib/Drupal/system/Plugin/ImageToolkitInterface.php
@@ -0,0 +1,157 @@
+  function settingsForm();
...
+  function resize(stdClass $image, $width, $height);
...
+  function load(stdClass $image);
...
+  function save(stdClass $image, $destination);

+++ b/core/modules/system/lib/Drupal/system/Plugin/system/imagetoolkit/GDToolkit.php
@@ -0,0 +1,297 @@
+  function settingsFormSubmit($form, &$form_state) {
...
+  function resize(stdClass $image, $width, $height) {
...
+  function rotate(stdClass $image, $degrees, $background = NULL) {

We're generally missing method visibility declarations; all of these seem to be public.

+++ b/core/modules/system/lib/Drupal/system/Plugin/system/imagetoolkit/GDToolkit.php
@@ -0,0 +1,297 @@
+ * @Plugin(
+ *   id = "gd",
+ *   title = @Translation("GD2 image manipulation toolkit"),
+ *   description = @Translation("GD2 image manipulation toolkit.")
+ * )
+ */
+class GDToolkit extends PluginBase implements ImageToolkitInterface {

Does the Plugin system require a description for all plugins? If not, let's remove it here. ;)

+++ b/core/modules/system/lib/Drupal/system/Plugin/system/imagetoolkit/GDToolkit.php
@@ -0,0 +1,297 @@
+  function settingsForm() {
+    $form['status'] = array(
+      '#markup' => t('The GD toolkit is installed and working properly.'),
+    );

Hmmmm... I don't think this status message belongs into the settings form ;)

Let's either move it into the image toolkit settings form (but there's no method yet to determine whether a toolkit is fully operational — which would be a very good feature addition to the image toolkit system, since toolkits like ImageMagick require additional checks, whereas isAvailable() only tests whether it is basically available, but that's for another issue...) — so... or drop it completely. ;)

+++ b/core/modules/system/tests/modules/image_test/lib/Drupal/image_test/Plugin/system/imagetoolkit/TestToolkit.php
@@ -0,0 +1,122 @@
+  function _logCall($op, $args) {

Let's make that:

protected function logCall(...)
alexpott’s picture

Status: Needs work » Needs review
FileSize
55.36 KB
13.13 KB

Agree with and applied feedback from #40...

alexpott’s picture

Agree with and applied feedback from #40...

aspilicious’s picture

Status: Needs review » Needs work
+++ b/core/includes/bootstrap.incundefined
@@ -2499,6 +2499,10 @@ function drupal_container(Container $new_container = NULL, $rebuild = FALSE) {
+
+    // Register image toolkit manager.
+    $container
+      ->register('image.toolkit.manager', 'Drupal\system\Plugin\ImageToolkitManager');

This is not needed in low level bootstrap. We should create an ImageBundle.php file and add the register the toolkit manager there.

This can possibly blow up the tests. Will put a link to the critical issue in a few seconds.

aspilicious’s picture

sun’s picture

Status: Needs work » Needs review

The image toolkit system works perfectly fine in low-level bootstrap today, so requiring a higher bootstrap level would be a regression. Therefore, the service registration on the container looks perfectly correct and fine to me.

aspilicious’s picture

Whoops I'm srry I thought these were added by image module. My fault :)

alexpott’s picture

Actually aspilicious is asking a question I have pondered several times whilst writing this patch. Why is all this image stuff in the system module? I think this patch can proceed as is and would be keen for someone to pull the rrbc hammer as I've rerolled a few times. And then I'll explore moving this all to the image module.

cweagans’s picture

I rerolled the patch (the bootstrap.inc hunk was no longer applying).

I didn't see anything that jumped out as an immediate problem, everything is working as expected, and sun has already reviewed this patch thoroughly and it looks like everything got cleaned up. Given that, if this patch comes back green, I'd like to call this RTBC, especially since it's blocking work elsewhere.

Status: Needs review » Needs work

The last submitted patch, 1664844_48.drupal8.image-toolkits.patch, failed testing.

cweagans’s picture

Status: Needs work » Needs review
FileSize
55.31 KB

Er, for real this time. ban.module added a new update to system.install.

cweagans’s picture

Status: Needs review » Reviewed & tested by the community

Cool. I say RTBC.

Lars Toomre’s picture

In light of #1791872: [Policy, no patch] Add special tag to identify issues ready for a high level only review, I am unsure when and whether one should do a detailed (some might say pedantry) review of this patch. Should it be at this point?

Reading through this patch at a high level, I noticed that image_get_available_toolkits($key = NULL) and image_get_toolkit($toolkit_id = NULL) both added variables to their function profiles. Yet neither docblock was changed. Also noted that there was no type hinting on either of those @return directives. At minimum, the missing @param directives need to be added.

Further down in the patch, there are many instances of type hinting missing from both @param and @return directives. There also is small stuff, like missing blank lines between @param and @return directives. My understanding is that type hinting should be added in all D8 additions/modifications.

I suspect that there are other changes that should be noted if one did a detailed review from a documentation and coding standards perspective. However, @cweagans said in #48 this patch is "blocking work elsewhere" so I am not reviewing this patch in any greater detail.

cweagans’s picture

Lars, if you want to help, why don't you reroll the patch with the changes that you want made?

alexpott’s picture

This patch adds type hinting on all the changed files and fixes the doc blocks mentioned in #52.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 1664844_55.drupal8.image-toolkits.patch, failed testing.

alexpott’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
69.3 KB
325 bytes

Doh... missing type hint

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 1664844_56.drupal8.image-toolkits.patch, failed testing.

alexpott’s picture

Reroll to bump system_update_8021() to system_update_8022().

Interdiff is a bit confusing... but it tends to be with rerolls :(

alexpott’s picture

Status: Needs work » Reviewed & tested by the community

Bumping back to RTBC since changes are docs and bumping hook_update_N number.

sun’s picture

Thanks! This still looks good to me.

cweagans’s picture

Still looks good to me too.

catch’s picture

Status: Reviewed & tested by the community » Needs review

Per #1614186: Coding standards for using "native" PHP classes like stdClass in namespaced code we're not supposed to use stdClass any more. We could fix that in a follow-up since lots of existing core code is broken.

This makes me a bit uncomfortable:

+
+    // Register image toolkit manager.
+    $container
+      ->register('image.toolkit.manager', 'Drupal\system\Plugin\ImageToolkitManager');
   }

We know that image toolkits are used once in a blue moon, so registering them as a service in the DIC feels like overkill, but if we want to dependency inject everything does that mean DIC-ing all the things?

+++ b/core/includes/image.incundefined
@@ -55,49 +57,38 @@ function image_get_available_toolkits() {
+function image_get_toolkit($toolkit_id = NULL) {
+  $toolkit = &drupal_static(__FUNCTION__, array());
+
+  if (!isset($toolkit_id)) {
+    $toolkit_id = config('system.image')->get('toolkit');
+  }
 
-  if (!isset($toolkit)) {
+  if (!isset($toolkit[$toolkit_id])) {
     $toolkits = image_get_available_toolkits();
-    $toolkit = variable_get('image_toolkit', 'gd');
-    if (!isset($toolkits[$toolkit]) || !function_exists('image_' . $toolkit . '_load')) {
+
+    if (!isset($toolkits[$toolkit_id]) || !class_exists($toolkits[$toolkit_id]['class'])) {
       // The selected toolkit isn't available so return the first one found. If
       // none are available this will return FALSE.
       reset($toolkits);
-      $toolkit = key($toolkits);
+      $toolkit_id = key($toolkits);
     }
-  }
-
-  return $toolkit;
-}
 
-/**
- * Invokes the given method using the currently selected toolkit.
- *
- * @param $method
- *   A string containing the method to invoke.
- * @param $image
- *   An image object returned by image_load().
- * @param $params
- *   An optional array of parameters to pass to the toolkit method.
- *
- * @return
- *   Mixed values (typically Boolean indicating successful operation).
- */
-function image_toolkit_invoke($method, stdClass $image, array $params = array()) {
-  $function = 'image_' . $image->toolkit . '_' . $method;
-  if (function_exists($function)) {
-    array_unshift($params, $image);
-    return call_user_func_array($function, $params);
+    if ($toolkit_id) {
+      $toolkit[$toolkit_id] = drupal_container()->get('image.toolkit.manager')->createInstance($toolkit_id);
+    }
   }
-  watchdog('image', 'The selected image handling toolkit %toolkit can not correctly process %function.', array('%toolkit' => $image->toolkit, '%function' => $function), WATCHDOG_ERROR);
-  return FALSE;
+
+  return $toolkit[$toolkit_id];

I'm a bit confused by this. We're static caching something that comes from the container. That breaks the container as replacing static caches in terms of services (if I swap out the toolkit in the container it won't be reflected in image_get_toolkit()).

Since the container doesn't actually contain a way to get the toolkit, only the factory itself, again this makes me wonder why we're registering this in there at all - why would I ever want to dependency inject the toolkit factory as opposed to an actual toolkit?

+++ b/core/modules/system/lib/Drupal/system/Plugin/system/imagetoolkit/GDToolkit.phpundefined
@@ -0,0 +1,292 @@
+    // PHP installations using non-bundled GD do not have imagefilter.
+    if (!function_exists('imagefilter')) {
+      watchdog('image', 'The image %file could not be desaturated because the imagefilter() function is not available in this PHP installation.', array('%file' => $image->source));

We're going to need to refactor logging so it can be injected at some point, not here though.

I'm only half way through reviewing but have run out of time. Moving back to CNR rather than CNW since I'm not sure all these complaints are 100% valid this evening.

jibran’s picture

Status: Needs review » Needs work
Issue tags: +API change, +Configuration system, +Plugin system

The last submitted patch, 1664844_58.drupal8.image-toolkits.patch, failed testing.

jibran’s picture

I tried to re-roll but it has a conflict with #1849004: One Service Container To Rule Them All unable to figure out the conflict.

gdd’s picture

Status: Needs work » Needs review
FileSize
70.39 KB

This is just a straight reroll to try and get this moving again. Also wanted to bring up that there seems to be a lot of overlap between the work being done here and #1821854: Convert image effects into plugins. Might make sense to merge these issues?

Status: Needs review » Needs work

The last submitted patch, 1664844-image-toolkits-as-plugins-66.patch, failed testing.

gdd’s picture

oh system_update_n(). Going from _22 to _49, that's how long this patch has been sitting around!

gdd’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 1664844-image-toolkits-as-plugins-68.patch, failed testing.

alexpott’s picture

The patch attached

  • In response to #63 the DIC now has the image.toolkit service to provide the default toolkit
  • The functions image_get_toolkit and image_get_available toolkits are now part of ImageToolKitManager.
  • should fix the tests
  • Removes some code that this patch makes redundant
  • moved a couple of test functions to the ToolkitTestBase

We are registering the image toolkit manager and the image toolkit on the DIC - this is consistent with other object factories eg language manager / queue / entity... there is a lot more on the DIC now than when this patch was first proposed and it is compiled to disk.

As for the use of stdClass() for managing the image objects - that is not introduced by this patch and I think should be addressed in a followup.

alexpott’s picture

Status: Needs work » Needs review
eojthebrave’s picture

Re-roll so that it applies cleanly. I don't know a whole lot about the plugin manager side of things but this looks reasonable to me. :)

Status: Needs review » Needs work

The last submitted patch, 1664844-73-image_toolkits_as_plugins.patch, failed testing.

eojthebrave’s picture

Status: Needs work » Needs review
FileSize
85.2 KB

Hmm. Incremented system_update_8049 to system_update_8050, should fix that failure. Go testbot go!

Status: Needs review » Needs work
Issue tags: -Plugin system

The last submitted patch, 1664844-74-image_toolkits_as_plugins.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
FileSize
4.63 KB
85.89 KB

Rerolled, had a look at the test failures. Let's see how this goes.

Status: Needs review » Needs work

The last submitted patch, image-toolkit-plugin-1664844-77.patch, failed testing.

ACF’s picture

Status: Needs work » Needs review
FileSize
85.93 KB

See if can reroll and get the patch to apply.

Status: Needs review » Needs work

The last submitted patch, image-toolkit-plugin-1664844-79.patch, failed testing.

ACF’s picture

Status: Needs work » Needs review
FileSize
149.01 KB

Another try.

Status: Needs review » Needs work

The last submitted patch, image-toolkit-plugin-1664844-81.patch, failed testing.

alexpott’s picture

Status: Needs work » Needs review
Issue tags: +SprintWeekend2013
FileSize
86.92 KB
3.82 KB

Hmmm reroll shouldn't make patch jump from 85kb to 149kb... some extra work must have slipped in.

The patch attached fixes the ToolkitGdTest::checkRequirements() - we can't use the container here as it's not really set up for the test environment at this point. Whilst doing this took the opportunity to convert that test to DrupalUnitTestBase.

aspilicious’s picture

An incorrect commit happened in #1937140: Convert image_jpeg_quality to cmi. Can we merge efforts? In short I think we should revert the update function change from that issue and remove the .yml file.

alexpott’s picture

Okey-dokey...

Removed the changes introduced by #1937140: Convert image_jpeg_quality to cmi

There should be follows up to this issue to use a form_controller_class, list_controller_class and to look at removing the use of stdClass() to pass around image objects.

Berdir’s picture

+++ b/core/includes/image.incundefined
@@ -121,20 +55,20 @@ function image_toolkit_invoke($method, stdClass $image, array $params = array())
+  if ($toolkit === NULL) {
+    $toolkit = drupal_container()->get('image.toolkit');

As of yesterday or so, the preferred way to access services from the container is Drupal::service('image.toolkit') unless it's important enough to get it's own Drupal::something() method. Not sure if this is... Maybe if it were then this function could be part of the plugin or the manager? (not in this issue, of course)

+++ b/core/modules/file/lib/Drupal/file/Tests/ValidatorTest.phpundefined
@@ -79,7 +79,7 @@ function testFileValidateImageResolution() {
-    if (image_get_toolkit()) {
+    if (drupal_container()->get('image.toolkit')) {

Hm, given that it replaces an existing API function and is accessed quite a bit, I think it might make sense to add a Drupal::method() for it.

+++ b/core/modules/image/image.effects.incundefined
@@ -64,22 +64,22 @@ function image_image_effect_info() {
-function image_resize_effect($image, $data) {
+function image_resize_effect(stdClass &$image, array $data) {
   if (!image_resize($image, $data['width'], $data['height'])) {

Wondering what kind of API makes sense for all these functions (follow-up material, of course). Should they be on the manager, or the plugin?

Also, why is $image by reference? Shouldn't be necessary unless we do $image = $something else within the function/plugin which would a) be weird and b) then we'd had the & before as well?

Also not sure what's the standard for type hinting stdClass, we afaik don't do that usually.

+++ b/core/modules/system/lib/Drupal/system/Plugin/ImageToolkitInterface.phpundefined
@@ -0,0 +1,165 @@
+   * @param $image
+   *   An image object. The $image->resource value will be modified by this
+   *   call.
+   *
+   * @return bool
+   *   TRUE or FALSE, based on success.
+   *
+   * @see image_desaturate()
+   */
+  function desaturate(stdClass $image);

Missing the stdClass, given that we actually want to add it.

Berdir’s picture

According to http://drupal.org/coding-standards/docs, we should be using object instead of stdClass which would then imply no type hint as object implies any object.

Berdir’s picture

Status: Needs review » Needs work
Issue tags: +API change, +Configuration system, +SprintWeekend2013

The last submitted patch, 1664844.image-toolkits-as-plugins.84.patch, failed testing.

alexpott’s picture

Status: Needs work » Needs review
FileSize
87.38 KB
47.79 KB

Yep the stdClass typehint is basically meaningless so I removed it. And converted all @param stdClass to @param object. Removed the unnecessary passing by reference... not sure where that crept in.

Not sure about creating a Drupal::imageToolkit() function so I've left that out.

The interdiff is a bit tricky to read due to the recent revert of image.settings::jpeg_quality patch.

Status: Needs review » Needs work

The last submitted patch, 1664844.image-toolkits-as-plugins.90.patch, failed testing.

alexpott’s picture

Status: Needs work » Needs review
FileSize
87.39 KB
674 bytes

Doh... use of container in tests...

Berdir’s picture

Status: Needs review » Reviewed & tested by the community

Looked through this again. It's a big patch (with tons of not strictly necessary doc-improvements, might have been better to leave that out but doesn't make sense to revert it now) but I think it's fine as a first step. This also removes some tricky variable_get() (dynamic plugin specific configuration).

Has some strange things, like the $image stdClass and $image->toolkit->method but that already existed more or less like that before this patch so it makes sense to do it like this as we would have to change a lot more otherwise. Can we maybe open a follow-up issue to get an overview of how image effects and the toolkit are working together exactly and look at how we can improve the whole thing further?

One thing that I noticed, that can be fixed if this needs a re-roll or in a follow-up patch.

+++ b/core/modules/system/lib/Drupal/system/Plugin/ImageToolkitInterface.phpundefined
@@ -0,0 +1,163 @@
+   * @return bool
+   *   True if the GD toolkit is available on this machine.

This is on the interface, so shouldn't be about GD :)

gdd’s picture

I think a lot of these cleanups to the way effects and plugins work together make sense to happen in #1821854: Convert image effects into plugins or possibly in a followup from that issue.

alexpott’s picture

xjm’s picture

catch’s picture

Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.x, thanks!

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.

tim.plunkett’s picture

Title: Convert image toolkits into plugins » Change notice: Convert image toolkits into plugins
Priority: Normal » Critical
Status: Closed (fixed) » Active

This needs a change notice. Also see #1987298: Shorten directory structure and PSR-0 namespacing for plugins for the updated path.

alexpott’s picture

Priority: Critical » Minor
Status: Active » Fixed

Change notice created https://drupal.org/node/1993056

marcvangend’s picture

Status: Fixed » Needs work

Minor issue in the change notice: the "D7" heading isn't closed properly.

<h4>D7<br /><h4>

This causes invalid HTML and weird rendering of the code block below it.

ParisLiakos’s picture

Title: Change notice: Convert image toolkits into plugins » Convert image toolkits into plugins
Priority: Minor » Normal
Status: Needs work » Fixed

Automatically closed -- issue fixed for 2 weeks with no activity.

Anonymous’s picture

Issue summary: View changes

Updated issue summary.