Lazy-load GD resource in GDToolkit

1. +++ b/core/lib/Drupal/Core/Image/ImageInterface.php
   @@ -21,6 +21,16 @@
   +   * Sets the image validity flag.
   +   *
   +   * @param bool $valid
   +   *   TRUE if the image object contains a valid image, FALSE otherwise.
   +   *
   +   * @return $this
   +   */
   +  public function setValid($valid);
   +
   

   Please no, not another method on the interface that means nothing to the outside world. If Image needs info from the toolkit about the validity of the image source, add a method on toolkit that gets called by image.

2. +++ b/core/modules/system/src/Plugin/ImageToolkit/GDToolkit.php
   @@ -50,11 +57,18 @@ public function setResource($resource) {
   +    if (isset($this->resource)) {
   +      return $this->resource;
   +    }
   +    elseif ($this->getImage()->isValid() && isset($this->preLoadInfo) && $this->load()) {
   +      unset($this->preLoadInfo);
   +      return $this->resource;
   +    }
   +    return NULL;
      }
   

   This seems like too much activity and logic in a getter: this should end up as something like:

   $this->load();
   return $this->resource;
   

   Where load has a quick return path or like:

   if (!$this->resource) {
     $this->load();
   }
   return $this->resource;
   

   When load() will always execute the loading.

3. +++ b/core/modules/system/src/Plugin/ImageToolkit/GDToolkit.php
   @@ -215,6 +238,9 @@ public function createTmp($type, $width, $height) {
   +    if (isset($this->preLoadInfo)) {
   +      return $this->preLoadInfo[0];
   +    }
        return $this->getResource() ? imagesx($this->getResource()) : NULL;
      }
   

   In cases where we have 3 options, I would prefer using the same logic, thus either a nested ternary or an if/elseif/else, not an if followed by a ternary. (Same for getHeight)

4. +++ b/core/tests/Drupal/Tests/Core/Image/ImageTest.php
   @@ -92,19 +92,31 @@ protected function getToolkitOperationMock($class_name, ImageToolkitInterface $t
   +   * @param bool $load_expected
   +   *   (optional) Whether the load() method is expected to ba called. Defaults
   +   *   to TRUE.
       * @param array $stubs
   

   be (instead of ba)

The first point will completely change the patch, so a new extensive review will be needed by then. However, I also doubt if this whole exercise is needed at all, as I prefer to reduce the functionality of the image system further according to the (now) related issue #2211227: Refactor image and imagetoolkit: isExisting, isSupported, supportedTypes, getMimeType. If we do that, there won't remain (m)any cases where an image object is created that will not undergo any processing. (Image field dimensions validation and then deciding that it is not needed to scale down is probably such a case.)

What do you think?

Great, this is much better. I would RTBC it in this state, but would even more appreciate it if the few nitpicks below are solved. These are not all are introduced by this patch here, but as this patch does touch the code, we might as well improve it here in this patch. So you may set it to NR (with a new patch) or RTBC if you think that these should not be part of this patch.

1. +++ b/core/lib/Drupal/Core/Image/Image.php
   @@ -157,10 +150,10 @@ public function save($destination = NULL) {
       * @return bool
       *   FALSE, if the file could not be found or is not an image. Otherwise, the
       *   image information is populated.
       */
      protected function parseFile() {
   -    if ($this->valid = $this->getToolkit()->parseFile()) {
   

   This is not properly documenting the return value but the desired side effect of the function.

2. +++ b/core/modules/system/src/Plugin/ImageToolkit/GDToolkit.php
   @@ -26,7 +26,7 @@ class GDToolkit extends ImageToolkitBase {
       *
       * @var resource
       */
   -  protected $resource;
   +  protected $resource = NULL;
    
   

   @var resource|null (as we accept and explicitly handle null values for this property).

3. +++ b/core/modules/system/src/Plugin/ImageToolkit/GDToolkit.php
   @@ -36,6 +36,13 @@ class GDToolkit extends ImageToolkitBase {
      /**
   +   * Image information from a file, available prior to loading the GD resource.
   +   *
   +   * @var array
   +   */
   +  protected $preLoadInfo = NULL;
   +
   

   - @ var array|null (as we accept and explicitly handle null values for this property).
   - We may consider more explicitly documenting what info from a file it contains, i.e. refer to @see imagegetsize().

4. +++ b/core/modules/system/src/Plugin/ImageToolkit/GDToolkit.php
   @@ -44,6 +51,9 @@ class GDToolkit extends ImageToolkitBase {
   +    if ($this->preLoadInfo) {
   +      $this->preLoadInfo = NULL;
   +    }
        $this->resource = $resource;
   

   The if is not necessary.

Top, even better so.

Hm. That $load_expected param kinda hurts my brain, but I guess as long as it's isolated to tests it's not a huge deal.

Committed and pushed to 8.x. Thanks!

Thanks, added a remark to issue #2109459: Review image test suite about that param.