- Issue created by @heddn
- 🇬🇧United Kingdom catch
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.
- 🇬🇧United Kingdom longwave UK
Could we render the logo alone in its own template, preprocessing the height/width, and leverage the render cache?
- 🇬🇧United Kingdom catch
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.
- Status changed to Needs work
about 1 year ago 9:17am 29 March 2024 - 🇬🇧United Kingdom catch
Made a start on this but it's quite painful.
The pain is mostly because of
system_preprocess_block
andcore/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.
- Merge request !7241Beginnings of rendering image normally in the branding block. → (Open) created by catch
- Status changed to Needs review
about 1 year ago 12:06pm 29 March 2024 - 🇬🇧United Kingdom catch
Deprecated the 'site_logo' variable, adding a change record and a release note, this ought to be reviewable now.
- Status changed to Needs work
about 1 year ago 12:31pm 29 March 2024 - 🇬🇧United Kingdom catch
That svg logic isn't happy, removed that and also switched to using the image factory for non-svg images.
- First commit to issue fork.
- 🇺🇸United States nmangold United States
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.
- 🇺🇸United States smustgrave
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
- 🇺🇸United States nmangold United States
Added a screenshot of the logo as an inline SVG.
- 🇺🇸United States nmangold United States
nmangold → changed the visibility of the branch 3345259-logo-image-dimensions to hidden.
- 🇺🇸United States nmangold United States
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 additionalsite_logo_rendered
for rendering the<img>
tag with attributes.Also, I added tests covering both situations.
- 🇺🇸United States nmangold United States
Added a screenshot of the logo as a PNG, which renders the
<img>
tag with attributes including dimensions. - 🇺🇸United States nmangold United States
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.