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

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

Could it get rebased please?

Re-rolled.

Now all tests are green.

@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.

Rebased the MR onto the latest 3.x.
@mirakolous, you should now be able to apply the MR's plain diff to 3.x-dev again.

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.

Rebased.
@sagesolutions, you should be able to test this now against 3.x-dev.

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! 🙂

All feedback has been addressed, except for one thing where @mably requests input from @Grevil. Setting back to NR.

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

Addressed feedback by @grevil.

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?

Is there an open issue for this already?

Not yet. I had a quick look at this yesterday and was able to reproduce. I also have a fix ready.
Will create a new issue with that fix soon.

@mably, I will also have a look into ✨ Use a trait to reduce code duplication in formatters Active .

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 override the #url property when we override the #theme property and set it 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.

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

Oops, missed that. I created 🐛 Fatal error when linking SVGs to file Active in the meantime, but I'll be happy to add the fix to this issue if that is preferred.

Created a new MR that addresses all open issues.
Not sure what the right status should be now 🙈

mably → changed the visibility of the branch 3257729-formatters-shouldnt-repeat to hidden.

mably → changed the visibility of the branch 3257729-formatters-shouldnt-repeat to active.

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.

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

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!