Convert template_preprocess functions in image and file modules

Created on 30 June 2025, 16 days ago

Problem/Motivation

Part of πŸ“Œ [pp-1] Convert Template Preprocess hooks to OOP equivalent Postponed: needs info

Steps to reproduce

Proposed resolution

Remaining tasks

User interface changes

Introduced terminology

API changes

Data model changes

Release notes snippet

✨ Feature request
Status

Active

Version

11.0 πŸ”₯

Component

theme system

Created by

πŸ‡¨πŸ‡­Switzerland berdir Switzerland

Live updates comments and jobs are added and updated live.
Sign in to follow issues

Merge Requests

Comments & Activities

  • Issue created by @berdir
  • Merge request !12554convert template preprocess in image module β†’ (Open) created by berdir
  • Pipeline finished with Failed
    16 days ago
    Total: 323s
    #535595
  • πŸ‡¨πŸ‡­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.

  • Pipeline finished with Failed
    16 days ago
    Total: 3431s
    #535597
  • πŸ‡¨πŸ‡­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.

  • Pipeline finished with Success
    16 days ago
    Total: 641s
    #535837
  • πŸ‡¨πŸ‡­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.

  • Pipeline finished with Failed
    11 days ago
    Total: 644s
    #540188
  • πŸ‡ΊπŸ‡ΈUnited States smustgrave

    Are we doing CRs for all the twig templates that would have to be updated on contrib modules?

  • πŸ‡ΊπŸ‡ΈUnited States nicxvan

    What twig templates would need to be updated?

  • πŸ‡ΊπŸ‡Έ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.

  • Pipeline finished with Failed
    3 days ago
    Total: 600s
    #547075
Production build 0.71.5 2024