streamline responsive_image_build_source_attributes()

Patch mostly moves the "image dimensions" logic, currently split between template_preprocess_responsive_image() and responsive_image_build_source_attributes() (which then just redoes the same logic for all source tags), all the way into the former.

This makes it clearer that responsive_image_build_source_attributes() only needs a $dimensions array with 'width' and 'height', not the full $variables array from template_preprocess_responsive_image() with lots of other stuff it doesn't use.

Then, this clarifies that I don't understand that logic to begin with :-) :
- We allow calling #theme 'responsive_image' passing forced, pre-defined width and height.
- the beginning of template_preprocess_responsive_image() has a comment saying "if present, we'll output them", but that seems misleading: unlike what theme_image() does with "flat" imgs, we don't output those as 'width' / 'heigth' attributes in the HTML
- Those dimensions are forced as "the dimensions of the original image" to compute the resulting pixel width of the various image derivatives used in the srcset.

So I'm confused why would you would want to pass arbitrary dimensions there, other than the ones of the actual original image ? That sounds like it would totally mess the resulting widths in the srcset ?

Patch then does a couple other cleanups I found along the way:
- Adds a couple comments for balance, and adjust those about the "fallback image"
- Visually aligns the two mappings added in the code sample in the phpdoc for responsive_image_build_source_attributes()
- in that same code sample, changes the 'sizes_image_styles' to a simple numeric array without the duplicate key/values (the config schema says it's a sequence, so it's the only thing that's ever going to get saved anyway ?)
- Fixes the @return doc (a single Attribute object rather than an array now)

Oh, of course, patch is rolled on top of #2260061: Responsive image module does not support sizes/picture polyfill 2.2, so it's out of reach for the tesbot for now.

#2260061: Responsive image module does not support sizes/picture polyfill 2.2 got committed (YAY!), hence this is now unblocked :)

Let's see if the patch applies.

Seems to apply OK after all :-)

Side note: #2424697: ResponsiveImageFormatter throws an exception on node preview
This predates #2260061: Responsive image module does not support sizes/picture polyfill 2.2, but the right people are likely subscribed here, so, shameless plug :-)

Assigning to @attiks (I can't assign to @Jelle_S — do you want to officially become a maintainer of the responsive_image module, Jelle_S? :)) to gather a reply to @yched's questions in #1.

@Wim I'd be honoured!

So I'm confused why would you would want to pass arbitrary dimensions there, other than the ones of the actual original image ? That sounds like it would totally mess the resulting widths in the srcset ?

You're absolutely right! It's a leftover from old code. When I first wrote theme_picture() (back when it was still picture.module) I started off with theme_image() as the base code. That in combination with some old outdated specs of the <picture> and <source> tags resulted in the code using the passed dimensions. It should be removed now. I missed it in the big "Responsive Image Cleanup Patch" (#2260061: Responsive image module does not support sizes/picture polyfill 2.2), sorry about that...

So I guess that makes this NW.

@Jelle_S thanks for the pointer.

Something like this then ?

[edit; interdiff is in #14]

Although, looking at how things work for "flat" image styles, it looks like theme 'image_style' *requires* passing the image dimensions.
template_preprocess_image_style() just takes those and directly passes them to $style->transformDimensions() without trying to figure them out from the original image.

Maybe we should just do the same for consistency ?

Oops, forgot the interdiff for #12

@attiks : Actually, theme 'image_style' / template_preprocess_image_style() work the way they do (require the caller to pass the source image dimensions, don't bother trying to read them from the file) because the dimensions are stored in ImageItem field values (see ImageItem::preSave()), and are thus already available.

Displaying a derivative image in an Image field formatter is the 95% use case for image styles output, other use cases need to fetch the image dimensions themselves before calling theme 'image_style'.

So if we did #15 for responsive image styles, that would be a perf regression, because template_preprocess_responsive_image() would perform i/o operations to read the image dimensions from the file while we already have them available in the ImageItem.
That would also be the opposite of how theme 'image_style' works. Keeping image_style and responsive_image_style consistent is IMO a cognitive and maintenance win.

Attached patch:
- Requires image dimensions to be passed, like theme 'image_style' does,
- Then, no need to go through the Image object to grab the extension, we can call pathinfo() on $variables['uri']. This way, no need to go through the 'image.factory' service to create an Image object, the information passed by the caller are enough.
- Additional doc fix : AFAICT the 'image_style' param for theme_responsive_image_formatter() is not optional, template_preprocess_responsive_image() assumes there is one.

Side note: looking at the code at this point, it might make sense to turn responsive_image_build_source_attributes() back into "handle all mappings" - thus reverting one of the last refactors I suggested in #2260061: Responsive image module does not support sizes/picture polyfill 2.2 ;-). The logic around "dimensions" was a bit more confusing back then...

Also : opened #2426757: responsive_image_build_source_attributes() calls mime type guesser over and over

Doh.
- arguments order mismatch
- there was still code that used the $image object to get the uri.

The # of parameters for responsive_image_build_source_attributes() make more arguments for #17. Let's get back to green first.

We need a reroll on this. It looks like this was mostly ready and just needed some reviews? Would be good to get this cleaned up.

Created reroll.

Now that #2123251: Improve DX of responsive images; convert theme functions to new #type element has been committed, this will need a reroll. Some of this may no longer be necessary since the DX was changed around a fair amount in that patch.

Rerolled the patch.

As said in #16: Passing the width and height is now required for theming responsive images.

Updated the IS, added a beta evaluation.

Also, additional cleanup of a couple stale "use" statements, no need for ImageInterface anymore :-)

1. +++ b/core/modules/responsive_image/responsive_image.module
   @@ -204,25 +200,22 @@ function template_preprocess_responsive_image(&$variables) {
     * can contain image style mappings of mixed types (both 'image_style' and
     * 'sizes'). For example:
     * @code
   - * $responsive_img_style = ResponsiveImageStyle::create(array(
   + * $responsive_img_style = ResponsiveImageStyle::create([
     *   'id' => 'style_one',
     *   'label' => 'Style One',
     *   'breakpoint_group' => 'responsive_image_test_module',
   - * ));
   - * $responsive_img_style->addImageStyleMapping('responsive_image_test_module.mobile', '1x', array(
   + * ])
   + * ->addImageStyleMapping('responsive_image_test_module.mobile', '1x', [
     *   'image_mapping_type' => 'image_style',
     *   'image_mapping' => 'thumbnail',
   - * ))
   - * ->addImageStyleMapping('responsive_image_test_module.narrow', '1x', array(
   + * ])
   + * ->addImageStyleMapping('responsive_image_test_module.narrow', '1x', [
     *   'image_mapping_type' => 'sizes',
   - *   'image_mapping' => array(
   + *   'image_mapping' => [
     *     'sizes' => '(min-width: 700px) 700px, 100vw',
   - *     'sizes_image_styles' => array(
   - *       'large' => 'large',
   - *       'medium' => 'medium',
   - *     ),
   - *   ),
   - * ))
   + *     'sizes_image_styles' => ['large', 'medium']
   + *   ],
   + * ])
     * ->save();
   

   Changing to the short array syntax is out of scope and makes the patch harder to review.

2. +++ b/core/modules/responsive_image/responsive_image.module
   @@ -311,26 +304,23 @@ function template_preprocess_responsive_image(&$variables) {
   -function responsive_image_build_source_attributes(ImageInterface $image, array $variables, BreakpointInterface $breakpoint, array $multipliers) {
   ...
   +function responsive_image_build_source_attributes($uri, array $dimensions, $extension, BreakpointInterface $breakpoint, array $multipliers) {
   

   Why change the $image to $uri - seems unnecessary.

3. +++ b/core/modules/responsive_image/responsive_image.module
   @@ -371,13 +361,13 @@ function responsive_image_build_source_attributes(ImageInterface $image, array $
   -  $source_attributes = new \Drupal\Core\Template\Attribute(array(
   +  $source_attributes = new Attribute(array(
   

   Out of scope

I'm not sure I see the point of this issue - since nothing that uses the responsive_image template is changing I assume that everything already provides dimensions - and therefore will this actually have any performance impact. There does not seem to be any proof of this impact on the issue.

1. reverted the array() / [] changes (but kept the re-alignment of the two calls to addImageStyleMapping() in the code sample)
2. $image doesn't exist anymore, the point of this patch is that there is no need to create an ImageInterface object :-)
3. sure - but unlike the array() / [] changes mentioned in 1., it hardly complicates patch review ? Temptatively leaving that in.

since nothing that uses the responsive_image template is changing I assume that everything already provides dimensions - and therefore will this actually have any performance impact. There does not seem to be any proof of this impact on the issue

Yes, only the ResponsiveImageFormatter currently calls 'responsive_image' in core, and it does already pass the dimensions, and thus does not need any change.
But even in that case, template_preprocess_responsive_image-) currently needlessly creates an Image object, and responsive_image_build_source_attributes() repeatedly calls getDimensions() on it [edit: sorry, that was a little hasty. Current code doesn't read $image dimensions if they are already passed in. But the $image is not needed to begin with]

Reroll.

Minor fix (forgot to pass the $uri argument to responsive_image_get_image_dimensions). Let's see what the bot thinks (assuming the bot is fixed by now...)

I think you mixed in some work from another issue. There are changes to the template here that I don't think belong.

Oops! You're right, my bad. That patch is for an entirely different issue. Good to know it's green though ;-) . New patch without the code from the other issue.

Might be worth looking to see if any of these improvements could go in under the hood in 8.1.

Would be great if we could get this going again. No longer applies to 8.1.x

Hi guys,

reroll is ready

Thanks! :)

Checking for errors

>>fail: [PHP Fatal error] Line 402 of core/modules/responsive_image/responsive_image.module:
>> Class 'Attribute' not found

fixed

interdiff and patch added

Code looks great, +1 rtbc here
But there's API change and #36 points about scope...

+++ b/core/modules/responsive_image/responsive_image.module
@@ -138,25 +138,19 @@ function template_preprocess_responsive_image_formatter(&$variables) {
- *   - width: The width of the image (if known).
- *   - height: The height of the image (if known).
+ *   - width: The width of the image.
+ *   - height: The height of the image.

@@ -344,27 +337,23 @@ function template_preprocess_responsive_image(&$variables) {
-function responsive_image_build_source_attributes(ImageInterface $image, array $variables, BreakpointInterface $breakpoint, array $multipliers) {
...
+function responsive_image_build_source_attributes($uri, array $dimensions, $extension, BreakpointInterface $breakpoint, array $multipliers) {

this requires change record with example code for contrib maintainers

So on the one hand responsive_image_build_source_attributes() looks like just a helper for the preprocess that should have an an underscore on it.

On the other hand it's got very extensive documentation that makes it look like an API function, and it doesn't have an underscore.

So probably we should add a new method, and if possible have responsive_image_build_source_attributes() call that method? Shame to duplicate the code unless we really have to, since this probably should never have been an API function in the first place.

#59 sounds good to me. I'd also explicitly deprecate responsive_image_build_source_attributes() with that approach.

Unassigning because it has been 5 years since vprocessor was able to work on this issue.

This already had release manager review - we should do this in bc-friendly way.

Rerolled the patch for current version and attaching the reroll diff.

Fixed issues with the above patch

Reroll the patch #79 with Drupal 10.1.x.

Rerolled patch #81, please review it.

Fix Custom command failure for #83