Follow-up to #2262863: Add srcset to template_preprocess_image

Problem/Motivation

  • The new picture html tag allows to define alternatives based on mime type (image format) using the type attribute. To render this attribute, we need to be able to determine the mime type of a derivative without generating it.
  • To be able to generate derivatives in multiple output formats, we need an image effect that can change the format an image is saved in.

Proposed resolution

  • Add ImageEffectInterface::getDerivativeMimeType(&$mime_type)
  • Add ImageEffectInterface::getDerivativeExtension(&$extension)
  • Now that ImageStyle supports changing the MIME type & extension, add a "convert" ImageEffect

User interface changes

None

API changes

  • Adds ImageEffectInterface::getDerivativeMimeType(&$mime_type)
  • Adds ImageStyleInterface::getDerivativeMimeType(&$mime_type)
  • Adds ImageEffectInterface::getDerivativeExtension(&$extension)
  • Adds ImageStyleInterface::getDerivativeExtension(&$extension)
  • Adds ImageInterface::convert($format)
  • (Adds \Drupal\image\Plugin\ImageEffect\ConvertImageEffect)
  • (Adds \Drupal\system\Plugin\ImageToolkit\Operation\gd\Convert)
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

attiks’s picture

Jelle_S’s picture

FileSize
7.1 KB

Patch with tests for the new method.

attiks’s picture

Status: Needs review » Reviewed & tested by the community

Let's get this committed so we can resume work on picture

webchick’s picture

Status: Reviewed & tested by the community » Needs review
+++ b/core/modules/image/src/ImageEffectBase.php
@@ -77,6 +77,12 @@ public function transformDimensions(array &$dimensions) {
+  public function transformMimeType(&$mime_type) {
+  }

Hm. Why does this not have a function body? It does not in fact seem to be transforming any mime types? :)

Empty methods on the interfaces make sense, but this is a base class.

attiks’s picture

Because all image effect provided by core don't do anything with the MIME type, the same is with TransformDimensions and DesaturateImageEffect.

If contrib wants to add an Image Effect that transforms jpg to webP, it can do so.

RainbowArray’s picture

Yes, TransformDimensions is empty in DesaturateImageEffect, but DesaturateImageEffect extends ImageEffectBase, where TransformDimensions is defined. Although I admittedly may be confused because TransformDimensions in ImageEffectBase is defined as:

public function transformDimensions(array &$dimensions) {
    $dimensions['width'] = $dimensions['height'] = NULL;
}

That doesn't make a ton of sense to me, so what do I know.

I just don't understand why TransformMimeType isn't defined as changing one MIME type into another one in ImageEffectBase.

Jelle_S’s picture

Yes, TransformDimensions is empty in DesaturateImageEffect, but DesaturateImageEffect extends ImageEffectBase, where TransformDimensions is defined. Although I admittedly may be confused because TransformDimensions in ImageEffectBase is defined as:

public function transformDimensions(array &$dimensions) {
$dimensions['width'] = $dimensions['height'] = NULL;
}
That doesn't make a ton of sense to me, so what do I know.

IMHO that doesn't make sense either. Currenty every single image effect provided by Core overwrites the transformDimensions method, meaning that image effects that don't affect the dimensions of an image overwrite it with an empty method so that $dimensions['width'] and $dimensions['height'] aren't set to NULL. To me it would seem cleaner if ImageEffectBase would just define transformDimensions as an empty method so that only image effects that affect the image dimensions have to implement it. (This is the way transformMimeType is implemented in this patch)

I just don't understand why TransformMimeType isn't defined as changing one MIME type into another one in ImageEffectBase.

Because the image effects provided by core don't actually transform the MIME type. If you pass them a JPEG, you will get a JPEG back.
Image effects that do transform it, should overwrite the TransformMimeType method of ImageEffectBase.

attiks’s picture

Status: Needs review » Reviewed & tested by the community

Assuming all is explained well

alexpott’s picture

Status: Reviewed & tested by the community » Needs work

We need support for the mime type and we need to be able to determine the mime type of a derivative without generating it.

Why? Yes I can read the parent issue - but let's save everyone else the work of doing this.

Also I feel these transform* methods are poorly documented and named - they are about telling you what the applying the image effect will do WITHOUT applying it and interrogating the resulting image. The method names make it sound like they are doing the transformation which is not true. Let's do better.

attiks’s picture

Issue summary: View changes
attiks’s picture

#9 Good point about the naming of those functions, but they are based on the existing transformDiminsions.

What about getDerivativeMimeType?

Jelle_S’s picture

Status: Needs work » Needs review
Related issues: +#2341251: Allow image effects to change the extension
FileSize
20.98 KB
17.61 KB

Marked #2341251: Allow image effects to change the extension as a duplicate, since changing the MIME type of an image will also change its extension. Merged the two patches together and changed the method names to getDerivative*. I also adjusted the comments a bit.

RainbowArray’s picture

I took a close look through how the patch in #12 merged the patch in #2 with patch #6 in #2341251: Allow image effects to change the extension. It looks like a sensible merging of the two. Hopefully the improved comments and function names help to clear things up. I'm a little more clear at least.

On #2063373: Cannot save image created from scratch in comment #8, attiks asked if it made sense to get in #2063373: Cannot save image created from scratch first, but that was never resolved. That discussion then spiraled off to #2340699: Let GDToolkit support WEBP image format.

Since this is now the last blocker for getting Picturefill updated, could somebody clarify if getting #2063373: Cannot save image created from scratch in first is a nice to have or a must have? If it's a nice to have, but not necessary, let's look at moving this issue to RTBC so we can get it in and then get Picturefill updated. If there is a better way to create images from scratch, great, but I'm not sure that is a necessity to hold up the Picturefill update. It seems like that could maybe be handled in a follow-up.

attiks’s picture

#13 I think it is a nice to have, but it should not block the picture patches

mondrake’s picture

#14 Agree, let's keep the two independent.

attiks’s picture

FileSize
21.45 KB

New patch fixing getPathToken

alexpott’s picture

#16 no interdiff?

attiks’s picture

FileSize
2.3 KB

#17 Here you go

Jelle_S’s picture

FileSize
21.45 KB
1.04 KB

We forgot to rename some test methods. New patch fixes that.

attiks’s picture

Status: Needs review » Reviewed & tested by the community

Since mark tested this and it was RTBC before

attiks’s picture

Status: Reviewed & tested by the community » Needs work

We need to have a closer look at the tests

Jelle_S’s picture

Status: Needs work » Needs review
FileSize
26.44 KB
10.76 KB

New patch with added tests for ImageStyle.

Jelle_S’s picture

FileSize
26.44 KB
729 bytes

In the previous patch one assertion was still commented out (forgot to remove the comment slashes after testing). New (correct) patch.

mondrake’s picture

+++ b/core/modules/image/src/ImageStyleInterface.php
@@ -171,4 +190,30 @@ public function addImageEffect(array $configuration);
+  /**
+   * Returns the image effect plugin manager.
+   *
+   * @return \Drupal\Component\Plugin\PluginManagerInterface
+   *   The image effect plugin manager.
+   */
+  public function getImageEffectPluginManager();
+
+  /**
+   * Gets the Drupal private key.
+   *
+   * @return string
+   *   The Drupal private key.
+   */
+  public function getPrivateKey();
+
+  /**
+   * Gets a salt useful for hardening against SQL injection.
+   *
+   * @return string
+   *   A salt based on information in settings.php, not in the database.
+   *
+   * @throws \RuntimeException
+   */
+  public function getHashSalt();
+

Do we really need these methods public and on the interface? If you need them for PHPUnit test, AFAIK, you can still mock them if they are protected.

Jelle_S’s picture

Assigned: Unassigned » Jelle_S
Status: Needs review » Needs work

Do we really need these methods public and on the interface? If you need them for PHPUnit test, AFAIK, you can still mock them if they are protected.

Oh, I thought they needed to be public to be able to mock them. I'll change it :-)

fietserwin’s picture

Assigned: Jelle_S » Unassigned
Issue tags: +Needs issue summary update

We need support for the mime type and we need to be able to determine the mime type of a derivative without generating it.

I don't see the need for this and no I did not read the parent issue, so please explain the why in this issue summary. Explicitly explain the use case where you need it without generating it.

- For any use case where we need this info during or after generating it, issue #2168511: Allow to pass save options to ImageInterface::save is a far more generic solution.
- If there 's a use case for need of this info before generating it, like with the dimensions, I would like to propose to make the current transformDimensions into something more generic like transformImageInfo(). This so because I am afraid that in the next version of Chrome we can get 10% performance improvement if we add an attribute to the img tag that declares if an image contains transparency or not and so we add a method transFormTransparency(). 2 weeks later, Firefox announces support for feature Y that needs to know how many bits the color channels have and we add a method transformColorChannelDepthMeasuredInBits().

My proposal stabilizes the API methods, but still allows to pass information around not yet foreseen.
If needed, transformImageInfo() should be well documented with the info it currently expects and the base method should, indeed, alter nothing. Note that this is in contrast with the current implementation of transformDimensions().

Wim Leers’s picture

#26.a: it's right there, ever since alexpott asked it in #9, it was added in #10: https://www.drupal.org/node/2330899/revisions/view/7648927/7648969
#26.b: No, because then you've made things *less* explicit. The structure of the array you'd be passing around then would be a D7-style "structured array" that we cannot possibly validate. In other words: terrible DX.


  1. +++ b/core/lib/Drupal/Core/Image/ImageInterface.php
    @@ -149,6 +149,14 @@ public function scale($width, $height = NULL, $upscale = FALSE);
    +   *   The format to convert to (e.g. 'jpeg', 'png').
    

    Is it 'jpeg' or 'jpg'? Does it matter? Where is this used? The docs don't tell.

  2. +++ b/core/modules/image/src/Controller/ImageStyleDownloadController.php
    @@ -130,8 +130,13 @@ public function deliver(Request $request, $scheme, ImageStyleInterface $image_st
    +      // Remove the extension to check if the image style added one.
    +      $path_info = pathinfo($image_uri);
    +      $image_uri = $path_info['dirname'] . DIRECTORY_SEPARATOR . $path_info['filename'];
    +      if (!file_exists($image_uri)) {
    

    What? Not clear at all.

    Also: this is a *nested* file_exists() call. This effectively doubles the IO.

  3. +++ b/core/modules/image/src/Entity/ImageStyle.php
    @@ -180,6 +180,15 @@ public function buildUri($uri) {
    +    // Check the extension of the derivative.
    +    $extension = pathinfo($path, PATHINFO_EXTENSION);
    +    $original_extension = $extension;
    +    $this->getDerivativeExtension($extension);
    +    if ($original_extension != $extension) {
    +      $path .= '.' . $extension;
    +    }
    

    1) the comment says "check", but it should be "append"
    2) why append an extension, why not *change* it?
    3) why no strict comparisoin?

  4. +++ b/core/modules/image/src/Entity/ImageStyle.php
    @@ -310,9 +319,35 @@ public function transformDimensions(array &$dimensions) {
    +    // Check the extension of the derivative.
    +    $extension = pathinfo($uri, PATHINFO_EXTENSION);
    +    $original_extension = $extension;
    +    $this->getDerivativeExtension($extension);
    +    if ($original_extension != $extension) {
    +      $uri .= '.' . $extension;
    +    }
    

    Duplicate of the code above. Let's put that in a protected helper method then.
    That will also allow for clearer documentation.

  5. +++ b/core/modules/image/src/ImageEffectInterface.php
    @@ -44,6 +44,22 @@ public function applyEffect(ImageInterface $image);
    +   *   The MIME type to be modified.
    

    … to be set to the derivative's MIME type

  6. +++ b/core/modules/image/src/ImageEffectInterface.php
    @@ -44,6 +44,22 @@ public function applyEffect(ImageInterface $image);
    +   *   Extension to be modified.
    

    s/Extension/The file extension/

    Plus analogous to the above.

  7. +++ b/core/modules/image/src/ImageStyleInterface.php
    @@ -132,6 +132,25 @@ public function createDerivative($original_uri, $derivative_uri);
    +   *   The MIME type to be modified.
    

    Again.

  8. +++ b/core/modules/image/src/ImageStyleInterface.php
    @@ -132,6 +132,25 @@ public function createDerivative($original_uri, $derivative_uri);
    +  /**
    +   * Determines the extension of the derivative without generating it.
    +   *
    +   * Stores the extension of this image style into $extension.
    +   *
    +   * @param string $extension
    +   *   String passed by reference. Implementations have to store the
    +   *   resulting extension, without the leading dot.
    +   */
    +  public function getDerivativeExtension(&$extension);
    +
    

    Completely different documentation compared to the above. Let's make it consistent.

  9. +++ b/core/modules/image/src/ImageStyleInterface.php
    @@ -171,4 +190,30 @@ public function addImageEffect(array $configuration);
    +  public function getImageEffectPluginManager();
    ...
    +  public function getPrivateKey();
    ...
    +  public function getHashSalt();
    

    Why do these need to be on the interface?

  10. +++ b/core/modules/image/src/Plugin/ImageEffect/ConvertImageEffect.php
    @@ -0,0 +1,106 @@
    + * Converts an image resource.
    

    First time I see "resource"?

  11. +++ b/core/modules/image/src/Plugin/ImageEffect/ConvertImageEffect.php
    @@ -0,0 +1,106 @@
    +    // Guess needs a filename.
    

    ? Please improve this line.

  12. +++ b/core/modules/image/src/Tests/ImageStyleTest.php
    @@ -0,0 +1,224 @@
    + * @group Drupal
    

    We did this for a while, we no longer do this. Can be removed.

  13. +++ b/core/modules/image/src/Tests/ImageStyleTest.php
    @@ -0,0 +1,224 @@
    +  protected $entityTypeId;
    +
    +
    +  /**
    

    Nit: Should be only one newline.

  14. +++ b/core/modules/image/src/Tests/ImageStyleTest.php
    @@ -0,0 +1,224 @@
    +   *   The image effect id.
    

    Nit: s/id/ID/

  15. +++ b/core/modules/image/src/Tests/ImageStyleTest.php
    @@ -0,0 +1,224 @@
    +    include_once DRUPAL_ROOT . '/core/includes/file.inc';
    

    This will cause problems for other tests, please don't include anything.

  16. +++ b/core/modules/image/src/Tests/ImageStyleTest.php
    @@ -0,0 +1,224 @@
    +    $this->assertEquals($image_style->buildUri('public://test.jpeg'), 'public://styles/' . $image_style->id() . '/public/test.jpeg');
    +
    +  }
    ...
    +    $this->assertEquals(substr(Crypt::hmacBase64($image_style->id() . ':' . 'public://test.jpeg', $private_key . $hash_salt), 0, 8), $image_style->getPathToken('public://test.jpeg'));
    +
    +  }
    

    Extraneous newline.

  17. +++ b/core/modules/image/src/Tests/ImageStyleTest.php
    @@ -0,0 +1,224 @@
    +    // Assert the extension has been added to the uri before creating the token.
    

    Nit: s/uri/URI/.

  18. +++ b/core/modules/system/src/Plugin/ImageToolkit/Operation/gd/Convert.php
    @@ -0,0 +1,84 @@
    +/**
    + * Defines GD2 convert operation.
    + *
    + * @ImageToolkitOperation(
    + *   id = "gd_convert",
    + *   toolkit = "gd",
    + *   operation = "convert",
    + *   label = @Translation("Convert"),
    + *   description = @Translation("Converts an image to the given format.")
    + * )
    + */
    +class Convert extends GDImageToolkitOperationBase {
    

    This feature addition is not captured at all in the issue title!

Jelle_S’s picture

1. Is it 'jpeg' or 'jpg'?
jpeg, jpg or jpe for Drupal Core (see next answers).
Does it matter?
Depends on the implementation of the Convert operation of the Toolkit. In the case of Drupal Core (GDToolkit) it can be jpeg, jpg or jpe (see \Drupal\system\Plugin\ImageToolkit\Operation\gd\Convert, which is also added by this patch)
Where is it used?
In the implementation of the Convert operation of the current Toolkit (again, GDToolkit for Drupal Core).
The docs don't tell.
Not sure how to document that in a comment?

2. buildUri adds the extension to the end (test.jpeg -> test.jpeg.png), this means that when the derivative is created, it will look for a source file with the name test.jpeg.png, which won't exist, so we strip the last extension and then it will look for test.jpeg, which will exist. That's also why there are two file_exists calls, but the second one will only get called if the image style converts the mime type.

3. See above explanation

15. file.inc is included because of this code in ImageStyle::buildUri():

$scheme = file_uri_scheme($uri);
if ($scheme) {
  $path = file_uri_target($uri);
}
else {
  $path = $uri;
  $scheme = file_default_scheme();
}

Can we Unit Test this method without including it? Any suggestions?

18. Since ImageStyle now supports changing the MIME type & extension, we might as well add a Convert operation to the GDToolkit so that we can create an ImageEffect to convert between image types in Drupal Core. It also makes (manual) testing easier.

Jelle_S’s picture

Issue summary: View changes
Jelle_S’s picture

Issue summary: View changes
Wim Leers’s picture

Title: Add MIME type support for image styles » Allow image effects to change the MIME type + extension, add a "convert" image effect
Issue tags: -Needs tests, -Needs issue summary update
  • #28.1: Add this explanation to the docblock?
  • #28.2: But is there a good reason to do it this way? If there is, please document. If there isn't, please fix it.
  • #28.15: Yes, you can copy it locally, in the scope of this unit test. Or, alternatively, add fileUriScheme(), fileUriTarget() and fileDefaultScheme() wrapper methods on the class that call the procedural code, they can then be removed later on. Plenty of classes do this: e.g. drupalRender() which calls drupal_render().
  • #28.18: okay, but then please update the issue title accordingly. Here's my attempt.

#29, #30: thanks, that helps.

Jelle_S’s picture

But is there a good reason to do it this way? If there is, please document. If there isn't, please fix it.

Yes:

  1. Name clashes: If we convert test.gif to test.jpeg and test.png to test.jpeg using the same ImageStyle we have a name clash and we can't assume the two images will be the same once converted
  2. We need to be able to find the original file to create the derivative. Using the above example: if we get test.jpeg in ImageStyleDownloadController::deliver and it doesn't exist, we won't know if we have to look for test.gif or test.png.
mondrake’s picture

#28.15: you can take cues on wrapping procedural code in a protected method here from the Image class. FYI at least file_uri_scheme() is being replaced by a OO equivalent in #2050759: Move drupal_chmod and other code in file.inc to a Drupal\Core\File\FileSystem class, but that's WIP

fietserwin’s picture

Issue summary: View changes

Well, I didn't know the details of the new picture element, so I rephrased it a bit more explicitly.

  1. The patch seems to accept uppercase formats in the effect UI, but supportedExtensions() only returns lower case strings (not documented). So the validateArgumetns will fail when upercase formats are passed. This raise the question:
  2. Do we want to be able to explicitly change the extension, introducing 2 more methods, or does it suffice to allow the user to only select an image format and have the system define the extension it uses for that? The latter being my preference, this would simplify the code in a number of places.
attiks’s picture

#34Th extensions are coming from the toolkit, it uses \Drupal::service('image.toolkit.manager')->getDefaultToolkit()->getSupportedExtensions();

Jelle_S’s picture

Status: Needs work » Needs review
FileSize
30.18 KB
15.47 KB

I think I got all remarks from #27, except #27.10: That docblock is a copy-paste from the other ImageEffects (CropImageEffect, DesaturateImageEffect, ResizeImageEffect, RotateImageEffect, ScaleAndCropImageEffect, ScaleImageEffect)

Jelle_S’s picture

FYI: patch still applies. Anyone up for a review?

Dave Reid’s picture

Issue tags: +Media Initiative
fietserwin’s picture

Status: Needs review » Needs work

This is only a review of the image/imagetoolkit part. The image style/effect part follows.

  1. +++ b/core/lib/Drupal/Core/Image/ImageInterface.php
    @@ -149,6 +149,17 @@ public function scale($width, $height = NULL, $upscale = FALSE);
       /**
    +   * Converts an image to a specified format.
    +   *
    +   * @param string $format
    +   *   The format to convert to (e.g. 'jpeg', 'png'). Allowed values depend on
    +   *   the implementation of the convert operation of the image toolkit. For a
    +   *   working example, see
    +   *   \Drupal\system\Plugin\ImageToolkit\Operation\gd\Convert.
    +   */
    +  public function convert($format);
    +
    

    Missing @return.

    Moreover, the format is not changed by the this convert operation but on saving the image. So, strictly speaking, something like "Instructs the toolkit to save the image in the specified format" is better.

  2. +++ b/core/modules/system/src/Plugin/ImageToolkit/Operation/gd/Convert.php
    @@ -0,0 +1,84 @@
    + *   description = @Translation("Converts an image to the given format.")
    

    See remark above.

  3. +++ b/core/modules/system/src/Plugin/ImageToolkit/Operation/gd/Convert.php
    @@ -0,0 +1,84 @@
    +    switch ($arguments['format']) {
    

    $format is a supported extension (see validateArguments()), thus it is easier to just foreach through all supported types and use image_type_to_extension() (as in GdToolkit::geSupportedExtensions) to get the type.

  4. +++ b/core/modules/system/src/Plugin/ImageToolkit/Operation/gd/Convert.php
    @@ -0,0 +1,84 @@
    +        if (defined($constant)) {
    +          $type = constant($constant);
    +        }
    +        break;
    +    }
    +
    +    if (!$type) {
    +      return FALSE;
    +    }
    +
    

    validateArguments() assures that this cannot occur. (Unless you have a broken PHP installation, in that case it is better to fail fast and hard). So it is better to remove the checks (if you don't want to do it as suggested above).

  5. +++ b/core/modules/system/src/Plugin/ImageToolkit/Operation/gd/Convert.php
    @@ -0,0 +1,84 @@
    +    $res = $this->getToolkit()->createTmp($type, $this->getToolkit()->getWidth(), $this->getToolkit()->getHeight());
    +    if (!imagecopyresampled($res, $this->getToolkit()->getResource(), 0, 0, 0, 0, $this->getToolkit()->getWidth(), $this->getToolkit()->getHeight(), $this->getToolkit()->getWidth(), $this->getToolkit()->getHeight())) {
    +      return FALSE;
    +    }
    +
    +    imagedestroy($this->getToolkit()->getResource());
    +
    +    $this->getToolkit()->setType($type);
    +    // Update image object.
    +    $this->getToolkit()->setResource($res);
    +
    

    Only the line setting the type is correct. The other lines do nothing but duplicating the existing resource:

    - not gif -> gif: no transparent color was set and no transparent color will be set. If the original contained transparency and we want to retain that we must allocate a color to represent transparent and set all pixels that are currently (almost) transparent to that color.
    - gif -> not gif: a transparent color could be set but will be lost as the new format does not work with transparency or works with a transparency channel, in which case we should make all pixels having the the color defined as representing transparent, fully transparent.
    - gif -> gif: the transparent color will be copied to the new resource (exact duplicate): thus resulting in a no-op.

    I think it is OK to loose transparency when coming from or going to gif, we do not want to make it to complex here.

  6. +++ b/core/modules/system/src/Tests/Image/ToolkitGdTest.php
    @@ -174,6 +174,12 @@ function testManipulations() {
    +      'convert' => array(
    +        'function' => 'convert',
    +        'width' => 40,
    +        'height' => 20,
    +        'arguments' => array('format' => 'jpeg'),
    +      ),
         );
    

    So, we are only testing conversion to jpeg? The interesting cases are formats that support transparency, Please add these as well and we will notice that we have to better describe what happens with transparency with this operation (and tests are a way to specify behavior).

attiks’s picture

Status: Needs work » Needs review
FileSize
29.94 KB
2.54 KB

Comments fixed and a bit of code, will discuss tests with Jelle

fietserwin’s picture

Status: Needs review » Needs work

And the review of the imagestyle/effect part:

  1. +++ b/core/modules/image/src/Controller/ImageStyleDownloadController.php
    @@ -130,8 +130,16 @@ public function deliver(Request $request, $scheme, ImageStyleInterface $image_st
    +      $image_uri = $path_info['dirname'] . DIRECTORY_SEPARATOR . $path_info['filename'];
    +      if (!file_exists($image_uri)) {
    +        $this->logger->notice('Source image at %source_image_path not found while trying to generate derivative image at %derivative_path.',  array('%source_image_path' => $image_uri, '%derivative_path' => $derivative_uri));
    +        return new Response($this->t('Error generating image, missing source file.'), 404);
    +      }
         }
    

    This will invalidate the error message in case there was no convert operation. Something like Source image at sample not found instead of Source image at sample.png not found.

  2. +++ b/core/modules/image/src/Entity/ImageStyle.php
    @@ -310,9 +310,27 @@ public function transformDimensions(array &$dimensions) {
       /**
        * {@inheritdoc}
        */
    +  public function getDerivativeMimeType(&$mime_type) {
    +    foreach ($this->getEffects() as $effect) {
    +      $effect->getDerivativeMimeType($mime_type);
    +    }
    +  }
    +
    +  /**
    +   * {@inheritdoc}
    +   */
    +  public function getDerivativeExtension(&$extension) {
    +    foreach ($this->getEffects() as $effect) {
    +      $effect->getDerivativeExtension($extension);
    +    }
    +  }
    +
    

    I don't see the need for 2 methods here and on ImageEffectInterface. Extension and mime-type are interchangeable. We can go from one to the other using ExtensionMimeTypeGuesser or MimeTypeExtensionGuesser.

    The only case where it can be useful to distinguish is to give the user the option to define the extension (JPG or jpg or Jpeg), but IMO we don't have to provide that feature here. Being able to change (and query upfront) the image format is the goal of this issue. So let's go for a specific not configurable extension per mime-type. In fact the GdConvert operation also does so.

    Next, we have to decide what to store in the ImageEffect: mime-type or extension?

    - Mime-type: change the operation to accept a mime-type instead of extension.
    - Extension: change the picture module (or other users of this new feature) to do the mime type lookup based on extension, but do not bother image effects with that task.

    FWIW: I Prefer to use extension in the image system, because we already have an getAllowedExtensions() method on toolkits over there. So a convert operation should have a parameter in the set of allowed extensions. That is more logical than introducing yet another concept in the toolkit layer to represent image format.

  3. +++ b/core/modules/image/src/Plugin/ImageEffect/ConvertImageEffect.php
    @@ -0,0 +1,107 @@
    +    $mime_type = \Drupal::service('file.mime_type.guesser.extension')->guess('test.' . $this->configuration['format']);
    

    This is what I don't want in the image system but in the picture module. 1 line of code in picture can replace 2 methods on interfaces and about 60 lines of code in the image system!

Setting back to NW, though the tests may run for the changes posted in between.

fietserwin’s picture

This is a remnark on 1 of the changes in #40

+++ b/core/modules/system/src/Plugin/ImageToolkit/Operation/gd/Convert.php
@@ -0,0 +1,69 @@
+    // Make sure we have an image type.
+    $type = $this->getToolkit()->getType();
+    if (!$type) {
+      return FALSE;
+    }
+
+    $res = $this->getToolkit()->createTmp($type, $this->getToolkit()->getWidth(), $this->getToolkit()->getHeight());
+    if (!imagecopyresampled($res, $this->getToolkit()->getResource(), 0, 0, 0, 0, $this->getToolkit()->getWidth(), $this->getToolkit()->getHeight(), $this->getToolkit()->getWidth(), $this->getToolkit()->getHeight())) {
+      return FALSE;
+    }
+
+    imagedestroy($this->getToolkit()->getResource());
+
+    // Update image object.

Sorry, I was not clear with my remark. what I meant is that the execute() method should become something like:

  protected function execute(array $arguments = array()) {
    foreach (GDToolkit::supportedTypes() as $type) {
      if (image_type_to_extension($type) === $arguments['format']) {
        break;
      }
    }
    $this->getToolkit()->setType($type);

    return TRUE;
  }
attiks’s picture

#41

1/ Will fix it

2/ Adding both methods will make it easier to use the class, otherwise every module wanting use this class has to replicate code. The other reason is that there's is - like you said - that there's is no 1 on 1 mapping between extensions and MIME types.

attiks’s picture

#42 No I'm lost, you want to remove this as well?

    $res = $this->getToolkit()->createTmp($type, $this->getToolkit()->getWidth(), $this->getToolkit()->getHeight());
    if (!imagecopyresampled($res, $this->getToolkit()->getResource(), 0, 0, 0, 0, $this->getToolkit()->getWidth(), $this->getToolkit()->getHeight(), $this->getToolkit()->getWidth(), $this->getToolkit()->getHeight())) {
      return FALSE;
    }

    imagedestroy($this->getToolkit()->getResource());

The last submitted patch, 40: i2330899-39.patch, failed testing.

Jelle_S’s picture

#42, #44:
That code can't be removed (if you want the convert operation to keep functioning):
createTmp creates a new resource of the correct type (e.g. for a conversion of gif to jpeg it creates an empty jpeg with the correct dimensions, for png and gifs it also determines the transparency, as you described in #39/6). Then we copy the old resource to the new (which can be seen as the actual conversion) and set the new resource as the toolkit's resource. The setType method is merely informative and does not do any conversion by itself. We set it here so it is in sync with the actual resource that is set on the toolkit.

attiks’s picture

Status: Needs work » Needs review
FileSize
32.58 KB
6.45 KB

New patch, discussed with Jelle

We added supportedTypes to the interface and made it public.

attiks’s picture

FileSize
33.31 KB
7.67 KB

Convert tests added for gif and png

Interdiff is against #36

mondrake’s picture

#47 Hm. Fine to make that method public in GDToolkit so it can be used by GD operations, but why also on the interface? Other toolkits may decide not to use the PHP constants internally (see for instance this sandbox with a test conversion of Imagemagick). We already have getResource(), setResource(), getType() and setType() public in GDToolkit but not on the interface, can we follow the same pattern?

mondrake’s picture

Sorry, I must have xposted with #48 and the patch file disappeared. I can't seem to be able to restore it :(

attiks’s picture

FileSize
32.4 KB
6.77 KB

#49 You're right new patch, also fixing #50

The last submitted patch, 47: i2330899-47.patch, failed testing.

fietserwin’s picture

Status: Needs review » Needs work
FileSize
33.31 KB

RE #43.2:
We are talking either:
- 1 line in Picture
Or
- 8 lines in ImageStyleInterface
- 8 lines in ImageEffectInterface
- 9 lines in ImageStyle
- 6 lines in ImageEffectBase
- 9 lines in ConvertImageEffect.php
- 22 lines in ImageStyleTest
For a total of 62 lines for a feature that on top of this is also in the wrong place: the image system should not be concerned with mime types, see issue #2257163: Restrict image system to image processing.

Oh and yes there is a 1-to-n mapping between mime type and extensions, but we can leave that task up to the mime type guesser (as this patch also does).

So, PLEASE, let's keep the interfaces clean and lean: NW

RE #46:
- No, createTmp is there to create true color resources regardless the type. thus also on opening gif images we get a true color resource instead of a color palette resource. True color resources are in essence image format free. I have my doubts about the imagesavealpha() only called for png, but that would be a bug that you cannot solve here. The place of the convert image effect in an image style should not have influence on the end result.

- setType() should be called by createTmp() (this is a bug that you can solve here.

So, OK, for now let that call to createTmp in, I will file a follow-up to make GD less type specific.

RE #47:
- supportedTypes() cannot be in the interface as IMAGETYPE is a gd specific thing and will restrict all toolkits to support only those formats that GD can support (read: no webp support): NW. Even for PHP 5.5. GD webp support we might have to revisit this whole type thing, as GD forgot to introduce an IMAGETYPE constant for webp: NW

So you can make it public to allow access from an operation, because an operation knows which class it works for. BTW: the PHP doc states: Static properties cannot be accessed through the object using the arrow operator ->..: NW

fietserwin’s picture

I see a file attached to my comment in #53 which I didn't post. d.o.seems a bit confused...

attiks’s picture

#54 To avoid further confusion, can you reroll the patch as you described?

The last submitted patch, 48: i2330899-48.patch, failed testing.

mondrake’s picture

One thought - sorry if it comes late.

In this patch we have a concept of 'format' which makes me to struggle with. Why? Because we already have the concepts of 'extension' and of 'MIME type' in the image system (let alone the GD internal 'type' represented by IMAGETYPE_* constants).

Everywhere in the patch it seems to me that the 'format' equals the 'extension' - this is what is presented in the image effect form, stored in the effect configuration, and what is passed from the effect to the toolkit operation.

Now, if this gets in, with the GDToolkit in its current status, our 'format' dropdown in the effect form will have

  • JPEG
  • GIF
  • PNG

which is OK, it can match a concept of 'format'.

But if, later on, #2340699: Let GDToolkit support WEBP image format gets in, the 'format' dropdown will have sth like

  • JPEG
  • JPG
  • JPE
  • GIF
  • PNG
  • WBMP
  • WEBP
  • XBM

and here the concept of 'format' would blur... one may say that JPEG, JPG and JPE are the same 'format' with different 'extensions'.

So, to cut short: my suggestion is to just have the concept of 'extension' internally throughout the code (names of variables and arguments), with maybe the convert effect to present in the UI a field titled 'Derivative image file extension' (or the likes) where we have now 'Format'.

Then, the cherry on the cake could be to have in the dropdown displayed to users both the extension and the MIME type, à la

  • JPEG (image/jpeg)
  • JPG (image/jpeg)
  • JPE (image/jpeg)
  • GIF (image/gif)
  • PNG (image/png)
  • ...

Thoughts?

attiks’s picture

#57 Good thinking, I'll change the naming to extension.

The last submitted patch, 51: i2330899-51.patch, failed testing.

attiks’s picture

Status: Needs work » Needs review
FileSize
12.96 KB
33.28 KB

renaming format to extension

Jelle_S’s picture

RE #53

setType() should be called by createTmp() (this is a bug that you can solve here.

I don't think it should. createTmp() just creates the resource, it doesn't change the resource the toolkit instance is working with, so it shouldn't change the type based on the created resource.

Status: Needs review » Needs work

The last submitted patch, 60: i2330899-60.patch, failed testing.

attiks’s picture

Strange error: PHP Fatal error: Call to a member function warning() on a non-object in /var/lib/drupaltestbot/sites/default/files/checkout/core/lib/Drupal/Core/ImageToolkit/ImageToolkitBase.php on line 134

attiks’s picture

Status: Needs work » Needs review

go bot

attiks’s picture

FileSize
33.89 KB

file

Status: Needs review » Needs work

The last submitted patch, 65: i2330899-64.patch, failed testing.

mondrake’s picture

Re #63

+++ b/core/tests/Drupal/Tests/Core/Image/ImageTest.php
@@ -389,6 +389,38 @@ public function testCrop() {
+    // Test jpg.
+    $this->getTestImageForOperation('Convert');
+    $this->toolkitOperation->expects($this->once())
+      ->method('execute')
+      ->will($this->returnArgument(0));
+
+    $ret = $this->image->convert('jpg');
+    $this->assertEquals('jpg', $ret['extension']);
+
+    // Test gif.

'jpg' is not (yet ;)) a supported extension, so convert's validateArguments throws an exception which the toolkit base class tries to log as a warning - but there's no logger object so you get that failure. Changing 'jpg' to 'jpeg' PHPUnit tests pass locally.

fietserwin’s picture

There are also fails in other tests, see the log.

@attiks: do you still want me to reroll the patch?
@mondrake: do you prefer mime type or extension/format, both here and in your patch in #2063373: Cannot save image created from scratch (I think both issues should use the same)?

mondrake’s picture

#68 I'll be fine to use 'extension' on the other issue too. Yes, we should have consistency.

attiks’s picture

FileSize
33.89 KB

#67 Thanks for the pointer, new version attached

#68 Yes please, the goal of this issue was just to add a way to allow image extensions to change the extension/MIME type, you're right this has to be solved, but it does not has to happen in this issue. So for me it does not really matter how you want to solve this, but keep in mind that there's a beta tag, so only API additions are possible.

mondrake’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 70: i2330899-70.patch, failed testing.

fietserwin’s picture

Assigned: Unassigned » fietserwin

OK, reroll coming soon.

fietserwin’s picture

Status: Needs work » Needs review
FileSize
30.98 KB
16.88 KB

OK, I rerolled the patch from #70, so the interdiff is agains that patch. Most notable changes:

- Removed all mime type related functions. Consumng compoinentes can call the mime type guesser themselves on the result they get from getExtension().
- Changed the getExtension() methods to actually return the extension instead of passing by reference. This feels more natural given the name. (in fact, I think that transformDimensions should be adapted and renamed to getDimensions(), but that is for a follow up.)
- Extracted a part of the GDConvert::execute() method to a mehod GDToolkit::ExtensionToMimeType() so getSupportedTypes can remain protected and issue #2063373: Cannot save image created from scratch was doing the same int eh create_new operation introduced over there.
- Changes some comments and reduced them to 80 chars max.

Some notes:
- As expected (#39.6), some GDtoolkit tests still fail (I got 3 failures locally). This has to do with the way that GD handles transparency for various formats and how we handle image loading. so simply converting the extension and image format works, but what do we do with transparency when going from gif to png, etc. I will have another look at the tests to see if the tests need to be adapted or if the operation needs to be adapted. This should not stop others form reviewing.
- In ImageStyleTest::testGetDerivativeExtension() I get the idea that we are testing the mock method, not the method on ImageStyle itself. Can someone confirm or deny?

mondrake’s picture

I just cannot judge the implications of removing the mime type functions, so I'll leave to @attiks / @Jelle_S to comment.

From an image system point of view this seems ok, couple of points

1.

+++ b/core/modules/image/src/Entity/ImageStyle.php
@@ -380,4 +390,115 @@ public function setName($name) {
+  protected function addExtension($path) {
+    $extension = pathinfo($path, PATHINFO_EXTENSION);
+    $original_extension = $extension;
+    $extension = $this->getDerivativeExtension($extension);
+    if ($original_extension !== $extension) {
+      $path .= '.' . $extension;
+    }
+    return $path;
+  }
+

this can then be simplified:

+++ b/core/modules/image/src/Entity/ImageStyle.php
@@ -380,4 +390,115 @@ public function setName($name) {
+  protected function addExtension($path) {
+    $original_extension = pathinfo($path, PATHINFO_EXTENSION);
+    $derivative_extension = $this->getDerivativeExtension($extension);
+    if ($original_extension !== $derivative_extension) {
+      $path .= '.' . $derivative_extension;
+    }
+    return $path;
+  }
+

2.

+++ b/core/tests/Drupal/Tests/Core/Image/ImageTest.php
@@ -389,6 +389,38 @@ public function testCrop() {
+    // Test jpg.
+    $this->getTestImageForOperation('Convert');
+    $this->toolkitOperation->expects($this->once())
+      ->method('execute')
+      ->will($this->returnArgument(0));
+
+    $ret = $this->image->convert('jpeg');
+    $this->assertEquals('jpg', $ret['extension']);
+

the assert should check for 'jpeg', not 'jpg'

Status: Needs review » Needs work

The last submitted patch, 74: allow_image_effects_to-2330899-74.patch, failed testing.

attiks’s picture

#74, #75 I'm ok with it, nice job

mondrake’s picture

Couple more points

1.

+++ b/core/modules/image/src/ImageStyleInterface.php
@@ -132,6 +132,18 @@ public function createDerivative($original_uri, $derivative_uri);
+   * Determines the extension of the derivative without generating it.
+   *
+   * @param string $extension
+   *   The file extension of the original image.
+   *
+   * @return string
+   *   The extension the derivative image will have, given the extension of the
+   *   original.
+   */
+  public function getDerivativeExtension($extension);
+

At this point, why to pass the $extension as an input parameter? This method could just return NULL if no changes are needed, and a string with a new extension if any effect specifies it. It seems to me that the original extension is not needed in input.

2.

+++ b/core/modules/system/src/Plugin/ImageToolkit/GDToolkit.php
@@ -369,6 +369,27 @@ public static function getSupportedExtensions() {
+   * This is the reverse of the @see image_type_to_extension() function.

Please put the @see in a separate line so that docs can pick it up as a reference

attiks’s picture

#78 1/ it needs to be passed by reference, so it can go through all the effects. It is the same principle as the image dimensions.

mondrake’s picture

#79 I see, but in #74 it's no longer passed by ref - should be reverted then?

Jelle_S’s picture

+++ b/core/modules/image/src/Entity/ImageStyle.php
@@ -310,9 +310,19 @@ public function transformDimensions(array &$dimensions) {
+  public function getDerivativeExtension($extension) {
+    foreach ($this->getEffects() as $effect) {
+      $effect->getDerivativeExtension($extension);
+    }
+    return $extension;
+  }

Since you changed it not to be by reference anymore, this won't actually do anything. And also what @attiks said in #79

- In ImageStyleTest::testGetDerivativeExtension() I get the idea that we are testing the mock method, not the method on ImageStyle itself. Can someone confirm or deny?

I can deny. We mock the imageStyle, yes, but a mock is just a class extending the class it is mocking. Since we didn't overwrite the getDerivativeExtension method, it is testing that method on the original class.
Also we add an image effect mock to the image style that changes the extension. Because what we want to test is that if there is an image effect in the image style that changes the extension, that the getDerivativeExtension method on the imageStyle returns the correct extension. It doesn't really matter if the imageEffect is a real one or a mocked one.

Jelle_S’s picture

PS: We could start a discussion about the by reference passing of the dimensions and extension here, but since changing transformDimensions would be an API change and we are already in beta, it's a no-go. I think we should just be consistent (and IMHO, as @attiks explained it's the best or even only way to go) and pass the extension by reference as well.

fietserwin’s picture

Status: Needs work » Needs review
FileSize
32.88 KB
6.21 KB

Thanks all for reviewing, especially given that there were still failures.

#75.1: done

#75.2: done (I get the idea that unit tests don't run in the UI).

#77: OK, we stay with the current idea.

#78.1: I think that in #1826362: ImageEffects of the same image style should be able to pass variables between them we are going to add $style as well: if we want an image effect to "replay" what it is going to do, these methods need all the information that is available in the apply as well. E.g: contrib now has an aspect switcher: depending on if the derivative so far is portrait or landscape execute one or another action. If I want to create an effect that only changes jpeg to png but leaves gif as is (because I want transparency, not a specific format), I need the input.

#78.2: done

#79/#80: I solved the bug (see below), I stay with pass and return.

#81.1: Bug solved: return value is now used (did I already say that I think that unit tests don't run in the UI?)

#82: image is not a critical API, the only consumers are in image module/system itself and in contrib image effects. The latter being represented by me and @mondrake: if we say we want it, it might still get in.

Regarding the test failures in ToolkitGdTest: there was an error in the convenience method ExtensionToMimeType() I added. But more problematic is that the gif to png conversion (and vice versa) cannot handle transparency correctly. I propose that for now we accept this as a "documented feature" (the tests document it) as this might be very hard to solve, if solvable at all with GD.

i also solved another problem with expects($this->once()) in testConvert() in ImageTest. I reduced the number of tests to 1 as in the other tests. We don't need to test all possible (and correct) inputs, even if we can as in this case.

This should turn green (interdiff wrt to #74).

attiks’s picture

Status: Needs review » Reviewed & tested by the community

This is looking good, marking as RTBC. Thanks all for your help.

Dave Reid’s picture

This seems reasonable to me and good test coverage to back it up. The only thing to me was the 'addExtension' method which without reading the docblock was confusing to me. Once I understood what it actually did, it might have been better to just name it something like getPathWithExtension, but considering that path isn't really "stored" on the image style class, it makes sense the way it is. +1

fietserwin’s picture

Yeah, the term extension often leads to confusion #1308152-92: Add stream wrappers to access extension files :)

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Ok, read this patch over in-depth and couldn't find anything to complain about. This also unblocks progress on the picture stuff, so...!

Committed and pushed to 8.0.x. Thanks!

fietserwin’s picture

Assigned: fietserwin » Unassigned
Status: Fixed » Reviewed & tested by the community

Hmm, I don't see the commit in Drupal 8.0.x, can you check if there was a problem?

Jelle_S’s picture

Yeah, the code is nowhere to be found in the latest checkout either.

  • webchick committed 0366f4b on 8.0.x
    Issue #2330899 by attiks, Jelle_S, fietserwin: Allow image effects to...
webchick’s picture

Status: Reviewed & tested by the community » Fixed

Ugh I am so sorry. This was the last commit of the day and d.o rejected it with Permission denied as it sometimes does. :(

Pushed now!

Status: Fixed » Closed (fixed)

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

Gábor Hojtsy’s picture

Issue tags: -Media Initiative +D8Media

Fix media tag.

bendale’s picture

Hey everyone, I'm curious to know if there's a particular reason for keeping both the old extension and the new one when using the convert effect.

Chris Burge’s picture

@bendale - That's an excellent question. The source of the issue is potential namespace conflicts. Let's say you have image.jpg and image.png. If both are converted to webp, then you have an problem. I've dug into this very issue over the past week and should probably open a new issue against core. I don't have a solution, but I think it deserves its own issue for discussion.

Chris Burge’s picture

On a related note, this issue introduced a bug, #3412154: ImageStyleDownloadController::deliver() infers source image when generating derivatives with image format conversion, which would need to be addressed if we wanted to solve for replacing an image file extension instead of appending it when converting.