drupal_image() needs #width / #height values for fully working image cache scale, lazy-loading and img attributes

Okay let's further split this! I'll create a separate issue for the lazy loading things.
Also I updated the issue summary.

MR!22 implements a partial solution, calculating width and height similar to what @batkor did in 💬 Add width and height attributes in drupalImage Closed: won't fix (please add him for credits)

The other points still have to be discussed.

Important note regarding responsive_image:
The fallback image never has width / height attributes by design, see:
See the example at [#3279032] and the comments in code:

// We don't set dimensions for fallback image if rendered in picture tag.
// In Firefox, it results in sizing the entire picture element to the size
// of the fallback image, instead of the size on the source element.

(responsive_image.module)

The reason why original #width and #height parameters are needed can be found here in Core:
https://api.drupal.org/api/drupal/core%21modules%21image%21image.module/...
=>
https://api.drupal.org/api/drupal/10/search/transformDimensions
=>
https://api.drupal.org/api/drupal/core%21modules%21image%21src%21Plugin%... (e.g.)

While I'm not sure what's still wrong, here's the test-only patch to show the issue exists and width and height are not returned from image module, if they are not present, as code shows in #10.

Okay, before investing more time here, I think @Chi should have a look and express his oppinion how to fix this.

The last submitted patch, 11: 3356042-image-style-width-height-missing-TEST-ONLY.patch, failed testing. View results →

Well, drupal_image() currently has 5 arguments. That's already too many. Adding more arguments will make it more complex for users.

Still wonder what's wrong with using attributes.

{{ drupal_image('1', attributes={width: 120, height: 80, alt: 'Example'|t }) }}

@Chi: The difference is, that these from #16 are just passed to HTML output as-is. You could also add data-* attributes etc. which would be fine, but the properties listed in the theme hook are special:

      'variables' => [
        'style_name' => NULL,
        'uri' => NULL,
        'width' => NULL,
        'height' => NULL,
        'alt' => '',
        'title' => NULL,
        'attributes' => [],
      ],

so in contrast to any other image_theme implementation, the current one here won't provide width, height, alt and title where it's expected, for example when altering / preprocessing (responsive_)image_theme. That's bad.

But even worse the missing width and height detection, which this issue is mainly about, leads to missing width / height attributes because imagecache doesn't calculate them, if the original sizes are not given. We run into this issue with every Google Pagespeed check:

Image elements do not have explicit width and height
Set an explicit width and height on image elements to reduce layout shifts and improve CLS.

for any image run through drupal_image(). Reasons are given above.
So if a $style is set, the width and height should be determined. If no style is given, that part can be left out.

I think Pagespeed warns about missing attributes not Drupal variables. Passing them as described in #16 would definitely resolve the problem.

As for the variables in the preprocess hooks, they are optional. That means we do not violate the contract.
Note that in many cases real images size it not something users want. So if we extract the size form image file they will still have to provide width/height through attributes to override the calculated values.

Again the challenge here is tricky implementation.

The proposed solutions require either a run-time file operation with image processing or extending the function signature which is already overwhelmed.

I think Pagespeed warns about missing attributes not Drupal variables. Passing them as described in #16 would definitely resolve the problem.

Yes of course, but this makes the functionality very uncomfortable and I think it's not how Drupal typically handles this. Drupal expects width and height for image caches to work.

As for the variables in the preprocess hook, they are optional. That means we do not violate the contract.

Yes, optional, but standardized and helpful.

Note that in many cases real images size it not something users want.

In modern development I have to disagree and that's what imagecache / image styles are useful for. Furthermore the proposed implementation matches this, as they are not added, if no image cache is used. If one is used, you'd want the sizes to work and be (re-)calculated.

My key point still is: With the current implementation, imagecache will never set width / height and that's pretty bad (and from my perspective totally unexpected).
Did you try that out yourself? Can you reproduce this issue?

Which kind of solution, perhaps from the proposed ones above would you prefer?

Note that in many cases real images size it not something users want. So if we extract the size form image file they will still have to provide width/height through attributes to override the calculated values.

True, but my understanding here is: The original size is passed to the drupal_image_style() and will result in the correct width & height, which are returned/calculated using the given imagecache.

Still wonder what's wrong with using attributes.

{{ drupal_image('1', attributes={width: 120, height: 80, alt: 'Example'|t }) }}

While this sure will work, its a horrible worflow, isnt it? If an imagecache is given, you would have to go to the imagecache configuration page, and get the height and width. While its completely unclear to me, how the process will be, if the image is only scaled (so you have only one value height or width configured). If the imagecache configuration is changed, the width & height also need to be changed in the Twig code..

Well, drupal_image() currently has 5 arguments. That's already too many. Adding more arguments will make it more complex for users.

Yes, I am with you on that point. Probably Twig Named Arguments, are a really good workaround here.

The original size is passed to the drupal_image_style() and will result in the correct width & height.

That's the problem. We don't have the original size. We have to load the file and process it using the image toolkit each time a template is rendered. This will obviously have some performance penalty. That's why Drupal core stores image size in field storage when the image is uploaded.

its a horrible worflow, isnt it?

Probably Twig Named Arguments, are a really good workaround here.

I am a bit confused. How would named arguments help to avoid the current "horrible" workflow?

So perhaps the solution here would be to work at the field based level (one level up) instead of the image level? Then we wouldn't have the performance impact, but could have a similar solution, if we're working on field values already.
In that case it would already help to be able to set the #width / #height properties, because they can be obtained from the field and drupal_image() could still be used.

So we should see these three things as separate, but more or less related problems with that function.

Or let the developer decide which approach to use by introducing a new function (prefered) or parameter (nice idea but too many parameters)

I understand your performance concerns, but on the other side I'd still consider the missing width / height a bug and unexpected with imagecache.

I am a bit confused. How would named arguments help to avoid the current "horrible" workflow?

I think he meant they would help with the many parameters, not with the workflow of getting the width / height manually or in twig.

I think he meant they would help with the many parameters, not with the workflow of getting the width / height manually or in twig.

Alright, it will prevent this weird additional arguments:
{{ drupal_image('public://ocean.jpg', 'thumbnail', {}, true, true, 'The alternative text'|t, 'The title text'|t}) }}

Instead its something like:
{{ drupal_image(image:'public://ocean.jpg', imagecache:'thumbnail', alt:'The alternative text'|t, title:'The title text'|t}) }}

it will prevent this weird additional arguments

That was a proposal, not something that Twig Tweak has now.

The current approach is like follows.

{{ drupal_image('public://ocean.jpg', 'thumbnail', {alt:'The alternative text'|t, title:'The title text'|t}) }}

Note that image can have many more different attributes. We cannot create a dedicated arguments for each one. And using title attributes with image is generally considered as bad practice ( 🌱 [META] Discourage non-inclusive use of the HTML title attribute Active ).

Note that image can have many more different attributes. We cannot create a dedicated arguments for each one.

Well I think that misses the point! We're just talking about the 3(-4) missing attributes from image_theme()!
Any other attributes absolutely belong into #attributes! That's two different things. These attributes listed in the issue summary is what Drupal and all preprocess-hooks based on that expect as structure or at least suggest. And we should really follow that.
Totally different from all other custom #attribute values.

And using title attributes with image is generally considered as bad practice

Totally agree! Let's exclude #title attribute from the discussion, that's wrong in image_theme() and we shouldn't implement that.

So only

• width
• height
• alt

are left to implement.

Should we separate the auto-calculation of width / height into a separate issue then? Or because of the many important comments, the other way around and add the three attributes without auto-calculation in a separate issue?

Thanks for the good discussion, @Chi btw :)
I'm sure we'll find a good solution!

To summarize the challenge.

Here is the current signature of image view builder. It's already overwhelmed. There this no place to put more parameters into it.

  /**
   * Builds an image.
   *
   * @param \Drupal\file\FileInterface $file
   *   The file object.
   * @param string $style
   *   (optional) Image style.
   * @param array $attributes
   *   (optional) Image attributes.
   * @param bool $responsive
   *   (optional) Indicates that the provided image style is responsive.
   * @param bool $check_access
   *   (optional) Indicates that access check is required.
   *
   * @return array
   *   A renderable array to represent the image.
   */
  public function build(FileInterface $file, string $style = NULL, array $attributes = [], bool $responsive = FALSE, bool $check_access = TRUE): array {

To make it get more options we could drop all parameters and replace them with on object $settings that would enclose style, attributes, etc.

However, I am afraid in Twig side it'll cause bad UX. Definint multilevel objects in Twig is annoying.

As for auto-calculation image sizes I cannot think of a good way to implement it.

Expected:

  public function build(FileInterface $file, string $style = NULL, string $alt = NULL, $width = NULL, $height = NULL, array $attributes = [], bool $responsive = FALSE, bool $check_access = TRUE): array {
  [...]
}

Here is the current signature of image view builder. It's already overwhelmed. There this no place to put more parameters into it.

I disagree. These are many parameters, yes. But not too many, with named arguments. It's just what image_theme() expects. And as you wrote somewhere else

Twig Tweak is just exposing core functions to twig

. That would be the case then.

The real problem I see in this point is, that we can't simply change the function arguments, as it's already in use. And adding these parameters to the end would also not be that nice.
So the only way would be to introduce a new function and deprecate the new one.

However, I am afraid in Twig side it'll cause bad UX. Definint multilevel objects in Twig is annoying.

I agree and it mixes up the different levels! That's what named arguments are for. So it's better to use parameters than a huge array here.

As for auto-calculation image sizes I cannot think of a good way to implement it.

If we can pass the parameters manually, at least the imagecache functionality can be used. So let's split that auto-caclulation discussion into a separate issue?

Proceeding here now checking the tests and implementations. Still we need to find a way to fix this to have width / height set correctly by imagecache. At least via a separate twig_tweak function, if not in drupal_image.

@Chi: Perhaps an option would be to introduce "drupal_image_style()" with the proposed fixes and leave "drupal_image()" untouched?
That way we could fix these bugs, add the missing expected properties and still align with core.

I came to this idea from these lines in twig_tweak:

      if ($style) {
        if ($responsive) {
          $build['#type'] = 'responsive_image';
          $build['#responsive_image_style_id'] = $style;
        }
        else {
          $build['#theme'] = 'image_style';
          $build['#style_name'] = $style;
        }
      }
      else {
        $build['#theme'] = 'image';
      }
    }

(twig_tweak/src/View/ImageViewBuilder.php)

Of course I'd also be happy with any other appropriate fix, but not having width / height from imagecache still is no option from my perspective for pages with SEO relevancy.

(The failing test is now unrelated, as in 🐛 Wrong preg_replace in ImageViewBuilderTest removes relevant attributes Fixed )

The last submitted patch, 33: 3356042-image-style-width-height-missing-TEST-ONLY-33.patch, failed testing. View results →
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

@Chi: Just added the following part to the issue summary to make clear, why the current implementation is not sufficient in large parts:

Drupals Imagecache / Imagestyle handling needs these parameters to resize the images keeping the aspect ratio: See Image::scaleDimensions()

So besides missing width / height attributes this also means that imagecache can't do its work!

How can we proceed here?

Anybody → closed merge request !26

I've split-off the "alt" attribute. With this implementation we should discuss if adding width / height attribute as parameter is still required and helpful (for overwriting). I think it's not, but overwriting in such cases should work using the attributes.

One last issue I wasn't able to fix, at least locally:

Testing Drupal\Tests\twig_tweak\Kernel\ImageViewBuilderTest
.                                                                   1 / 1 (100%)

Time: 00:01.004, Memory: 6.00 MB

OK (1 test, 15 assertions)

Unsilenced deprecation notices (2)

  2x: realpath(): Passing null to parameter #1 ($path) of type string is deprecated
    2x in ImageViewBuilderTest::testImageViewBuilder from Drupal\Tests\twig_tweak\Kernel

Remaining self deprecation notices (1)

  1x: realpath(): Passing null to parameter #1 ($path) of type string is deprecated
    1x in ImageViewBuilderTest::testImageViewBuilder from Drupal\Tests\twig_tweak\Kernel

Failed to run phpunit.sh tests/src/Kernel/ImageViewBuilderTest.php: exit status 1

Static patch attached, if anyone needs it so far. It's working, just the test issue from #44 left.

The last submitted patch, 46: 3356042-46.patch, failed testing. View results →

Fixed the tests, all green now! 🙂👍

Please review!

Great work, thank you @Grevil! Ready for the GO, working perfectly as expected now.

Hi, do you have any estimate when this is going to be released?
Thank you

We are not maintainers unfortunately, we need to wait for @chi to make a proper release here.

Submitted a review. Thank you.

Adjusted the code based on your comments, just one comment is left unresolved.

Refactored a bit so when width or height attributes are given the image toolkit is not involved.

Thank you.

Thanks @Chi! Great to see this fixed :)

@Chi I think I found a fatal error regression: 🐛 Regression: Fatal error: Cannot use Drupal\Core\Image\ImageFactory as ImageFactory because the name is already in use in /var/www/vhosts/precima.de/precima.de/web/modules/contrib/twig_tweak/src/View/ImageViewBuilder.php on line 9 Active

I'll take a deeper look now!

@Chi sorry false alert, something seems to have gone wrong on our side. All good, code looks fine! The error didn't make sense code-wise.

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