Formatters shouldn't repeat core code

Rebased against 3.x, here is the diff: https://git.drupalcode.org/project/svg_image/-/merge_requests/18.diff

There is an issue:

svg_image_responsive/src/Plugin/Field/FieldFormatter/SvgResponsiveImageFormatter.php
$linkFile in isset() is never defined

Pointing out @mably's comment here, as some people don't get mail notifications through gitlab:

Could it get rebased please?

@mrshowerman thanks for the rebase.

The suggested improvement seems to executes some code twice like;

$images = $this->getEntitiesToView($items, $langcode)

which gets executed in the parent first and then is executed a second time in the child formatter.

So I'm not sure the gain is really worth it.

Let's see what others think about it.

This code is executed twice because it looks like we need $images in the overriding method, to loop on it. It seems there's no clean way to do otherwise without re-implementing the parent method. Ideally, the parent method might be split into several ones so that we don't have to make that second call, but from a contrib module point of view, we can't change core code.

Yes, this is not perfect because it could have a negative impact on performance.

BUT performance is not the only criteria. I this case, I suspect the second call will be very fast and lead to no additional SQL query thanks to various Drupal cache layers. Additionally, the result of this execution can also be cached (render caching).
So I'd say we can trade a tiny performance degradation to get a contrib module that has much more chance to work well with subsequent core versions.

Sharing in case anyone else runs into this. The current patch is behind and does not apply. I attempted to rebase, but it was too many conflicts for me to resolve confidently. In order to not stall the updates to other modules, I had to change svg_image in composer.json like so:

"drupal/svg_image": "dev-3.x#2fe16c7a9f0c511bbcaa44499a4fb61fa0f9854c",

That is the last hash that this patch applies cleanly to.

@mirakolous The patch generated from the MR (https://git.drupalcode.org/project/svg_image/-/merge_requests/18.diff) applied on 3.0.1 some days ago.

I could not apply the MR #18 diff to any logical branch. I tried version 3.0.2, version 3.1.0, dev-3.x#2fe16c7a9f0c511bbcaa44499a4fb61fa0f9854c, and the latest 3.x-dev branch.

Patching all of them failed.

Tested on the 3.x-dev branch and Image loading attributes are now working properly. Thanks @mrshowerman!

Changes LGTM! Could you add a few explanatory comments in the code?

Some of the code is not straight forward. Adding comments would speed up understanding the code in the future! 🙂

Resolved most threads, but one! Then we can merge this :)

Merged! Thanks for all the great work.

just discovered after merging that having configured a link to the image in the responsive image display formatter generates a WSOD.

Is there an open issue for this already? What are the steps to reproduce this?

Thanks @mrshowerman !

The problem is that ImageFormatter and Responsive ImageFormatter render the link differently:

image-formatter.html.twig:

{% if url %}
  {{ link(image, url) }}
{% else %}
  {{ image }}
{% endif %}

responsive-image-formatter.html.twig:

{% if url %}
  <a href="{{ url }}">{{ responsive_image }}</a>
{% else %}
  {{ responsive_image }}
{% endif %}

So we just need to also override the #url property when we override the #theme property to image_formatter in the SvgResponsiveImageFormatter class.

@mrshowerman not sure we need to create a new issue for that, just open a new MR here with the fix.

I agree with @mably. I originally thought, that the problem was unrelated based on:

Actually it fails with release 3.1.0 too, so most probably not related to this MR

Fine if we create a follow-up fix here!

Yep, removed that comment in Gitlab as it is working with 3.1.0 in fact.

Should be an easy fix.

Merged. Thanks for the quick fix @mrshowerman!

Great stuff! Thanks all! :)

I guess, we should create a new release pretty soon! Let's wait a couple of days, just in case we find anything else.

Just wondering if there's an ETA for the next release with this since it includes the image attribute fix as well?

Great work to all involved by the way!

@nkind version 3.2.0 including this fix has been released!

@mably Thank you!