Let GDToolkit support WEBP image format

Continuing the discussion from #2063373: Cannot save image created from scratch. to start< I copy the relevnat comments in this comment:

#2063373-37: Cannot save image created from scratch:

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: Allow image effects to change the extension, and with #2340699: Let GDToolkit support WEBP image format: GDToolkit has a fixed list of supported types 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.

#2063373-38: Cannot save image created from scratch:
#37: I would leave the discussion on supporting additional image formats to a separate issue like e.g. #2340699: GDToolkit has a fixed list of supported types. 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.

#2063373-39: Cannot save image created from scratch:
#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.

#2063373-40: Cannot save image created from scratch:
#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 :-) )

If you want to extend an existing toolkit, you probably want to "hijack" the id as well (besides aa a plugin I also see the toolkits as a service and thus as replaceable items). Otherwise you can alter plugin definitions with a hook or, indeed, you get a lot of boilerplate code to inherit each and every operation, just to be able to place another annotation above it.

Maybe I'm misunderstanding this, but is one of the comments above suggesting that Drupal wouldn't be able to use gif files?

@mdrummond: No, GIFs are supported, but BMPs for example are not. This means you can upload them in an imagefield and they will be displayed in the browser but you won't be able to apply image styles to it because the gd toolkit in Drupal Core doesn't support them.

First stab at this. The image tests take ages on my local machine so I didn't run them all, but the ones I did were green.

This patch adds support for all image types that have a corresponding imagecreatefrom* and image* function (e.g. imagecreatefromjpeg and imagejpeg)

Fatal error seems (to me) to be unrelated to this patch. Either HEAD is broken or testbot is acting up... So let's try this again.

Thank for giving this a try.

There's a fail on ImageFieldValidateTest in the log as well, I guess that could be due to this patch.

1. +++ b/core/modules/system/src/Plugin/ImageToolkit/GDToolkit.php
   @@ -30,11 +30,11 @@ class GDToolkit extends ImageToolkitBase {
      /**
   -   * Image type represented by a PHP IMAGETYPE_* constant (e.g. IMAGETYPE_JPEG).
   +   * The image extension.
       *
   -   * @var int
   +   * @var string
       */
   -  protected $type;
   +  protected $extension;
    
   

   I don't think we want extension back into the GD toolkit, stick with type. getimagesize() gives you the correct type, regardless extension.

   This means reverting all the type/extension related changes.

2. +++ b/core/modules/system/src/Plugin/ImageToolkit/GDToolkit.php
   @@ -200,21 +222,31 @@ public function save($destination) {
   +    elseif (!$data && $extension == 'webp') {
   +      // The getimagesize function does not support WebP, load the resource so
   +      // that isValid() still returns TRUE if it is in fact a valid image.
   +      $this->preLoadInfo = TRUE;
   +      if (!$this->load()) {
   +        $this->preLoadInfo = FALSE;
   +        return FALSE;
   +      }
   +      return TRUE;
   +    }
        return FALSE;
   

   GD does not support webp, so there's no need to accept that as valid and return TRUE.

   BTW: there's an IMAGETYPE_xxx constant for webp in 5.5, so if GD properly supports it in 5.5, than make it dependent on either the PHP version or that constant being available or not. However, I doubt that besides that constant any real support for it has been added.

This patch lacks tests for newly supported types. See e.g. #2215369: Various bugs with PHP 5.5 imagerotate(), including when incorrect color indices are passed in where we added tests for a non-transparent gif. So, I expect new test images for all types that are going to be supported by this type. If it is a format that supports transparency, I guess that even 2 test images are needed.

There is an imagecreatefromwebp function and an imagewebp function in PHP 5.5 but there is no IMAGETYPE_WEBP constant (not even in PHP 5.5) so GD supports WebP and doesn't at the same time. I was trying to add support for WebP as well in this patch, that's why I couldn't use imagesize and the IMAGETYPE_xxx constants for this patch.

Oops, I think I misread IMAGETYPE_WBMP as something like IMAGETYPE_WEBP.

Some historic background:
- #2066219: Decouple image type from image extension: moved from extension to image type, as using extension is even worse than using image type. Back then it was already discussed that image type is not perfect as it can only be used for GD supported image types, and even that turns out not to be true
- #2217783: Remove $extension and $mimeType from Image, use image_type_to_xxx function in the getters instead: cleaned up, by removing extension altogether. note that we are talking about image processing toolkits, not objects representing an image file. So, IMO, extension is not coming back!
- #2219349: Use file_mimetype_mapping instead of IMAGETYPE_* constants in Image class contains some explaining/reasoning about this subject

Some other considerations:
- Adding support for other image formats should include setting/passing save options specific to that format.
- #2063373: Cannot save image created from scratch will introduce image objects that started from scratch (i.e. a blank canvas). These do not have an extension. Even mime type is questionable, see the comments over there.
- #2257163: Restrict image system to image processing tries to remove the whole image format thing altogether, as it should not be part of Image to begin with.
- #2168511: Allow to pass save options to ImageInterface::save tries to allow for passing in options to the save method (possibly depending on the format).
- Contrib defines an image effect that changes the format of the derivative image. However it cannot change the derivative file name including extension, thus there will be images where the file extension is not telling the truth. This should be taken into account (including tests) when using mime type guesser.
- The same holds for webp. I understand that, as long as support is not common across all/most browsers, many web applications serve images with a .jpg extension and decide the actual image format that will be served based on the user agent header. Again: file extension is not always telling the truth.

Concluding:
So, if we need to represent the image format in the Image object:
- image type fails when we want to use the new 5.5 webp support.
- extension is a mess, not unique, confused with the concept of actual file extension and actual file extension is nothing but a hint.
- mime type is capable of representing all types we want and is (quite) unique, so we should go for this, but:
- mime type may not be based on file extension only, getimagesize() or exif_imagetype() should be preferred (exit_imagetype tells it's much faster than getimagesize(), but has the same limitations and requires an new (soft) dependency, including requirements reporting etc).

My suggestion:
- Create a prerequisite issue that addresses mime type guesser to include a method to get mime type based on actual file contents as well
- Finish #2063373: Cannot save image created from scratch, #2168511: Allow to pass save options to ImageInterface::save and #2257163: Restrict image system to image processing first.
- Only then extend image format support for the GD toolkit.

Contrib defines an image effect that changes the format of the derivative image. However it cannot change the derivative file name including extension, thus there will be images where the file extension is not telling the truth. This should be taken into account (including tests) when using mime type guesser.

This should be fixed once #2330899: Allow image effects to change the MIME type + extension, add a "convert" image effect gets in. But I agree the extension isn't always reliable.

Create a prerequisite issue that addresses mime type guesser to include a method to get mime type based on actual file contents as well

Actually, these already exist (Thank you Symfony), we can use \Drupal\Core\File\MimeType\MimeTypeGuesser and add:

1. \Symfony\Component\HttpFoundation\File\MimeType\FileinfoMimeTypeGuesser
2. \Symfony\Component\HttpFoundation\File\MimeType\FileBinaryMimeTypeGuesser
3. \Drupal\Core\File\MimeType\ExtensionMimeTypeGuesser

as mime type guessers (in that order of priority).

This should just work since the finfo extension is enabled by default since PHP 5.3.0 (source: http://pecl.php.net/package/Fileinfo, see the description).

#Jelle_S: thanks for this info. I directly put it to use in #2257163: Restrict image system to image processing. So if you could review that patch? That might come handy here.

@fietserwin: I'll see if I find some time tomorrow to review that patch.

Support for IMAGETYPE_WEBP constant is implemented in PHP 7.1

See:
https://bugs.php.net/bug.php?id=65038
https://github.com/php/php-src/pull/2157

This should more or less do it, for PHP versions above 7.1 where the new IMAGETYPE_WEBP constant is implemented. But we need to have bots running on that PHP version to have meaningful tests.

Ops.

Let's add a WEBP test file, although I am afraid it will break a lot of tests (see #2377747-101: Incorrect node create validation error when an invalid image is attached to a field).

Indeed. The quickest workaround I can think of is to use a different file name prefix for the test WEBP image, in order to avoid messing up with drupalGetTestFiles.

Let's pause until when we have test bots running on PHP 7.1

Simplified a bit.

Postponed on #2832842: Support PHP 7.1

Actually, #31 should fail on PHP 7.1. Maybe the testbots do not have PHP 7.1 with the webp option installed. Let's see this.

Patch in #34 was not formatted with --binary hence apply failed, but no other failure is strange. Here forcing to TRUE the value returned by ::isWebpSupported. Should fail in 5.6 related to missing IMAGETYPE_WEBP constant, in 7.1 for missing imagewebp() function (if my hypothesis that PHP 7.1 in testbots is compiled without webp support).

Yes. Now let's revert changes to ::isWebpSupported. We should have 5.6 tests passing, and 7.1 failing because of missing webp support.

I will open an issue to request PHP 7.1 on testbots to be installed with webp support.

Opened #2843489: Add WEBP support to PHP 7.1+

So a couple of questions:

Does this make WebP an requirement of php, or does it make it entirely optional (like pecl yaml)..? We definitely dont want to add php requirements that are not necessarily widespread (my debian build doesnt have it, for example).

*if* it is optional, then the tests should also detect that webp is not available and not run.

As for testing, I think we need to have a split strategy where drupal is primarily tested on its minimally required compile time options, and regression tested against php builds that have every optional compilation flag turned on.

My concern is if we add optional support for this then somebody may implement something that relies on it without a fallback, and the testbots wont alert because they have it enabled.

@Mixologic thanks. My opinion below, but I think more input is necessary here.

1) WebP support should be optional in Drupal. If we have PHP 7.1 or higher AND the WebP extension is installed, then WebP is supported by the GD toolkit. Otherwise, it's not supported. At max, we could have a requirements warning in the status report. ATM, Drupal requires the GD extension, but there are no detailed checks for any of additional libraries to support e.g. JPEG or FreeType. PNG support is checked as part of the Color module requirements (which is a bit weird).

2) PHP installations for the testbots should however have WebP installed, because we want to test that running GDToolkit operations on WebP images leads to same results as for other image formats. Since the toolkit itself will 'skip' supporting WebP if the conditions set above are not met, a specific test in ToolkitGdTest will fail when PHP is above 7.1, but no 'imagewebp' function is found.

Any test of an optional configuration must also be optional, and not run when that configuration is not available. We've got tests that are specific to pgsql that do not fail when we run on mysql. The same policy applies for pecl yaml vs symfony yaml. Hence whatever the implementation, if webp is not installed, the test must not be run and/or must not fail. In other words, you should not have this in your tests:

+++ b/core/tests/Drupal/KernelTests/Core/Image/ToolkitGdTest.php
@@ -52,6 +52,11 @@ protected function setUp() {
+
+    // For PHP 7.1+, ensure test PHP has webp extension installed.
+    if (PHP_VERSION_ID >= 70100) {
+      $this->assertTrue(function_exists('imagewebp'));
+    }

The testbots can have WebP installed so that you can prove that it does what it is supposed to do when it *is* available, but you shouldnt have any tests that *depend* on it being available.

Ah ok, thanks. That's the status of #31, actually. Retesting it on both 5.6 and 7.1, both should pass. Obviously, WebP images are not yet tested given current bots setup. Still postponed on bots getting WebP support installed.

Okay, makes sense. I have added WebP support in the php 7.1 containers, and re-ran the test in #39 to prove that they were there, although the test actually fails, but due to some color not matching, not due to WebP not being there: https://www.drupal.org/pift-ci-job/575164

@Mixologic thanks, all good on testbots now!

Now we can focus on the actual patch, and its color issues. In previous patches I was adding a WebP image converted by external tools from the current image-test.png. It seems like we are loosing color quality with that. Here I am trying by adding the test WebP image on the fly using GD functions.

imagewebp() has a quality setting parameter, by default at 80. Let's see if raising to 100 fixes the color difference.

This should fix the tests for the color differences, passes locally.

It's mostly to identify the differences and be able to work on them, rather than the to-be tests, though.

I can see 3 points that need work:
1. minor color differences - where we expect red (255, 0, 0), we get (255, 1, 0). Not a major issue but of course exact color matching does not work. We could re-use here the method 'colorsAreClose' that is in ImageMagick/Image Effects contrib modules to allow a small tolerance. Would be interesting to know where this difference is introduced though, i.e. in the source image or during the resampling/converting functions.

2. expected white in crop - this is a problem with the image built on the fly that does not have the white frame that the other test images have

3. full opaque black where full transparent expected - this is to be tested manually, looks tricky.

NW for the points in #50.

This is the first reviewable patch, I think.

1. WebP image format is supported by the GDToolkit, as long as PHP version is 7.1.0 or above (because only in 7.1.0 the IMAGETYPE_WEB constant, used internally by the toolkit to track the format, is implemented), *AND* PHP is compiled with WebP support. The changes to code are relatively simple, mostly to check that the conditions above are fulfilled.
2. Tests are adjusted so that they check the conditions above and only if met, specific tests for WebP are executed. If conditions are not met, tests are executed without that part but no failure reported. See #44.
3. Testbots running PHP 7.1 have been added WebP support so tests for that version are significant. See #46.
4. I could not find any code to produce test images similar to the ones that are present in the core/modules/simpletest/files directory, that were introduced around #373613-4: Create "Image" Objects, Operate on Images By Resource. Here I added a method to make one for WebP. I think it would be useful also as a documentation in case other types need to be supported.
5. Apparently, WebP is compressing colors so exact color checks in the tests would fail. Instead of skipping them completely as in JPEG, I suggest introducing a method ::colorsAreClose that allows setting a tolerance for the color difference. This method is the same as the one used in ImageMagick/Image Effect contrib modules.

... the patch ...

Actually, it looks like that GD is never using WebP lossless option for saving the resource to disk file, not even if quality is set to 100. For this reason, the image generated on the fly always has some color optimization applied. I am now trying to use the cwebp utility to convert lossless the test PNG image image-test.png to WebP. This would allow adding the WebP file image instead of generating it, and to simplify the test quite a lot.

Yes. Let's start again:

This patch:

1. Enables WebP image format support by the GDToolkit, as long as PHP version is 7.1.0 or above (because only in 7.1.0 the IMAGETYPE_WEBP constant, used internally by the toolkit to track the format, is implemented), *AND* PHP is compiled with WebP support. The changes to code are relatively simple, mostly to check that the conditions above are fulfilled.
2. Tests are adjusted so that they check the conditions above and only if met, specific tests for WebP are executed. If conditions are not met, tests are executed without that part but no failure reported. See #44.
3. Testbots running PHP 7.1 have been added WebP support so tests for that version are significant. See #46.
4. A WebP test image is added as core/modules/simpletest/files/img-test.webp. It is the WebP equivalent of image-test.png. The prefix is different ('img-' instead of 'image-') to avoid messing up with other tests that use drupalGetTestFiles to retrieve test images. See #27 and #29.

Re-uploading patch from #55 as the tests got messy.

7.1 test seems failing due to #2843693: Random test failure in CKEditor AjaxCss

Rerolled after #2776975: March 3, 2017: Convert core to array syntax coding standards for Drupal 8.3.x RC phase

Forgot --binary option in git diff

Fixes patch #65

Re-rolled patch from #65 against 8.6.x.

Generated the patch correctly this time!

Third time lucky? This time after using the --binary option with git diff.

Patch from #74 works for me. Thank you so much for your work!

How can we add support to Entity browser for Webp extension file type

Cleaned patch a bit:
- 100% quality is not needed (added in #48 and #54 tells that has no effect)
- added polyfill (todo should point to proper issue instead #2992116: Bump core dependencies minimum version to PHP 7.0 but I found none)

1. +++ b/core/modules/system/src/Plugin/ImageToolkit/GDToolkit.php
   @@ -234,12 +234,21 @@ public function save($destination) {
   +        $alpha_supporting_types[] = IMAGETYPE_WEBP;
   
   @@ -439,7 +448,21 @@ public function extensionToImageType($extension) {
   +      $supported_types[] = IMAGETYPE_WEBP;
   ...
   +    return PHP_VERSION_ID >= 70100 && function_exists('imagewebp') ? TRUE : FALSE;
   
   +++ b/core/modules/system/src/Plugin/ImageToolkit/Operation/gd/CreateNew.php
   @@ -90,9 +90,13 @@ protected function execute(array $arguments) {
   +    $webp = $this->getToolkit()->isWebpSupported() ? IMAGETYPE_WEBP : -1;
   
   +++ b/core/tests/Drupal/KernelTests/Core/Image/ToolkitGdTest.php
   @@ -121,7 +127,9 @@ public function testManipulations() {
   +      $expected_image_types['webp'] = IMAGETYPE_WEBP;
   
   @@ -381,7 +404,11 @@ public function testManipulations() {
   +      $image_types[] = IMAGETYPE_WEBP;
   

   This kind of detection is not complete, the pointed php-bug is about missing constant which could use polyfill

2. +++ b/core/modules/system/src/Plugin/ImageToolkit/GDToolkit.php
   @@ -439,7 +448,21 @@ public function extensionToImageType($extension) {
   +   * Checks if WEBP images can be supported.
   +   *
   +   * @return bool
   +   *   TRUE if WEBP images can be processed by GD, FALSE otherwise.
   +   */
   +  public static function isWebpSupported() {
   

   It looks too specific - other image types also could be missing from GD and it's clear that once toolkit starts capability detection it needs to take into account other image types, Follow-up material?

Fix coding standards nitpick

Missed --binary option

@vilepickle patch is valid, please check how --binary used in patches

@vilepickle please file issue to composer-patches about it, I guess in your case patch applied with patch utility which does not support binary
Then please check that your php supports webp like

$ php -r 'var_dump(function_exists("imagewebp"));'
bool(true)

Looks good to me, RTBC

EDIT: tested manually, OK - Druapl 8.7.x, PHP 7.3. Important to note is that this patch affects the GD toolkit only, i.e. does not 'magically' enable webp files to be uploaded in image fields.

How to test on vanilla Drupal installation:
1. apply the patch
2. edit the image field of the 'Article' content type to allow also the webp extension on top of the predefined 'jpeg jpg gif png'
3. add an Article and upload a webp file in the image field
4. note that not all browsers support webp: https://caniuse.com/#feat=webp
5. adding a 'Convert to PNG' image effect to the image styles will force the image derivatives to always produce a png file, that is supported by all browsers

Unrelated test failure. Back to RTBC.
#3035318: `DateFormatter()` assumes 30 days per month, while February only has 28 days. Causes fails in tests.

+++ b/core/modules/system/src/Plugin/ImageToolkit/GDToolkit.php
@@ -439,7 +444,23 @@ public function extensionToImageType($extension) {
+      $supported_types[] = IMAGETYPE_WEBP;
...
+  public static function isWebpSupported() {

I'm not sure that we should be adding a public static to GDToolkit for this. I'd be more in favour of making supportedTypes() public and inlining this logic. Then the test can check if IMAGETYPE_WEBP is in the array.

We also need a change record and an issue summary update. Also I think we need a statement on security - as far as I can see that (unlike SVGs) there's no additional risks from letting users upload webp files - but this does need research.

In reviewing the patch I also noticed it's linking to #2992116: Bump core dependencies minimum version to PHP 7.0 in multiple places while the issue was closed as a duplicate of #3039611: Update core PHP dependencies for 8.8.x so we might want to update that for completeness.

1. +++ b/core/modules/system/src/Plugin/ImageToolkit/GDToolkit.php
   @@ -12,6 +12,11 @@
   +// @todo Remove when core require PHP 7.1 https://www.drupal.org/node/2992116.
   

   s/core require/Drupal core requires/g

   Should the URL be in an @see?

2. +++ b/core/modules/system/src/Plugin/ImageToolkit/GDToolkit.php
   @@ -12,6 +12,11 @@
   +// @todo Remove when core require PHP 7.1 https://www.drupal.org/node/2992116.
   
   @@ -439,7 +444,23 @@ public function extensionToImageType($extension) {
   +   * Checks if WEBP images can be supported.
   ...
   +   *   TRUE if WEBP images can be processed by GD, FALSE otherwise.
   

   s/WEBP/WebP/g (we're referring to the format itself, not a PHP constant).

   Also, "can be supported" seems odd to me. Perhaps "are supported"?

3. +++ b/core/modules/system/src/Plugin/ImageToolkit/GDToolkit.php
   @@ -234,8 +239,8 @@ public function save($destination) {
   +      // Image types that support alpha need to be saved accordingly.
   

   Are we talking about alpha channel or alpha blending? We should likely clarify that.

4. +++ b/core/modules/system/src/Plugin/ImageToolkit/GDToolkit.php
   @@ -439,7 +444,23 @@ public function extensionToImageType($extension) {
   +   * Checks if WEBP images can be supported.
   

   s/WEBP/WebP/g

   Should we clarify 'GD' is actually the 'GD library' or 'GD graphics library'?

5. +++ b/core/modules/system/src/Plugin/ImageToolkit/Operation/gd/CreateNew.php
   @@ -4,6 +4,11 @@
   +// @todo Remove when core require PHP 7.1 https://www.drupal.org/node/2992116.
   

   s/core require/Drupal core requires/g

   Should the URL be in an @see?

6. +++ b/core/tests/Drupal/KernelTests/Core/Image/ToolkitGdTest.php
   @@ -7,6 +7,11 @@
   +// @todo Remove when core require PHP 7.1 https://www.drupal.org/node/2992116.
   

   s/core require/Drupal core requires/g

   Should the URL be in an @see?

7. +++ b/core/tests/Drupal/KernelTests/Core/Image/ToolkitGdTest.php
   @@ -107,8 +112,14 @@ public function testManipulations() {
   +    // Get a blank Image object.
   

   Why would Image take an uppercase 'I'? If it's referring to somewhere in the API, we should probably clarify that. Otherwise let's just go with a lowercase 'i'.

+++ b/core/tests/Drupal/KernelTests/Core/Image/ToolkitGdTest.php
@@ -107,8 +112,14 @@ public function testManipulations() {
+    // Get a blank Image object.

It is referring to the Image class; that is why it's capitalized. The longer way to say that would be Get an instance of the Image class containing a blank image. Get a blank image object. would be too generic, IMO, as it could mean any object used to handle/represent an image.

The WebP module has come a long way and is almost stable I think - https://www.drupal.org/project/webp. Would it be easier to simply add this module/code into core?

As 7.2 a requirement for 9.0 it makes sense to ask @mixologic about support for testbots

@W01F and all, correct me if I'm wrong, but webp should be parallel to this. This current task will allow to manage webp in core, but dealing with the visualisation layer would be outside of the scope of core, and that's what webp module helps with. Am I having the wrong side of the stick here?

And yes, webp should be having a stable release as a Xmas gift :)

Thank you

@alexmoreno exactly! this issue targeted to read GD extension support for webp - everything else is follow-up

AFAIK a feature request should be, now, addressed to D9.1.x. Since that will have PHP 7.2 as a minimum, the patch here will be simpler.

Not sure it simplifies because php could be built without webp support, so detection still needed

Rerolled, adjusted according to #101, and addresses #92

... making supportedTypes() public and inlining this logic. Then the test can check if IMAGETYPE_WEBP is in the array.

Looks good! Except image in simpletest, should go core or system module

#104 thanks @andypost, done.

This constant defined since 7.0.10 in gd extension https://www.php.net/manual/en/function.imagetypes.php
But 8.x set on ≥7.0.8

Drupal 9 requires PHP 7.3; the constant is always defined, for that PHP version.

Here's a patch that just extends supported types as core 9 require 7.3 (so all conditions reverted)

Patch still using binary image so should be applied with git apply 2340699-110.patch

The reason for this

+++ b/core/modules/system/src/Plugin/ImageToolkit/GDToolkit.php
@@ -458,12 +458,8 @@ public function extensionToImageType($extension) {
-    if (function_exists('imagewebp')) {
-      $supported_types[] = IMAGETYPE_WEBP;
-    }

though is to check that PHP was compiled with WebP support, independently from the presence or not of the IMAGETYPE_WEBP constant. Agree that probably this is less of a problem now than it was 4 years ago, but still.

Feature detection is good to have but the same applies to jpeg, png at least... So could use follow-up to add this capabilityChecker

As webp supposed to be used in most PHP installs since 7.0 then core should not limit modern gd

Tested #110. the patch applied nicely. before the patch you can not upload webp images, after the patch you can upload and show webp images.

I think we need to do #111 because atm having IMAGETYPE_PNG, IMAGETYPE_JPEG, IMAGETYPE_GIF are implicit dependencies. So you have a working site if you compile PHP without webp support. This issue changes that - so we need to add the check in.

Also this still needs a change record.

I've googled around for security issues with webp - checked Mitre's CVEs etc and haven't found anything. So that looks okay.

For #111 it could use cheaper way to check support of the format... gonna dig (as constant is always defined)
If toolkits someday will can detect supported formats it should be fast one-time (statically cachable state)

+++ b/core/modules/system/src/Plugin/ImageToolkit/GDToolkit.php
@@ -458,12 +458,8 @@ public function extensionToImageType($extension) {
-    if (function_exists('imagewebp')) {

check for function exists is expensive so may need static cache somehow

https://www.php.net/manual/en/function.imagetypes.php#refsect1-function....
States that support of format is independent from constant presence in PHP itself

And as we require PHP 7.3 for 9.1 core we sure that constant will be defined and format could be used

If existing sites has no webP(or png) support build in they will behaves the same way after patch - no format supported by php, then no image you'll get
That's why I find real detectionion of format support a separate from definition of constant

I am not sure what the supported by the version of GD linked into PHP phrase in the imagetypes() documentation means, but should not code similar to the following one work?

if (imagetypes() & IMG_WEBP) {
    // The library supports the format.
}

Does supported by the version of GD linked into PHP mean the version of GD linked into PHP has been compiled with the support for that file format or the version of GD linked into PHP would support that file format if it were compiled with the support for that file format? In other words, are there sites where imagetypes() & IMG_WEBP would evaluate to TRUE but the GD extension isn't able to handle that file format?

Filed CR https://www.drupal.org/node/3171135

Updated IS

Re #118 basically support of WebP is defined at compile time by --with-webp (or similar) also GD extension could be build using system's libgd.so with OS vendor support of WebP and other types (jpeg,png,gif)

to check support it needs to use gd_info() or hacks like function exists (not sure the definition of function means real support in libgd)

And as PHP 7.3 source shows - function always exists https://github.com/php/php-src/blob/PHP-7.3/ext/gd/php_gd.h#L182

Furthermore, are we really interested in knowing if the version of GD linked into PHP is able to handle the file format? The documentation for imagejpeg() and imagewebp() contains the following warning.

Caution

However, if libgd fails to output the image, this function returns TRUE.

I take this means that libgd could fail to output a JPEG image, for any reason, and we will get a return value saying that the operation was successful. If this is the case, then the code should apply to the WEBP support what done for the JPEG support.

@kiamlaluno thank you! that's what I'm trying to said in updated summary

PHP supports format but delegates implementation for GD&co so core can't be 100% sure that image will be created

I apologize: I was posting my comment when you edited the IS, and I didn't see the changes you have done. My comment was not meant to say that your edit was not clear.

@andypost it's good to know that the constant is defined separate from the inclusion of format support. I agree that #110 looks good then.

+1

Re-roll #110 for 9.2.x looks noting left

Use git apply --binary to apply patch

Caniuse reports 92.13% support of the format, initially released in 2010. I daresay it's time to get it in. AVIF is already in the pipeline to replace it...

#3116611: Add a requirements check for GD support of allowed image types could follow to improve status report, telling which formats are actually supported by the current PHP implementation.

Avif needs to be accepted to GD first (so around D11 sounds possible) https://github.com/libgd/libgd/issues/557

Committed d829a0e and pushed to 9.2.x. Thanks!

Atm the release note says:

Core can manage WebP images

I think this should be

The GD toolkit and, therefore Image styles, can manage WebP images.

Updated release note, also noting that WebP needs to be supported by PHP itself.

Hey all - I just ran across this issue!

I'm the one creating the PR to add AVIF support to GD. I expect it will be accepted and merged within the next few days. The project plans to then do a cherry-picked release to make this available in libgd.

I imagine you want to add webp support first, and avif support later - but I thought I'd bring this up in case it was convenient to add AVIF as a followup! I imagine the task would be quite similar.

Thanks @benmorss. It would be worth opening a follow-up issue for that, to be postponed upon upstream availability of the PHP release that will eventually support the format first. Webp is now supported in the 9.2.x branch, Drupal 9.2.0 will be the first point release supporting it in productive sites.

BTW, and since this was a blocker for Drupal GD toolkit supporting WebP for a couple of PHP minors: libgd support by itself is not enough. Unless we change logic, we’ll need also PHP’s getimagesize function (which is separate from libgd) to return an IMAGETYPE_XXX constant specific for thr image format.

Thanks for pointing out that getimagesize would be a blocker.

In the docs, getimagesize is categorized under "GD functions". And yet, you're right, it's not part of GD. Looks like it needs support here.

Filed #3202016: Let GDToolkit support AVIF image format

Tagging for either a release note or a release highlight (I'm not sure which one we want for this). The snippet is already in the issue summary: thanks for that!

I'm guessing a release note, so removing the other tag.

@catch clarified to me that this should be a highlight, not a release note, because there's no risk of it breaking anything on existing sites.

PHP 8.1 Added lossless conversion for webp https://github.com/php/php-src/commit/eb6c9eb936c62a5784874079747290672b...

PHP 8.1 Added lossless conversion for webp https://github.com/php/php-src/commit/eb6c9eb936c62a5784874079747290672b...

Indeed!

@andypost, @mondrake, etc - I just made an issue for lossless webp support in Drupal :)

See this follow-up issue for missing fallback for older browsers (IE11 / Safari): #3213491: Add fallback format support to responsive images

Due to replacing the original image with a .webp image in <img> tag instead of using a combined <picture> tag.