The service definitions are already in core.services.yml instead of system, we should move the classes as well.

Files: 
CommentFileSizeAuthor
#25 move-toolkit-2048827-25.patch14.41 KBclaudiu.cristea
PASSED: [[SimpleTest]]: [MySQL] 59,027 pass(es).
[ View ]
#25 interdiff.txt537 bytesclaudiu.cristea
#23 move-toolkit-2048827-23.patch14.28 KBclaudiu.cristea
FAILED: [[SimpleTest]]: [MySQL] 56,844 pass(es), 80 fail(s), and 58 exception(s).
[ View ]
#14 move-toolkit-2048827-14.patch17.49 KBclaudiu.cristea
PASSED: [[SimpleTest]]: [MySQL] 58,923 pass(es).
[ View ]
#14 interdiff.txt876 bytesclaudiu.cristea
#8 image-toolkit-2048827-8.patch16.41 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 58,412 pass(es).
[ View ]
#5 move-toolkit-2048827-5.patch33.21 KBclaudiu.cristea
PASSED: [[SimpleTest]]: [MySQL] 59,032 pass(es).
[ View ]
#3 image-2048827-3.patch17.59 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 57,459 pass(es).
[ View ]
#3 interdiff.txt3.36 KBtim.plunkett
#1 image-2048827-1.patch15.89 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] 57,491 pass(es), 0 fail(s), and 38 exception(s).
[ View ]

Comments

Status:Active» Needs review
StatusFileSize
new15.89 KB
FAILED: [[SimpleTest]]: [MySQL] 57,491 pass(es), 0 fail(s), and 38 exception(s).
[ View ]

Status:Needs review» Needs work

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

Status:Needs work» Needs review
StatusFileSize
new3.36 KB
new17.59 KB
PASSED: [[SimpleTest]]: [MySQL] 57,459 pass(es).
[ View ]

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

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?

Assigned:Unassigned» claudiu.cristea
StatusFileSize
new33.21 KB
PASSED: [[SimpleTest]]: [MySQL] 59,032 pass(es).
[ View ]

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.

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.

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.

StatusFileSize
new16.41 KB
PASSED: [[SimpleTest]]: [MySQL] 58,412 pass(es).
[ View ]

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.

Status:Needs review» Reviewed & tested by the community

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

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?

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.

Status:Reviewed & tested by the community» Needs work

Sorry. Cross-posting on the same issue :)

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

Status:Needs work» Needs review
StatusFileSize
new876 bytes
new17.49 KB
PASSED: [[SimpleTest]]: [MySQL] 58,923 pass(es).
[ View ]

Fixed #10 (#11).

Injecting the config factory, mostly.

@tim.plunkett

Oh, yes, totally forgot about that :)

Status:Needs review» Reviewed & tested by the community

So, RTBC now.

Status:Reviewed & tested by the community» Needs review

Status:Needs review» Reviewed & tested by the community

Crosspost

+++ 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!

Status:Reviewed & tested by the community» Needs work

Status:Needs work» Needs review
StatusFileSize
new14.28 KB
FAILED: [[SimpleTest]]: [MySQL] 56,844 pass(es), 80 fail(s), and 58 exception(s).
[ View ]

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.

Status:Needs work» Needs review
StatusFileSize
new537 bytes
new14.41 KB
PASSED: [[SimpleTest]]: [MySQL] 59,027 pass(es).
[ View ]

Ouch!

Status:Needs review» Reviewed & tested by the community

back to RTBC

Status:Reviewed & tested by the community» Fixed

Awesome, thank you!

Committed and pushed to 8.x.

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.