Remove $image parameter in calls between and within Image and toolkit

Review of interdiff code in #4

+++ b/core/lib/Drupal/Core/ImageToolkit/ImageToolkitInterface.php
@@ -60,25 +60,39 @@ public function settingsForm();
+   * @throws \BadMethodCallException when called twice.

The @throws description should be a separate line, see the coding standards.
Also, I see @throws is usually after the @param/@return blocks. I don't know if it matters though.

+++ b/core/modules/system/lib/Drupal/system/Plugin/ImageToolkit/GDToolkit.php
@@ -7,6 +7,7 @@
+use BadMethodCallException;

You don't need use for root classes, just call \BadMethodCallException with backslash in the code. OTOH, how about introducing a specific ImageToolkitException here? There's one waiting in #2122605-18: Remove isAvailable() from ImageToolkitInterface.

+++ b/core/modules/system/lib/Drupal/system/Plugin/ImageToolkit/GDToolkit.php
@@ -37,6 +45,23 @@ class GDToolkit extends ImageToolkitBase {
+      throw new BadMethodCallException(__METHOD__ . '() may only be called once');

Same here

+++ b/core/modules/system/lib/Drupal/system/Plugin/ImageToolkit/GDToolkit.php
@@ -23,6 +24,13 @@
+   * Image object this toolkit instance is tied to..
+   *
+   * @var \Drupal\Core\Image\ImageInterface
+   */
+  protected $image;
+
+  /**
    * A GD image resource.
    *
    * @var resource
@@ -37,6 +45,23 @@ class GDToolkit extends ImageToolkitBase {

@@ -37,6 +45,23 @@ class GDToolkit extends ImageToolkitBase {
   protected $type;
 
   /**
+   * {@inheritdoc}
+   */
+  public function setImage(ImageInterface $image) {
+    if ($this->image) {
+      throw new BadMethodCallException(__METHOD__ . '() may only be called once');
+    }
+    $this->image = $image;
+  }
+
+  /**
+   * {@inheritdoc}
+   */
+  public function getImage() {
+    return $this->image;
+  }
+

This could go to the ImageToolkitBase? I am expecting this to be same in all toolkits, and GDToolkit and TestToolkit already extend from ImageToolkitBase.

+++ b/core/modules/system/lib/Drupal/system/Plugin/ImageToolkit/GDToolkit.php
@@ -23,6 +24,13 @@
+   * Image object this toolkit instance is tied to..

One full stop too much ;)

+++ b/core/modules/system/lib/Drupal/system/Plugin/ImageToolkit/GDToolkit.php
@@ -37,6 +45,23 @@ class GDToolkit extends ImageToolkitBase {
+      throw new BadMethodCallException(__METHOD__ . '() may only be called once');

Full stop at the end of text?

1. +++ b/core/modules/system/tests/modules/image_test/lib/Drupal/image_test/Plugin/ImageToolkit/TestToolkit.php
   @@ -7,6 +7,7 @@
   +use BadMethodCallException;
   

   Leftover, it's in the parent class now

2. +++ b/core/modules/system/tests/modules/image_test/lib/Drupal/image_test/Plugin/ImageToolkit/TestToolkit.php
   @@ -45,6 +53,23 @@ class TestToolkit extends ImageToolkitBase {
   +  public function setImage(ImageInterface $image) {
   +    if ($this->image) {
   +      throw new BadMethodCallException(__METHOD__ . '() may only be called once');
   +    }
   +    $this->image = $image;
   +  }
   +
   +  /**
   +   * {@inheritdoc}
   +   */
   +  public function getImage() {
   +    return $this->image;
   +  }
   +
   +  /**
   +   * {@inheritdoc}
   +   */
   

   Same

+++ b/core/modules/system/tests/modules/image_test/lib/Drupal/image_test/Plugin/ImageToolkit/TestToolkit.php
@@ -7,6 +7,7 @@
 use Drupal\Core\Image\ImageInterface;

I suppose it can be removed here too, now?

Sorry, couple more minor things

+++ b/core/modules/system/lib/Drupal/system/Plugin/ImageToolkit/GDToolkit.php
@@ -8,7 +8,6 @@
@@ -35,7 +34,6 @@ class GDToolkit extends ImageToolkitBase {

@@ -35,7 +34,6 @@ class GDToolkit extends ImageToolkitBase {
    * @var int
    */
   protected $type;
-
   /**

Let's keep this blank line.

+++ b/core/tests/Drupal/Tests/Core/Image/ImageTest.php
@@ -16,6 +16,13 @@
   /**
+   * Path to the image file.
+   *
+   * @var string
+   */
+  protected $source;
+

Just to check, this is because setUp() is setting this property but it was not declared in the class, right?

Seems OK now, we need to wait for the parent to get in first.

+++ b/core/lib/Drupal/Core/Image/Image.php
@@ -58,6 +58,7 @@ class Image implements ImageInterface {
+    $this->toolkit->setImage($this);

@@ -75,14 +76,14 @@ public function isValid() {
-    return $this->toolkit->getHeight($this);
+    return $this->toolkit->getHeight();

+++ b/core/lib/Drupal/Core/ImageToolkit/ImageToolkitBase.php
@@ -7,11 +7,36 @@
+      throw new \BadMethodCallException(__METHOD__ . '() may only be called once.');

Why is this desirable? Why would we *choose* to make the toolkit stateful?

Can we edit https://drupal.org/node/2084547 to add a reference to this issue? That way, the CR can document the work in progress and will appear in the sidebar of this node.

Also, this and that shouldn't both be RTBC at the same time. This seriously confused me. :P .Postponing on #2211227: Refactor image and imagetoolkit: isExisting, isSupported, supportedTypes, getMimeType.

Very minor, but let's fix it anyway :)

+++ b/core/lib/Drupal/Core/ImageToolkit/ImageToolkitInterface.php
@@ -244,10 +235,10 @@ public function getMimeType(ImageInterface $image);
+   *   True if the toolkit is available on this machine, false otherwise.

TRUE and FALSE (capital letters)

Good to go for me. Nice cleanup. RTBC

Committed/pushed to 8.x, thanks!