- Issue created by @berdir
- π¨πSwitzerland berdir Switzerland
Reducing scope to image module. there are several, in different include files, so that seems enough on its own.
Not 100% sure on BC for the functions in the include files, there might be some edge cases where that might fail anyway, but kept the pattern for now and moved them to .module.
Used the new closure pattern for the injected logger service, makes it a tiny bit more complicated, but seems like a good compromise for the typical logger-used-in-rare-case scenario and we'd need an Attribute anyway for autowiring.
- π¨πSwitzerland berdir Switzerland
The performance test fail confused me, I thought we somehow no longer load a config or I had the wrong config name. But that's not it, it's the opposite. Because this is now injected here, it's loaded already on the initial user/login request that warms up bootstrap caches and it's cached afterwards.
I opened π Do not fetch default image toolkit ID in ImageFactory::__construct() Active for this.
- π¨πSwitzerland berdir Switzerland
Updated the comment references in templates that I forgot before. I'd prefer doing the issue in #4 first to avoid the misleading performance test change here.
- πΊπΈUnited States smustgrave
Are we doing CRs for all the twig templates that would have to be updated on contrib modules?
- πΊπΈUnited States smustgrave
This one has 5 twig template it appears to be updating the @see for.
- πΊπΈUnited States nicxvan
We've not done CRs on other issues like this for that, I don't think this really warrants one since it's just a comment.
- π¨πSwitzerland berdir Switzerland
Initially I considered to drop this pattern with the @see referencing the template/initial preprocess, but got some resistance on that from theme maintainers, so I gave up on that, at least in this scope. It's similar with the docs in twig templates in general, some of these templates have extensive docs that are copy pasted around and tend to get outdated. The default node.html.twig for example has 60 lines of comments and 20 lines of actual markup. And that whole thing exists 11 times in core. And it is at least partially outdated, there's a whole block on default classes, which in fact do not exist but are set within the twig template itself in some cases and not others. Many of usprobably using IDE's or editors that hide that by default and don't even notice it anymore. But I also don't have a great idea on what else we should do. It can be useful for people who are new to Drupal and theming I guess.
- πΊπΈUnited States smustgrave
Yea not sure what the best approach is. I know when I override a template I usually copy the whole thing, comments included for the next dev.
What if we had just a generic overall CR for themers to update their templates to use these classes?
- π¨πSwitzerland berdir Switzerland
We have a generic change record: https://www.drupal.org/node/3504125 β . but I don't see the point of telling people to update the @see. Nobody is going to do that, that's a lot of tedious work for no real gain. Yeah, in 5 years, if someone looks at a template they won't find that function anymore. But they can just look at the source template, that takes a few seconds, far less time than manually updating all templates.