Add webP support to all image and file fields (including hero block).

[indigoxela]

Description of the bug

This popped up in the forum. Webp support is in core since 1.17.5. However, the list of allowed extensions in hero blocks is currently hardcoded and doesn't consider the latest change.

https://github.com/backdrop/backdrop/blob/ac819913fdb7aadcec9c7579fd33920327526792/core/modules/layout/includes/block.hero.inc#L56

Proposed solution

Either add webp to that hardcoded list (only if supported, of course), or provide a core function for "allowed image extensions", which modules can re-use.

---

TRANSLATION UPDATE:

Updated strings:

• old string: Only JPEG, PNG and GIF images are allowed.
• new string: Only images with the following extensions are allowed: @formats.

New strings:

• The selected image handling toolkit %toolkit can not correctly process %function.

[MikheevDesign]

Sub

[klonos]

Hey @indigoxela 👋 ...thanks for providing a PR for this, but it seems to be duplicating the code of the system_image_supported_extensions_alter() function.

...or provide a core function for "allowed image extensions", which modules can re-use.

Yes, that please ^^

[indigoxela]

...or provide a core function for "allowed image extensions", which modules can re-use.

To be honest, I looked at the existing code, but wasn't able to find an appropriate place for a generic "allowed_image_extensions_default" function. Any hint is welcome.

[klonos]

Well, it would have to be a component that is always loaded (like the System module), or somewhere like core/includes. Some places to consider:

• core/includes/common.inc ("Common functions that many Backdrop modules will need to reference.")
• core/includes/file.inc ("API for handling file uploads and server file management.")
• core/includes/utility.inc (not 100% sure about this)
• core/modules/file/file.api.php
• core/modules/file/file.field.inc
• core/modules/file/file.file.inc

[philsward]

Originally I thought "let it be customized", then I thought "it needs to be locked to image extensions, so hard code it".

Guess I'm on the fence. An admin might like to limit the image extensions for content editors, so there's that.

Has anyone complained about it being hard coded other than not allowing webp? If not, it might be easiest and fastest to add that extension and wait to see if anyone brings up the ability to edit them in a later issue.

Editing or limiting the extensions could also live in contrib.

Food for thought.

[indigoxela]

Hm. At first sight core/includes/file.inc looked promising, but if such a function gets put there, it'll require a complete rewrite of the current way hook_image_supported_extensions_alter() is implemented. That's a big API change IMO.

[indigoxela]

@klonos I tried several ways to use a generic (re-usable) function, but it's not possible without breaking changes.

Explanation:

Hero blocks ship with the layout module, which has no dependency on the image module, that's why the upload field in hero blocks is a simple file field, not an image field (with image library support), I suppose.

The hook used by the system module to add webp to the list of supported extensions ships with the image module - hook_image_supported_extensions_alter(). Having that generic function available in the layout module, and still using that hook would require to move it out of the image module, which would in turn require renaming it. (Or adding a dependency in the layout module to the image module.) That's an API change I wouldn't want to do just to prevent a few lines of code duplication.

So my conclusion is: a little code duplication doesn't hurt as much as a breaking API change. :wink:

[klonos]

So my conclusion is: a little code duplication doesn't hurt as much as a breaking API change. 😉

Agreed. I'll review/test this soon as I get a chance. Thanks @indigoxela 👍

[indigoxela]

@MikheevDesign are you willing to test? It should be easy, as every pull request ships with a sandbox:

• Website: https://pr3527-qsuae6jlw3fqrgbsohnisgcntdf5xxil.tugboat.qa/
• Username: admin
• Password: cd0c66198907

Just log in, do your testing, and report your findings here in this thread. We really appreciate testing of all kinds. :smiley:

[laryn]

Can we expand this issue to include webp extension on new image fields by default?

[indigoxela]

Can we expand this issue to include webp extension on new image fields by default?

I'm not sure if this is the right issue for that. The hero image is a pretty special thing, not related to "normal" image fields created via UI.

[MikheevDesign]

@MikheevDesign are you willing to test? It should be easy, as every pull request ships with a sandbox:

• Website: https://pr3527-qsuae6jlw3fqrgbsohnisgcntdf5xxil.tugboat.qa/
• Username: admin
• Password: cd0c66198907

Just log in, do your testing, and report your findings here in this thread. We really appreciate testing of all kinds.

Sorry, didn't understand the purpose of testing, because webp is still not available for hero

[indigoxela]

@MikheevDesign sorry, that was my fault, I forgot that the php/gd version in sandboxes do not support webp.

You'd actually have to test locally. Make sure that your gd version supports webp on /admin/reports/status/php. If so, then webp should be available out of the box in hero blocks.

Let us know, if you need any guidance regarding local setup for testing.

[MikheevDesign]

@MikheevDesign извините, это моя вина, я забыл, что версия php / gd в песочницах не поддерживает webp.

На самом деле вам придется тестировать локально. Убедитесь, что ваша версия gd поддерживает webp в / admin / reports / status / php. Если это так, то webp должен быть доступен прямо из коробки в блоках героев.

Сообщите нам, если вам понадобятся какие-либо указания относительно локальной настройки для тестирования.

Thanks for the quick response, I will soon solve the problem with hosting and test it on a working host and write about my results here

[laryn]

Can we expand this issue to include webp extension on new image fields by default?

I'm not sure if this is the right issue for that. The hero image is a pretty special thing, not related to "normal" image fields created via UI.

I guess I was referring to the conversation about taking this one level higher with a generic "allowed_image_extensions_default" function -- which would also be useful in the case I raised. In any case, I've opened https://github.com/backdrop/backdrop-issues/issues/4998

[yorkshire-pudding]

@indigoxela - thank you for this. I've tested, reviewed the code and updated labels. Hopefully as this is a task, we can get it in the next bug fix release.

[avpaderno]

This happens also for user pictures: Despite the GD library used by the server handles WebP images, I cannot use that image type because the allowed extensions do not include .webp. (The error message is Only files with the following extensions are allowed: jpg jpeg gif png txt doc xls pdf ppt pps odt ods odp docx docm pptx pptm xlsx xlsm.)

[yorkshire-pudding]

@kiamlaluno - probably worth a separate issue but linked to this; it might use a similar approach but will be in a different location.

[avpaderno]

@yorkshire-pudding I have already created it. I was just saying that hard-coded extensions are used on different form elements.

[yorkshire-pudding]

I see that now - https://github.com/backdrop/backdrop-issues/issues/5859

[avpaderno]

I forgot about that issue. See also #5917.

[klonos]

@yorkshire-pudding the PR introduces yet another place where we are hardcoding extensions, so we need to do better than that.

@indigoxela I was thinking something like a generic backdrop_allowed_image_extensions() function (or image_allowed_extensions() or something like that), which has an array of extensions, and invokes a hook to allow contrib/custom modules to alter the extensions in that array. The check for support of webp should live in that function or in a hook in the image module I guess. Then use that function in BlockHero::form().

Based on the above + on the fact that the PR is rather old now and needs a rebase, setting this to NW.

[indigoxela]

I was thinking something like a generic backdrop_allowed_image_extensions()

Agreed, it's about time to find a more generic approach and replace these hardcoded checks. :+1:

[jenlampton]

Can we expand this issue (and update the PR) to include the new function backdrop_allowed_image_extensions()?

I don't like explicitly adding webp in this one instance, and only when the image toolkit is gd. People could be using imagemagick with webP support, and this solution wouldn't work for them. (removing RTBC label)

If we add the new function backdrop_allowed_image_extensions() function, we should make sure the resulting list of extensions is alterable, so that the imagemagick module can add webP if/when it is supported.

[jenlampton]

~I'm expanding this issue to cover all image form elements, since I think it is possible to add an image type upload validator directly to the form API element, so it gets called for every image upload.~

Oh 🤯 there's no image form API element, we only have file and managed_file... I'm going to create a separate issue to add one and return this issue to hero block only for the time being.

[jenlampton]

I have created https://github.com/backdrop/backdrop-issues/issues/5946 - it might make more sense to be adding backdrop_allowed_image_extensions() over there.

[yorkshire-pudding]

That makes sense, then this ticket can hook into the new global elements.

[herbdool]

I disagree. This can't be tied to a new form element. A generic function is needed that can be loaded without the image module because it has to work for file elements as well. We can't switch all of them to use a new form element, at least not in 1.x. And there's contrib too. I think we want to allow webp or other extensions even if using the file element.

I think we'll need to somehow make another system_image_supported_extensions_alter and put it in file.inc. Maybe that old hook can be a "wrapper" for the new one?

[herbdool]

Thinking more about it: we can just keep that hook as it is, and have it's invocation call the new function suggested backdrop_supported_image_extensions(). Though I changed it to "supported" and not "allowed" since what's allowed can still be restricted per field. Webp might be supported but maybe you don't want it allowed for a field.

[herbdool]

I've got a PR https://github.com/backdrop/backdrop/pull/4366 that could use some feedback. It might not be the best approach but I think it's getting there. It reduces duplication, is backwards-compatible and doesn't require the image module to be enabled.

[avpaderno]

Image toolkits return the images they support with image_<toolkit>_get_info(), which is not invoked in the PR.

[avpaderno]

To expand my previous comment: The image toolkit is the Backdrop part that should load, validate, and save the images uploaded in form elements. The fact WEBP images are not allowed in some form elements is not caused by the image toolkit that is rejecting the image, but it is caused by a validation handler that checks the file extension, which is not even able to handle images. In fact, it allows extensions that are not usually used for images, like txt, doc, xls, pdf, or ppt (just to list some of those extensions).

What is really causing the "issue" is that there is not an image form element that can be used to upload an image; we are using a form element that accept any file, as @jenlampton said in a previous comment.

[herbdool]

@kiamlaluno image_gd_get_info() is used and adapted in this PR, and so is the validation handler. We can't use it directly to provide a list of supported extensions because it requires an image object and we need a way to not just validate but also display what extensions are supported. That's why I took the approach in my PR.

Creating an image form element is a worthy effort, but it's not going to be backwards-compatible. This should work for contrib/custom modules as well. And will also need to support Webform file form elements, which rely on core's file form element.

So... I don't think it's perfect but I think this is the direction this needs to go.

[avpaderno]

Would the code still work if the site enables another image toolkit?
The code is hard-coding the allowed extensions, but that code is simply moved in image_get_supported_extensions(). To work with every image toolkit, it should invoke image_<toolkit>_get_info() where <toolkit> is the currently enabled toolkit (graphicsmagick_toolkit or imagemagick, for example).

[herbdool]

Yes. A toolkit already needed to implement hook_image_supported_extensions_alter() for the image widget and I kept that hook, but moved it calling it to core/image.inc so it can be called more broadly.

In GD it's still hardcoding the allowed extensions, as before. I didn't want to change that in this PR -- keep the number of changes in one PR as small as needed. It can be discussed on the other issue.

We can't call image_<toolkit>_get_info() in image_get_supported_extensions() because we don't have an $image object. Plus that function returns the details for that single image (which only has one extension) rather than all the extensions supported.

[herbdool]

A variation of this would be to create a new function like image_<toolkit>_supported_extensions() and have the hook implementation call that. But that would also require yet another new function to call that since the current functions like that use image_toolkit_invoke('some_action', $image) and we don't want the image. So using the hook was the easiest at this point. But I'm happy to adapt the PR to whatever makes it clearer (and still backwards-compatible).

[avpaderno]

@herbdool Thank you for your patience and help. Looking at the currently used code after reading your explanation, I understood better what the PR is doing.
image_field_widget_form() is already invoking hook_image_supported_extensions_alter(). I found strange to use two different mechanisms to handle image extensions, and I thought the image toolkit should have the preference.

I like the idea of image_<toolkit>_supported_extensions(), but I understand that is better to think about that in a different issue. I would just say that image_<toolkit>_settings() do not get any image object; invoking it just requires code similar to the following one.

$function = 'image_' . image_get_toolkit() . '_settings';
if (function_exists($function)) {
  $form['image_toolkit_settings'] = $function();
}

[herbdool]

Thank you too @kiamlaluno. I've changed image_get_supported_extensions so it now calls image_TOOLKIT_supported_extensions. So now it's clearer that a toolkit needs to implement that. And I simplified my adaptation of image_gd_get_info so it calls image_type_to_extension() but then ensures that the extension is one of the (currently) supported extensions for Backdrop's use of GD.

[herbdool]

@kiamlaluno it's ready for another test/review. Tests are passing. There is one test under PHP 5.6 that is passing for me locally (with PHP 5.6) but not here. Perhaps there's something weird about the PHP build here, but would be good to have a second person run the tests locally to compare.

[avpaderno]

The failing test is ImageFieldValidateTestCase::testTypeSupport().

Looking at the code used by that test, I noticed a bug I reported on #6046.

[herbdool]

@kiamlaluno thanks for letting me know. I added that small change to this PR since it would be good to see if all tests pass now. If your PR gets merged first I can update this PR again.

By the way, I also added $this->assertFalse($webp_supported, 'WebP image type is not supported for this PHP build.'); and $this->assertTrue($webp_supported, 'WebP image type is supported for this PHP build.'); so we can tell for sure if our tests were set up properly.

[stpaultim]

This looks like an issue that could get into 1.24.2 and 1.25.0.

[herbdool]

Nope. That one test still fails. I also tried this variation: $webp_supported = (imagetypes() & IMG_WEBP); which on PHP 5.6 build of the tests passes but the image fails to be created.

[herbdool]

Finally, all tests pass.

[stpaultim]

Seems like the next step is for someone to test the PR and if it's working, to mark "Works for Me."

[herbdool]

I'm not sure if our build of Tugboat supports webp. I can't get it to work there. I'll test my own stuff locally to compare.

[indigoxela]

I'm not sure if our build of Tugboat supports webp

It doesn't (oddly compiled version of GD). IIRC that was one thing we struggled with, when introducing webp. The GitHub runners do, though.

The only thing we can reliably test with the Tugboat builds is, that the messages are correct when webp isn't supported. :grin:

[herbdool]

It doesn't (oddly compiled version of GD).

Thanks @indigoxela, that helps. I thought I had done something wrong.

I tested again on my local and it seems to be working well enough. It should work for existing sites with hero blocks and with CKEditor. Any custom fields will need to be edited and have webp added to the list of supported extensions. We want it that way since site builders may have reasons for restricting it.

So anyone can test these scenarios:

• hero block image
• CKEditor image upload
• existing image fields (should not have it unless added manually)
• new image fields (should have it when created)

[indigoxela]

So I tested locally, with patch applied.

• Hero image
• CKEditor image upload
• Existing image field (with or without webp allowed)
• Newly created image field

Everything works as expected. :+1:

[herbdool]

Thanks @indigoxela.

Now I'm wondering if we should put a hook in this PR to support image types like SVG that are not strictly supported by the GD library (and don't need to be). I was just looking at https://github.com/backdrop/backdrop-issues/issues/3136