Theme logo image is rendered without dimensions

I wonder if this was an unexpected regression from #908282: Remove unnecessary I/O from theme_image() (although that issue was good in general).

The main issue with this is that we don't have anywhere obvious to store the dimensions, the theme settings just point to a file on disk, there's no file reference or similar.

We could do what #908282: Remove unnecessary I/O from theme_image() used to do in preprocess and cache it somewhere maybe? It's a bit hacky, but it's minimal and would help page rendering quite a lot.

Or we could add dimensions to the theme configuration form/config object and prepopulate them from the file info if not explicitly set? This would have the advantage of not needing an extra cache get but you could end up with weird issues if you change your theme logo file without updating the form.

Could we render the logo alone in its own template, preprocessing the height/width, and leverage the render cache?

Storing it in config feels like the wrong solution given it is not configuration and we can't guarantee it will be updated correctly.

I couldn't remember where the logo is rendered, but it's in SystemBrandingBlock - we could do it directly in there I think, just add the attributes when building the render array. It might even be OK to rely on render caching for of the block, although would need to check that.

Made a start on this but it's quite painful.

The pain is mostly because of system_preprocess_block and

core/modules/system/templates/block--system-branding-block.html.twig

In SystemBrandingBlock we do things 'properly' and use '#theme' => 'image', but then that preprocess and the template just extracts the URI from the image render array and renders the image with hard-coded HTML. Why?

This means any added attributes get stripped.

Promoting to a bug because there's no way at all to do this generically at the moment.

I got around this in stark by adding an extra variable for the rendered image, then using that in the template, it is really ugly but I can't think of any other way except adding an entirely new block and deprecating the old one.

Deprecated the 'site_logo' variable, adding a change record and a release note, this ought to be reviewable now.

That svg logic isn't happy, removed that and also switched to using the image factory for non-svg images.

Trying to remove the 'See more' from the snippet

This won't make it into 10.3.

The current MR, https://git.drupalcode.org/project/drupal/-/merge_requests/7241, dumps "boo" to the page. Consequently, the height and width attributes are not added to the image. This is because SVG is not a supported type in GDToolkit. The image factory method $image->isValid() returns false. I assume that is why the URI is extracted and used in the hard-coded <img> tag instead of the render array.

I created a new MR, https://git.drupalcode.org/project/drupal/-/merge_requests/11538, that proposes checking the extension and printing the SVG inline, which includes the height and width attributes.

So currently there are now 2 MRs, 1 should be hidden or closed

Also issue summary appears to be complete.

Will probably need test coverage

Added a screenshot of the logo as an inline SVG.

Now that I better understand this, I added code similar to the previous, hidden branch. The default logo in the themes is now an SVG. However, a non-SVG image could be configured or uploaded using the theme settings. Some of the tests also use the old Druplicon logo.

For SVGs, we cannot render the image using the image factory, because GDToolkit does not support SVGs. So, we need both solutions, one for images supported by GDTookit and one for images not supported (SVGs).

I changed the release notes for deprecating the site_logo and described the additional site_logo_rendered for rendering the <img> tag with attributes.

Also, I added tests covering both situations.

One question on the MR.

Added a screenshot of the logo as a PNG, which renders the <img> tag with attributes including dimensions.

I removed the release notes. The new variable is no longer used. The extension logic was moved to the block plugin which allows the templates to simply print the site_logo variable.

I believe everything is done, and this is ready for review again.

This seems like it's going to be immediate breaking change for many sites using their own branding-block twig..

@smustgrave You are correct. The site_logo variable now renders the entire <img> or <svg> element. Not just the URI. I suppose that should be in the release notes.

I think this needs some kind of backwards compatibility change. Definitely can't release in patched release or even minor release. So as is I don't think it should be included till Drupal 12

That might be best. Unless someone can suggest a backwards compatibility change.

I changed the release notes to reflect the change as of now.

Posted in #core-development to hopefully get some attention.

Improved the release notes.

We can change templates in minor releases with a change record and release note, see https://www.drupal.org/about/core/policies/core-change-policies/frontend...

@catch Can you review the MR, please?

Hi all, I was able to reproduce the issue by following the outlined steps, and I observed that the site branding logo was being rendered without its height, width, and loading attributes. I’ve applied the provided MR !1158 as a patch, and it applied cleanly. After applying the patch, the site branding logo is now rendered with height, width, and loading attributes for PNG, JPG, and SVG files, which was previously missing. Previously, only the src and alt attributes were rendered for the logo. The MR changes are working fine, and I’ve attached screenshots showing the issue before and after the fix for reference.

Additionally, with the new MR changes, $site_logo now returns an array containing the image elements with their properties instead of just the URI. This change may potentially cause layout issues for themes that use their own branding logo system or preprocess the branding block template.

Although the MR changes are working well with D11, I’m keeping the issue in "Needs work" state since the pipeline is failing and to consider backward compatibility.

I can confirm that MR11538 applies to D11.2.2
I have a custom subtheme of the bootstrap5 theme, so I had to overwrite themes/contrib/bootstrap5/templates/block/block--system-branding-block.html.twig in my custom theme. In there I replaced:

  {% if site_logo %}
    <a href="{{ path('<front>') }}" title="{{ 'Home'|t }}" rel="home" class="site-logo d-block">
      <img src="{{ site_logo }}" alt="{{ 'Home'|t }}" fetchpriority="high" />
    </a>
  {% endif %}

with:

  {% if site_logo %}
    <a href="{{ path('<front>') }}" title="{{ 'Home'|t }}" rel="home" class="site-logo d-block">
      {{site_logo}}
    </a>
  {% endif %}

At all works fine.

It seems that MR11538 no longer applies to D11.2.2 since the changes in #38.

I digged a little further and you seem to have core/modules/system/src/Hook/SystemThemeHooks.php which I dont see in my project.

ubuntu@MYMACHINE:~/projects/myproject$ git apply --reject 11538.diff --directory=web
Checking patch web/core/modules/system/src/Hook/SystemThemeHooks.php...
error: web/core/modules/system/src/Hook/SystemThemeHooks.php: No such file or directory
Checking patch web/core/modules/system/src/Plugin/Block/SystemBrandingBlock.php...
Checking patch web/core/modules/system/templates/block--system-branding-block.html.twig...
Checking patch web/core/modules/system/tests/src/Functional/System/ThemeTest.php...
Checking patch web/core/themes/olivero/templates/block/block--system-branding-block.html.twig...
Checking patch web/core/themes/starterkit_theme/templates/block/block--system-branding-block.html.twig...
Applied patch web/core/modules/system/src/Plugin/Block/SystemBrandingBlock.php cleanly.
Applied patch web/core/modules/system/templates/block--system-branding-block.html.twig cleanly.
Applied patch web/core/modules/system/tests/src/Functional/System/ThemeTest.php cleanly.
Applied patch web/core/themes/olivero/templates/block/block--system-branding-block.html.twig cleanly.
Applied patch web/core/themes/starterkit_theme/templates/block/block--system-branding-block.html.twig cleanly.

Even more, I do not see this file in the D11.2.x repository:
https://git.drupalcode.org/project/drupal/-/tree/11.2.x/core/modules/sys...

BUT it is in the D11.x repository:
https://git.drupalcode.org/project/drupal/-/tree/11.x/core/modules/syste...

I checked the version this patch is against and you are correct and I am wrong. It should not apply to D11.2.2, it looks like it should apply to D11.x which unfortunatly my project dont have. I believe that the MR modification in SystemThemeHooks.php was added after #37, which is why I was able to apply the MR to D11.2.2 at the time of #37 but now I cant anymore

I cut the SystemThemeHooks.php out of the MR to have a patch that applies to D11.2.2