Problem/Motivation

Up to #2244359: Lazy-load GD resource in GDToolkit, it was not possible to build and save an image from scratch.
It was only possible to load an image from a file, manipulate it and save it at a destination.
Getting an image object from the image.factory without specifying a file to read from led to an invalid image object.
Invalid image objects would not save and there was no way to change the validity status through image operations.

Proposed resolution

#2244359: Lazy-load GD resource in GDToolkit moved image validity status checking to the toolkit, so it is now possible to get image objects from scratch and modify their validity status through image operation.
However we are missing a core operation to do that.
The proposal is to introduce a new 'create_new' image operation that would initialise an image if it is not got from file, and let each toolkit manage what it means to it.
To match the OP code, this would mean that a new image could be created like

    $destination =  'public://image.png';
    
    $image = \Drupal::service('image.factory')->get();
    $image->createNew(20, 20);
    // ... any other image manipulation operation
    $image->save($destination);

which is toolkit agnostic.

For the GD toolkit, this means splitting the current 'createTmp' method into a 'getTransparentColor' method (to fetch transparent color channel from current resource) and a 'create_new' image toolkit operation plugin.

Remaining tasks

Review patch.

User interface changes

None.

API changes

One more method added to ImageInterface: createNew()

Original report by @claudiu.cristea

Updated: Comment #25

Problem/Motivation

I'm expecting that next piece of code will create an image on disk from an image resource:

    $destination =  'public://image.png';
    $res = imagecreatetruecolor(20, 20);

    $image = new Image($destination, \Drupal::service('image.toolkit'));
    $image
      ->setResource($res)
      ->setWidth(20)
      ->setHeight(20)
      ->save();

But it doesn't! :(

(unless I didn't understood well how to use the new Image class or I missed something)

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

claudiu.cristea’s picture

Status: Active » Needs review
FileSize
2.56 KB

I wrote 2 tests: a PHPUnit test and also a simpletest (for qa.drupal.org), both illustrating this bug.

Status: Needs review » Needs work

The last submitted patch, 2063373-1.patch, failed testing.

claudiu.cristea’s picture

Status: Needs work » Needs review
FileSize
1.48 KB

Dropped the phpunit test.

claudiu.cristea’s picture

Title: Image created from resource won't save » Cannot save image created from resource

Status: Needs review » Needs work

The last submitted patch, image-save-2063373-3.patch, failed testing.

larowlan’s picture

You shouldn't need setResource just new Image and save()

claudiu.cristea’s picture

Status: Needs review » Needs work

@larowlan, and how to pass the resource? In this case it's an empty one but imagine that you need to generate an image from a processed resource. It should work.

larowlan’s picture

Status: Needs work » Needs review
FileSize
2.94 KB
2.92 KB
@@ -177,4 +178,23 @@ protected function assertImageEffect($effect_name, array $data) {
+    $image = new Image($destination, \Drupal::service('image.toolkit'));

$this->container->get('image.toolkit') would be better in this case

So what we're missing here is ::setExtension() which prevents the call into ::processInfo() and makes the test pass.

claudiu.cristea’s picture

Status: Needs work » Needs review

Great! That was my guess too. Didn't have time to test :)

Why we don't try first to extract the extension from $image->getSource()? Maybe in ::processInfo()? Course, only if Image::$extension is empty. In that case the simpletest should work without ::setExtension() (but keep ::setExtension() method too).

larowlan’s picture

there is a line something like this in processInfo() (typing from memory mind you)

if (!file_exists($this->getSource()) && !is_uploaded_file($this->getSource()) {
  return FALSE;
}

So that doesn't work with generated images. Ie there are three uses for the API, existing files, uploaded (tmp) files and dynamically generated files.
Without an extension set ->save() looks for a function named image, instead of imagepng/imagejpeg, and just dies.
With it set, it just works.
So I think getExtension is needed.

claudiu.cristea’s picture

Here's my idea from #9.

claudiu.cristea’s picture

Just a comment to #11:

If the file is on disk or ::setExtension() is explicitly used, those will take precedence -- and that's fine.

larowlan’s picture

Yep, happy with that, can you update the UnitTest too, we had 100% coverage on that class and I'm keen to keep it that way.
Should be able to do create an image with a nonexistent path but containing a .png and then call getExtension() and still get the right thing.

larowlan’s picture

updates unit test coverage

larowlan’s picture

Code coverage report before #14

Screen Shot 2013-08-13 at 5.55.15 PM.png

and after

Screen Shot 2013-08-13 at 5.54.15 PM.png

andypost’s picture

@@ -369,14 +369,6 @@ services:
-  private_key:
-    class: Drupal\Core\PrivateKey
-    arguments: ['@state']
-  csrf_token:
-    class: Drupal\Core\Access\CsrfTokenGenerator
-    arguments: ['@private_key']
-    calls:
-      - [setRequest, ['@?request']]

patch needs re-roll against current core

Status: Needs review » Needs work

The last submitted patch, image-resource-save.13.patch, failed testing.

larowlan’s picture

Status: Needs work » Needs review
FileSize
3.45 KB

yeah I stuffed the commands royally

Status: Needs review » Needs work

The last submitted patch, image-resource-save.16.patch, failed testing.

claudiu.cristea’s picture

Status: Needs work » Needs review
FileSize
932 bytes
4.16 KB

We need to limit the extensions to image extensions.

And this creates a new discussion: Why only .png, .jpg, .gif? First of all .jpeg is a valid JPEG extension too but ::getExtension() will return .jpg (in GDToolkit::getInfo()). And this leads to a next question: Why not all image types? Next type are already defined by PHP (I think in GD): IMAGETYPE_GIF, IMAGETYPE_JPEG, IMAGETYPE_JPEG2000, IMAGETYPE_PNG, IMAGETYPE_SWF, IMAGETYPE_PSD, IMAGETYPE_BMP, IMAGETYPE_WBMP, IMAGETYPE_XBM, IMAGETYPE_TIFF_II, IMAGETYPE_TIFF_MM, IMAGETYPE_IFF, IMAGETYPE_JB2, IMAGETYPE_JPC, IMAGETYPE_JP2, IMAGETYPE_JPX, IMAGETYPE_SWC, IMAGETYPE_ICO.

Maybe not .SWF, but I see no reason why popular .BMP cannot be added. Anyway, should this be a followup? Especially for .JPEG extension?

claudiu.cristea’s picture

Turned green, any reviews?

claudiu.cristea’s picture

arunvs’s picture

New patch with more extension

claudiu.cristea’s picture

Thank you but the patch is not qualifying for rtbc.

@@ -100,8 +100,25 @@ public function __construct($source, ImageToolkitInterface $toolkit) {
+     $this->processInfo();

Invalid indentation.

@@ -100,8 +100,25 @@ public function __construct($source, ImageToolkitInterface $toolkit) {
+    	//added new extension
+	//it is better to accept some more extentions ..
  • Invalid indent.
  • Invalid indent characters. It should always be space.
  • There must be a space between // and comment.
  • Comments should start with uppercase, end with dot.
@@ -100,8 +100,25 @@ public function __construct($source, ImageToolkitInterface $toolkit) {
+      if (in_array($extension, array('gif', 'jpg', 'png','webp'))) {
+        $this->extension = $extension;
  • Space needed after comma ","
@@ -100,8 +100,25 @@ public function __construct($source, ImageToolkitInterface $toolkit) {
+/**
++   * {@inheritdoc}
++   */
+  public function setExtension($extension) {

Invalid docblock. It shouldn't start with "+".

Generally, the extension issue needs more discussion and work. We need to handle the extension issue also in Toolkit (GDTookit). I will get back with a patch.

claudiu.cristea’s picture

claudiu.cristea’s picture

Assigned: Unassigned » claudiu.cristea

Assigning to me.

claudiu.cristea’s picture

Status: Needs review » Postponed
claudiu.cristea’s picture

Issue summary: View changes

Updated issue summary.

fietserwin’s picture

Issue summary: View changes

Status is still postponed while #2066219: Decouple image type from image extension is already a while in. But this is not bad, I even think this is good, let's now keep it postponed on: #2196067: Remove setWidth and setHeight from ImageInterface and #2168511: Allow to pass save options to ImageInterface::save.

BTW: if i look at the piece of code you expect to work, I get a good feeling about all the work we are doing to clean up the interface. ;) TRy to explain what this should do and why it should work to a newbie that you just explained about Dupal's image system and the idea that it supports multiple toolkits :(

There are a number of problems with that code:
- setResource(): this method is GD specific, while Image should be toolkit agnostic, but that is solved in [##2103621] if you know that you are working with GD, you can now do $image->getToolkit()->setResource(), otherwise you will have to create a "create canvas" toolkit operation.
- setWidth(), setHeight(): and automagically the width and height of the image are set? But that will be handled by #2196067: Remove setWidth and setHeight from ImageInterface.

The real problem is indeed in the image format you want it to save in and there we have #2168511: Allow to pass save options to ImageInterface::save for, but even then, as last fallback, using the extension of the target file is a good idea.

fietserwin’s picture

Issue tags: +Regression

According to [#32196067-10], not being able to start with a canvas, is a regression from D7. Tagging as such, priority is already major

mondrake’s picture

Component: image.module » image system
Assigned: claudiu.cristea » mondrake
Status: Postponed » Active

After #2244359: Lazy-load GD resource in GDToolkit it should now be possible to address this. Working on a patch.

mondrake’s picture

Status: Active » Needs review
FileSize
23.14 KB

Let's see what bot thinks about this. Explanations later

mondrake’s picture

Title: Cannot save image created from resource » Cannot save image created from scratch
Issue summary: View changes

Explanations in the updated issue summary.

fietserwin’s picture

  1. +++ b/core/lib/Drupal/Core/Image/ImageInterface.php
    @@ -110,6 +110,25 @@ public function apply($operation, array $arguments = array());
       /**
    +   * Prepares a new image, without loading it from a file.
    +   *
    +   * @param int $width
    +   *   The width of the new image, in pixels.
    +   * @param int $height
    +   *   The height of the new image, in pixels.
    +   * @param string $mime_type
    +   *   (Optional) The MIME type of the image (e.g. 'image/png', image/gif',
    +   *   etc.). Defaults to 'image/png'.
    +   * @param string $transparent_color
    +   *   (Optional) The hexadecimal string representing the color to be used
    +   *   for transparency, needed for GIF images. Defaults to '#FFFFFF' (white).
    +   *
    +   * @return bool
    +   *   TRUE on success, FALSE on failure.
    +   */
    +  public function setNew($width, $height, $mime_type = 'image/png', $transparent_color = '#FFFFFF');
    +
    

    I'm not very happy with the name but am also finding it difficult to come up with a good name. At the operation level, 'canvas' seems more appropriate. Here, at the image level, createNew or createCanvas seem better.

    Another idea is to integrate this into the constructor: we only allow to pass in a source at constructor time, so we could (should!) also say that we only allow to start with a blank canvas at constructor time.

    Mime type is not really an argument at creation time, it is a save option.

    Transparent color idem and then only for gif. But to create a 'blank' canvas we should be able to specify the fill color, not only as rgb but as rgba! having a fully transparent value as default.

    Even if we do not change all of the above: NULL can be passed as transparent color when called from load or one of the (GD) operations.

  2. +++ b/core/lib/Drupal/Core/ImageToolkit/ImageToolkitOperationBase.php
    @@ -71,6 +71,15 @@ protected function getToolkit() {
    +  protected function getImage() {
    +    return $this->getToolkit()->getImage();
    +  }
    +
    

    Except for logging, I have not found any reasons, so far, for operations to have knowledge of the image. Operations should call each other via the toolkit, not via the image. (Compare how ScaleAndCrop and Scale call other operations.) So to create a convenience method for logging only seems not correct and to advocate bad practice.

  3. +++ b/core/modules/system/src/Plugin/ImageToolkit/GDToolkit.php
    @@ -121,16 +122,15 @@ protected function load() {
    +        if ($this->getImage()->setNew(imagesx($resource), imagesy($resource), $this->getMimeType(), $this->getTransparentColor())) {
    +          imagecopy($this->getResource(), $resource, 0, 0, 0, 0, imagesx($resource), imagesy($resource));
    

    Why not $this->apply('set_new')? Why use image to call a method on yourself. Especially as I think, see above, that this should not be a method on Image, or should only be called once, directly after creating an Image object.

  4. +++ b/core/modules/system/src/Plugin/ImageToolkit/Operation/gd/SetNew.php
    @@ -0,0 +1,94 @@
    + *   operation = "set_new",
    + *   label = @Translation("Set a new image"),
    

    As said, I would prefer canvas (or create_canvas) and without mime type and preferably with a fill color not a transparent color argument.

So, no real errors, but am also finding it a bit difficult to be happy with these changes. Regarding defining mime type as a save option, would mean that this issue would become dependent on #2168511: Allow to pass save options to ImageInterface::save or that we are going to refactor that part in that issue (or a follow-up). I think I prefer to do the other issue first.

What do others think? (thus leaving to NR to get more reactions)

mondrake’s picture

mondrake’s picture

FileSize
16.64 KB
21.26 KB

Thanks for review in #34, @fietserwin.

#1: changed (provisionally) to 'create_new'. Here I am mainly a) converting the current createTmp() into an image operation of its own, and b) enabling starting an image object from scratch through a core operation. A 'create_canvas' operation would mean to me getting into solid color background, gradients, patterns etc. I'd leave that to contrib. Same for the mime_type and transparent_color parameters - these are needed if we want to convert createTmp() to an operation.

#2: fine, reverted. IMHO, operations should throw exceptions rather than using the logger service, and let logging be done at toolkit level. But that can be a separate issue.

#3: I was 50/50 on that but fine, that's how ScaleAndCrop calls 'scale' and 'crop' operations, so let's be consistent.

Jelle_S’s picture

We need to limit the extensions to image extensions.

And this creates a new discussion: Why only .png, .jpg, .gif? First of all .jpeg is a valid JPEG extension too but ::getExtension() will return .jpg (in GDToolkit::getInfo()). And this leads to a next question: Why not all image types? Next type are already defined by PHP (I think in GD): IMAGETYPE_GIF, IMAGETYPE_JPEG, IMAGETYPE_JPEG2000, IMAGETYPE_PNG, IMAGETYPE_SWF, IMAGETYPE_PSD, IMAGETYPE_BMP, IMAGETYPE_WBMP, IMAGETYPE_XBM, IMAGETYPE_TIFF_II, IMAGETYPE_TIFF_MM, IMAGETYPE_IFF, IMAGETYPE_JB2, IMAGETYPE_JPC, IMAGETYPE_JP2, IMAGETYPE_JPX, IMAGETYPE_SWC, IMAGETYPE_ICO.

Maybe not .SWF, but I see no reason why popular .BMP cannot be added. Anyway, should this be a followup? Especially for .JPEG extension?

Coming from #2341251: Allow image effects to change the extension, and with #2340699: Let GDToolkit support WEBP image format in mind, we need to be careful using the PHP constants, as you mentioned for the .JPEG extension, but also because currently, PHP has no constant for WebP images, which is becoming very popular.

mondrake’s picture

#37

I would leave the discussion on supporting additional image formats to a separate issue like e.g. #2340699: Let GDToolkit support WEBP image format. Here the latest patches are about allowing creating an image (of any currently supported image type) without having to read it from a file to begin with.

fietserwin’s picture

#37: If I look at the problems we have with correctly supporting gif, I am more inclined to dropping support for gif instead of adding others. Internally, GD does not handle all formats the same, making it a nightmare to test and support all formats. E.g. internally GD works either with a truecolor image canvas or a palette color canvas and using transparency in either is not straightforwad either. In contrastt ImageMagick e.g. internally always works with 4 (16 or even 32bit) channels, handling all formats the same. Only upon saving, the format comes back into view.

BTW: We have added a change to D8 that allows the toolkits to define the supported formats, where formerly it was hardcoded into core. So it is very well possible to add support for formats in contrib, either by extending the core GD toolkit or by using another toolkit.

Jelle_S’s picture

#39: I tried that with the Picture module (adding WebP support).

It means adding a new toolkit (extending the current GD toolkit), AND for each effect (convert, scale, crop, ...) creating that again as well, because those effects define a toolkit for which they work. Maybe Toolkits should be able to define a base toolkit and inherit the effects? (but that's probably for an other issue, I don't mean to hijack this one :-) )

fietserwin’s picture

Copied comments 37 - 40 to #2340699: Let GDToolkit support WEBP image format. So, let's continue any discussion about supported image formats over there.

mondrake’s picture

FileSize
21.46 KB

Rerolled

Status: Needs review » Needs work

The last submitted patch, 42: 2063373-42.patch, failed testing.

mondrake’s picture

FileSize
21.81 KB
829 bytes

Fixed the test, the new test image 'image-test-no-transparency.gif' introduced in #2215369: Various bugs with PHP 5.5 imagerotate(), including when incorrect color indices are passed in is in any case reset to transparent via 'create_new'.

mondrake’s picture

Status: Needs work » Needs review

go bot

mondrake’s picture

Issue summary: View changes

Changed 'set_new' to 'create_new' in the issue summary.

mondrake queued 44: 2063373-44.patch for re-testing.

fietserwin’s picture

Status: Needs review » Needs work
  1. +++ b/core/modules/system/src/Plugin/ImageToolkit/GDToolkit.php
    @@ -210,58 +216,36 @@ public function parseFile() {
    +      return NULL;
         }
    -
    -    return $res;
    +    return NULL;
       }
    

    The upper return NULL can be removed.

  2. +++ b/core/modules/system/src/Plugin/ImageToolkit/Operation/gd/CreateNew.php
    @@ -0,0 +1,94 @@
    +class CreateNew extends GDImageToolkitOperationBase {
    +
    

    This class misses a validateArguments() that checks for:
    - positive width and height
    - valid and supported mime type
    - valid color

  3. +++ b/core/modules/system/src/Plugin/ImageToolkit/Operation/gd/CreateNew.php
    @@ -0,0 +1,94 @@
    +      default:
    +        return FALSE;
    +
    

    When the validation has been added, this can be removed.

  4. +++ b/core/modules/system/src/Plugin/ImageToolkit/Operation/gd/Crop.php
    @@ -80,17 +80,24 @@ protected function validateArguments(array $arguments) {
    +    if ($this->getToolkit()->apply('create_new', $data)) {
    +      if (!imagecopyresampled($this->getToolkit()->getResource(), $original_resource, 0, 0, $arguments['x'], $arguments['y'], $arguments['width'], $arguments['height'], $arguments['width'], $arguments['height'])) {
    +        return FALSE;
    +      }
    +      imagedestroy($original_resource);
    +      return TRUE;
         }
    +    return FALSE;
       }
    

    return false - true - false. The 1st if checks for failure, the 2nd for success. For me, this may be done in 1 if that checks for success.

  5. +++ b/core/modules/system/src/Plugin/ImageToolkit/Operation/gd/Resize.php
    @@ -59,17 +59,24 @@ protected function validateArguments(array $arguments) {
    +    if ($this->getToolkit()->apply('create_new', $data)) {
    +      if (!imagecopyresampled($this->getToolkit()->getResource(), $original_resource, 0, 0, 0, 0, $arguments['width'], $arguments['height'], imagesx($original_resource), imagesy($original_resource))) {
    +        return FALSE;
    +      }
    +      imagedestroy($original_resource);
    +      return TRUE;
         }
    +    return FALSE;
       }
    

    same as above.

  6. +++ b/core/modules/system/src/Tests/Image/ToolkitGdTest.php
    @@ -125,6 +125,12 @@ function testManipulations() {
    +      'create_new' => array(
    +        'function' => 'create_new',
    +        'arguments' => array('width' => 20, 'height' => 10, 'transparent_color' => '#ffff00'), // Yellow color for transparency of GIF file.
    +        'width' => 20,
    +        'height' => 10,
    +      ),
           'resize' => array(
    

    Does it makes (much) sense to test create_new as a real operation and have all test images converted to the same canvas?

    Especially as further down we have separate tests for create_new.

    IMO: remove the operation with the test images and extract the latter foreach (that specifically and only tests create_new) into its own test method.

Even when all my remarks are processed, this patch still buggers me, but I am not sure why.
- It may be due to another running issue that prefers extension(/format) over mime type,so we are getting a non-consistent use of image type, mime type and extension in the gd toolkit. (#2330899: Allow image effects to change the MIME type + extension, add a "convert" image effect)
- Can we create a non transparant gif from scratch?
- Why is (GD) image processing so storage format based?

But this should not hold this issue. This (and the other mentioned issue) reveal deeper lying problems with the GD toolkit that we only discover here, now we are going beyond very simple processing like a crop or resize. So, I think I will open another issue for that.

fietserwin’s picture

mondrake’s picture

Assigned: mondrake » Unassigned
Priority: Major » Normal
FileSize
14.85 KB
22.53 KB

#48 all done, thanks for the review.

Added also tests for failures of CreateNew, and changed the 'mime_type' argument to an 'extension' argument in line with what's being done in #2330899: Allow image effects to change the MIME type + extension, add a "convert" image effect.

mondrake’s picture

Status: Needs work » Needs review

go bot

mondrake’s picture

FileSize
2 KB
21.85 KB

Couple of nits, sorry

fietserwin’s picture

Status: Needs review » Needs work

(Sorry for the late reaction, the issue got out of sight)

In principle, this is good to go, but to keep in line with the mentioned issue #2330899: Allow image effects to change the MIME type + extension, add a "convert" image effect, I propose to introduce a public method extensionToImageType() on GdToolkit. That method will already have 2 usages and the method supportedTypes() can remain protected. (We can wait for the other issue, or do it here as well and see which one gets in first.)

mondrake’s picture

Status: Needs work » Needs review
FileSize
3.7 KB
23.48 KB
fietserwin’s picture

Status: Needs review » Reviewed & tested by the community

Exactly what I was looking for. Thanks.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs change record

This is looking good but we need a change record for this - or update the record(s) for the removal of image_gd_create_tmp

fietserwin’s picture

Status: Needs work » Reviewed & tested by the community
Issue tags: -Needs change record
alexpott’s picture

Thanks for creating the CR. The patch looks good - committed 7189de5 and pushed to 8.0.x. Thanks!

  • alexpott committed 7189de5 on 8.0.x
    Issue #2063373 by mondrake, claudiu.cristea, larowlan, arunvs: Fixed...
alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Status: Fixed » Closed (fixed)

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