Remove setWidth and setHeight from ImageInterface

I tried to review the patch. IMO this is far more than this issue is about. So I could highlight all things that do not belong to the original intent of this issue, but I will await your explanation. My idea is to not do the other features it in this patch, but in a follow-up (the already created and now related issue). This because the features seem so unrelated. Moreover, the whole image system is about processing existing images. The only use through Drupal (and that includes all of D7 contrib as known by me) is: take an existing image, process it (resize on file upload or create derivatives using image styles). So we are talking about a feature request! This issue is about misleading and incorrect interface methods. And now we are mixing in a feature request to this issue to be able to start an image from scratch (blank canvas). I'm not opposed to that, but can we defer that to #2063373: Cannot save image created from scratch?

If all these changes are needed to keep some tests running, I would think that these tests are wrong and should be removed or adapted. If so, what tests are the failing ones?

If #1 needs to be addressed in this issue (seems a bit unrelated as well), we need to define what this means: do you really need to create an image object or do you need to be able create another GD resource or do you need to be able to stack commands to imagemagick? In the latter case, the Image object is not the place to do so, we will have toolkit operations for that.

[EDIT: cross-post: I am referring to the patch in #5, I am awaiting your explanation]

1. +++ b/core/lib/Drupal/Core/Image/Image.php
   @@ -91,8 +77,11 @@ class Image implements ImageInterface {
   +    if ($source) {
   +      $this->source = $source;
   

   So Image is able to handle not passing in a source. Can you adapt the comment to something like:
   * @param string $source
   * The path to an image file or the empty string to start with no image source.
   Or make it allow NULL as well

2. +++ b/core/lib/Drupal/Core/Image/Image.php
   @@ -91,8 +77,11 @@ class Image implements ImageInterface {
   +      $this->processInfo();
   +    }
   

   Not sure that this is needed here now, though it would mean we could remove all the calls to it in the getters (if we also add it to setSource() and save(), though that one has this call already).

3. +++ b/core/lib/Drupal/Core/Image/ImageFactory.php
   @@ -53,19 +53,38 @@ public function setToolkitId($toolkit_id) {
   +   * Constructs a new Image object from an image file.
       *
   

   A bit inconsistent with the change on the Image constructor, where we now do accept an empty source param.

4. +++ b/core/lib/Drupal/Core/Image/ImageFactory.php
   @@ -53,19 +53,38 @@ public function setToolkitId($toolkit_id) {
   +   * @param string $toolkit_id
   +   *   (Optional) The image toolkit ID to use for this image.
       *
   

   I like this idea, though it will hardly have a normal use. But it does allow for easier and more dynamic toolkit selection. We can get the same with setToolkit and a getToolkit to be able to set the toolkit to its original value. Do we need a getToolkit anyway, for reasons of completeness? (may be a follow up).

5. +++ b/core/lib/Drupal/Core/ImageToolkit/ImageToolkitInterface.php
   @@ -193,11 +190,9 @@ public function scaleAndCrop(ImageInterface $image, $width, $height);
   -   * @return array
   +   * @return array|bool
       *   FALSE, if the file could not be found or is not an image. Otherwise, a
   

   I think that the Drupal standard is to specify array|FALSE (not official phpdoc syntax, but it is clearer).

6. +++ b/core/modules/system/lib/Drupal/system/Plugin/ImageToolkit/GDToolkit.php
   @@ -304,10 +295,8 @@ public function getInfo(ImageInterface $image) {
   -    if (isset($data) && is_array($data) && in_array($data[2], $this->supportedTypes())) {
   +    if (isset($data) && is_array($data) && in_array($data[2], static::supportedTypes())) {
          $details = array(
   

   Correct, but may be postponed to one of the follow-ups.

7. +++ b/core/tests/Drupal/Tests/Core/Image/ImageTest.php
   @@ -70,10 +60,14 @@ protected function setUp() {
      protected function getToolkitMock(array $stubs = array()) {
        $mock_builder = $this->getMockBuilder('Drupal\system\Plugin\ImageToolkit\GDToolkit');
        if ($stubs && is_array($stubs)) {
   -      $mock_builder->setMethods($stubs);
   +      $stubs += array('getPluginId', 'save');
   +    }
   +    else {
   
   @@ -208,11 +184,13 @@ public function testChmodFails() {
   +    $this->assertFalse((bool) $image->getWidth());
      }
   

   The type hinting will assure that you have an array, and NULL is not an array. So the if condition and the else branch can be removed.

8. +++ b/core/tests/Drupal/Tests/Core/Image/ImageTest.php
   @@ -208,11 +184,13 @@ public function testChmodFails() {
   +    $this->assertFalse((bool) $image->getWidth());
      }
   

   The specification says "..., or 0 if the image is invalid". Tests should test the specification (and as such clarify the specification), so lets test for 0.

In general the patch is OK, though there are a few points above that need to be solved (1, 5, 7, 8). Other points are more a matter of do it here or in the designated follow ups that already exist: #2211227: Refactor image and imagetoolkit: isExisting, isSupported, supportedTypes, getMimeType and #2063373: Cannot save image created from scratch.

1. +++ b/core/lib/Drupal/Core/Image/ImageFactory.php
   @@ -53,10 +53,21 @@ public function setToolkitId($toolkit_id) {
       * @param string $source
   -   *   The path to an image file.
   +   *   The path to an image file, or NULL to construct the object with no
   +   *   image source.
       * @param string $toolkit_id
   

   @param string|NULL $source (change accordingly for constructor in Image as well)

2. +++ b/core/tests/Drupal/Tests/Core/Image/ImageTest.php
   @@ -55,11 +55,11 @@ protected function setUp() {
   -    if ($stubs && is_array($stubs)) {
   +    if (!empty($stubs)) {
          $stubs += array('getPluginId', 'save');
   

   Formerly, you did add them in all cases, now only if the array passed in is not empty?

With the 2 changes from 1 this is good to go for me. Updated issue summary to reflect current status and contents of patch.

Sorry, I did not read the patch correctly. So my remark was a bit off and thus too vague for you:

+++ b/core/tests/Drupal/Tests/Core/Image/ImageTest.php
@@ -65,15 +55,19 @@ protected function setUp() {
+    if (!empty($stubs)) {
+      $stubs += array('getPluginId', 'save');
+    }
+    else {
+      $stubs = array('getPluginId', 'save');
     }
     return $mock_builder

Because we know that stubs is an array, the single line
$stubs += array('getPluginId', 'save');
will work both when stubs is an empty array and when it is not an empty array. So replace the whole if-else with this single line (from the if branch).

Thanks.

+++ b/core/lib/Drupal/Core/Image/ImageFactory.php
@@ -59,43 +60,33 @@ public function setToolkitId($toolkit_id) {
   public function get($source, $toolkit_id = NULL) {
-    $toolkit_id = $this->resolveToolkitId($toolkit_id);

public function get($source, $toolkit_id = '') {

#34.2: the way that #35 solves this is better, as getToolkitId() does not initialize the value,it just returns it. So it should indeed be set in the constructor.

I am looking into the test failure, but as that also happens in #2217783: Remove $extension and $mimeType from Image, use image_type_to_xxx function in the getters instead I don't think it is related to this change.

Locally the test passes. Though I did get an "PHP Fatal error: Allowed memory size of 134217728 bytes exhausted (tried to allocate 1130497 bytes) in ...\core\includes\common.inc on line 3917" While browsing to the test page. I think that rendering can be done more memory efficient, but that's something else.

@mondrake: can you make the little change from #37 and set to NR again?

On a last review I found this tricky bug (already introduced in the parent issue):

+++ b/core/modules/file/lib/Drupal/file/Tests/ValidatorTest.php
@@ -79,7 +79,7 @@ function testFileValidateImageResolution() {
+    if ($this->container->get('image.toolkit.manager')->getDefaultToolkitId()) {
       // Copy the image so that the original doesn't get resized.

All image objects that get created will now get a toolkit assigned based on the toolkitId property of the ImageFactory. This means that code outside the image system may no longer call getDefaultToolkitId() on the toolkit manager, because that may not be the image toolkit in use! So

• ALL usages of getDefaultToolkitId() outside the image system should use getToolkitId() on ImageFactory instead. This method is only called in this test, so no real harm is done here, but I propose to change it anyway as test should also document how things are to be used/queried.
• ALL usages of getDefaultToolkit() outside the image system should use getToolkit() on an image itself, if available, or use createInstance() and pass in the id from getToolkitId() from ImageFactory. This method is called in file.module and image.install. For completeness (best practices, documentation) and robustness, I would change the latter as well, although at this moment, I can't come up with a scenario where its usage might do any harm. The usage in file.module is easy to solve as there is already an image object.

If we don't do so, advanced scenario's in contrib, where e.g. toolkits are switched based on image field instance or whatever use case one can come up with, might fail.

This error was already introduced in the parent issue, but this issue allows us to solve it, as it adds a getToolkitId() method on Image Factory. Sometimes plans do come together...

IMO the validator test can and should use:

     if ($this->container->get('image.factory)->getToolkitId()) {

As that one returns exactly the same value in a set-up where the default toolkit is used (and thus also a value that evaluates to false when no toolkit is available). IMO this is better as toolkit becomes more and more a thing on the background. Image and ImageFactory are the classes that can be used by the rest of the system. Imagetoolkit and ImageToolkitManager should be avoided whenever possible. This would make the test reusable for other toolkits, although they are not the subject of the test.

Similarly image.install can use:

  $toolkit = \Drupal::service('image.toolkit.manager')->createInstance(\Drupal::service('image.factory')->getToolkitId());

But I agree that is too far off for this issue.

Great, thanks.

All the empty string versus NULL is from @catch's remark in #34. To say that this is the common pattern for PHP is too strong. PHP is notoriously bad in handling edge cases like the absence of a value (too many ways, empty versus !isset etc.). if I can see any tendency, it is towards working more type strictly, thus specify 1 type only and have an empty value within that type. int and string do have such a value and as 0 is indeed not a valid value for a width or height it is usable.

RE #46.8: We removed width and height from Image, not toolkit. GD doesn't need to store the width and height in properties as it has imagesx() and imagesy(). OTOH, IM will need to keep track of width and height as it has no resource like GD that keeps track of this info.. So, these properties do not belong to a base object as they are really implementation specific.

For me, this issue remains RTBC, except perhaps 5, but that can be done on committing.

Setting to RTBC as per #49 and a review of the reroll. The article #48 refers to is not very explicit on the use of NULL, it also tells us to return 1 type only, or to throw an exception. So IMO an "empty" value within a type also does.

I have to (partially) disagree with @catch. The article from Larry Garfield teaches us: throw an exception or return null when the array is actually a struct and not a collection.

So returning false indeed is wrong, as is (IMO) returning an empty struct. But this can be handled when we really solve working with blank canvases #2063373: Cannot save image created from scratch. Then we'll see if we better throw an error.