Support from Acquia helps fund testing for Drupal Acquia logo

Comments

tim.plunkett’s picture

Status: Active » Needs review
FileSize
15.89 KB

Status: Needs review » Needs work

The last submitted patch, image-2048827-1.patch, failed testing.

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
3.36 KB
17.59 KB

Some tests were manually creating the image toolkit manager, no point.

claudiu.cristea’s picture

The patch is moving the interface and the manager to Drupal\Core but still keep the GD implementation in the system module. An explanation may be the fact that GD is the implementation shipped with Drupal. But I see the general tests left in system (ToolkitTestBase & ToolkitTest). Shouldn't those be moved to Core as well?

claudiu.cristea’s picture

Assigned: Unassigned » claudiu.cristea
FileSize
33.21 KB

Just ignore my latest comment (no location for tests).

I reworked a little bit and rerolled this. I'm taking this so that I can track. It was hard to get an interdiff.

mondrake’s picture

Status: Needs review » Needs work
  • Why renaming image.toolkit.manager to plugin.manager.image_toolkit?
  • Would it make sense to move also the GDToolkit implementation to core somehow? It's weird to have the manager in core and the implementation in the system module. Also, Image is in core.
claudiu.cristea’s picture

Status: Needs work » Needs review

There's no need to switch to "needs work".

Why renaming image.toolkit.manager to plugin.manager.image_toolkit?

All plugin manager services are prefixed with plugin.manager.. It's simply a naming consistency reason and here we switch image tookit plugin manager to this standard. See how other plugin manager services are named.

Would it make sense to move also the GDToolkit implementation to core somehow? It's weird to have the manager in core and the implementation in the system module. Also, Image is in core.

Well, in #4 I asked the same thing but I also found the answer there. It makes perfectly sense to provide the image system foundation in core. And then the implementation is provided by modules. But because Drupal is shipped with a default toolkit implementation (GD) and that one has to be installed by default when Drupal is installed, it is provided by system.module. Other toolkits may be implemented in contrib modules, for symmetry the default implementation should be provided in the same way, by a module.

tim.plunkett’s picture

Removed some of the out of scope changes. This is a simple move of the API, let's keep it that way. Any moving of GS can be done in a separate issue.

mondrake’s picture

Status: Needs review » Reviewed & tested by the community

Simple move of API files / change of namespaces. Feedback addressed. RTBC for me.

mondrake’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/lib/Drupal/Core/ImageToolkit/ImageToolkitInterface.php
@@ -166,8 +166,8 @@ public function save(ImageInterface $image, $destination);
    *   keyed array containing information about the image:
    *   - "width": Width, in pixels.
    *   - "height": Height, in pixels.
-   *   - "type": Image type represented as an IMAGETYPE_* constant.
-   *   - "mime_type": MIME type (e.g. 'image/jpeg', 'image/gif', 'image/png').
+   *   - "extension": Commonly used file extension for the image.
+   *   - "mime_type": MIME type ('image/jpeg', 'image/gif', 'image/png').
    *
    * @see \Drupal\Core\Image\ImageInterface::processInfo()

Oops no, sorry, this is a regression - missed a pull maybe?

claudiu.cristea’s picture

Status: Needs work » Reviewed & tested by the community
+++ b/core/lib/Drupal/Core/ImageToolkit/ImageToolkitInterface.php
@@ -166,8 +166,8 @@ public function save(ImageInterface $image, $destination);
-   *   - "type": Image type represented as an IMAGETYPE_* constant.
-   *   - "mime_type": MIME type (e.g. 'image/jpeg', 'image/gif', 'image/png').
+   *   - "extension": Commonly used file extension for the image.
+   *   - "mime_type": MIME type ('image/jpeg', 'image/gif', 'image/png').

This is a regression. The "type" key has been introduced recently, in #2066219: Decouple image type from image extension and "extension" was dropped.

claudiu.cristea’s picture

Status: Reviewed & tested by the community » Needs work

Sorry. Cross-posting on the same issue :)

claudiu.cristea’s picture

But anyway, @tim.plunkett, what was out-of-scope in #5? I cannot see without an interdiff.

claudiu.cristea’s picture

Status: Needs work » Needs review
FileSize
876 bytes
17.49 KB

Fixed #10 (#11).

tim.plunkett’s picture

Injecting the config factory, mostly.

claudiu.cristea’s picture

@tim.plunkett

Oh, yes, totally forgot about that :)

mondrake’s picture

Status: Needs review » Reviewed & tested by the community

So, RTBC now.

claudiu.cristea’s picture

Status: Reviewed & tested by the community » Needs review
tim.plunkett’s picture

Status: Needs review » Reviewed & tested by the community

Crosspost

webchick’s picture

+++ b/core/core.services.yml
@@ -524,13 +524,13 @@ services:
-  image.toolkit.manager:
-    class: Drupal\system\Plugin\ImageToolkitManager
+  plugin.manager.image_toolkit:
+    class: Drupal\Core\ImageToolkit\ImageToolkitManager

So, yay for removing "Plugin" from the class location. As a module dev, I need to know that there are image toolkits and how to work with them, I do not need to know nor care how image toolkits are implemented under the hood.

But then boo, because then we *undo* that very same change in the machine name by adding "plugin.manager" to it. :(

Can we back out that change? If this really is a standard in core, I'd like to challenge it, because it's violating the Law of Demeter.

Otherwise, looks good!

webchick’s picture

Status: Reviewed & tested by the community » Needs work
dawehner’s picture

claudiu.cristea’s picture

Status: Needs work » Needs review
FileSize
14.28 KB

Dropped service renaming as it's not in the scope. We need this in asap otherwise we'll be forced to do too many rerolls in issues from #2105863: [meta] Images, toolkits and operations.

If renaming the manager is important I will open a new issue only for that.

Status: Needs review » Needs work

The last submitted patch, move-toolkit-2048827-23.patch, failed testing.

claudiu.cristea’s picture

Status: Needs work » Needs review
FileSize
537 bytes
14.41 KB

Ouch!

mondrake’s picture

Status: Needs review » Reviewed & tested by the community

back to RTBC

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Awesome, thank you!

Committed and pushed to 8.x.

claudiu.cristea’s picture

Thank you. Unfortunately we forgot to move \Drupal\system\Annotation\ImageToolkit too along with this relocation. Shame!

Opened a followup in #2108077: ImageToolkit annotation object left under system module.

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