Do not fetch default image toolkit ID in ImageFactory::__construct()

Created on 1 July 2025, about 2 months ago

Problem/Motivation

Fetching the default toolkit means loading config, we shouldn't do that in a constructor.

With DI, the ImageFactory is injected into hooks, plugins and preprocess stuff even though it might not actually be required in a given scenario.

Steps to reproduce

Proposed resolution

Remaining tasks

User interface changes

Introduced terminology

API changes

Data model changes

Release notes snippet

πŸ“Œ Task
Status

Active

Version

11.0 πŸ”₯

Component

image system

Created by

πŸ‡¨πŸ‡­Switzerland berdir Switzerland

Live updates comments and jobs are added and updated live.
  • Performance

    It affects performance. It is often combined with the Needs profiling tag.

Sign in to follow issues

Merge Requests

Comments & Activities

  • Issue created by @berdir
  • Merge request !12563lazy initialize of toolkitId β†’ (Closed) created by berdir
  • πŸ‡¨πŸ‡­Switzerland berdir Switzerland

    The problem is that this breaks BC for subclasses, of which there about 3 and I think at least the brandfolder one would be broken: http://codcontrib.hank.vps-private.net/search?text=extends%20ImageFactor...

  • Pipeline finished with Failed
    about 2 months ago
    Total: 491s
    #535841
  • πŸ‡ΊπŸ‡ΈUnited States smustgrave

    Seemed to break some performance tests.

    Anyway to cover with a deprecation. Like if Core version is 11.3 or great trigger a deprecation that toolkitId won't be set?

  • πŸ‡¨πŸ‡­Switzerland berdir Switzerland

    This is better than I expected, as it saves not just the config but also the toolkit lookup and on a cache miss discovery on a regular request and it doesn't just postpone it, it should actually move it out completely to the actual image style request.

    It's "only" two fast chained apcu cache lookups, but still.

    > Anyway to cover with a deprecation. Like if Core version is 11.3 or great trigger a deprecation that toolkitId won't be set?

    The problem is that it we still use the property but we switch to lazy initialize. There's still the setter as well, we can't drop that completely (although having the setter at all seems somewhat problematic as the service has then state and it could change. But that's out of scope.

    The only idea I have is that we could introduce a new property and deprecate the old. But that's a quite a bit of complexity. I've already created an issue in the brandfolder project: πŸ“Œ Core change to ImageFactory might break this Active .

  • Pipeline finished with Success
    about 2 months ago
    Total: 592s
    #539673
  • πŸ‡ΊπŸ‡ΈUnited States nicxvan

    I'd create an issue in the other two add well just for visibility.

    This needs significant from someone, I'm not sure who, but it seems like a low risk BC break.

  • πŸ‡¨πŸ‡­Switzerland berdir Switzerland

    The other two do not use $this->toolkitId as far as I see, so they don't require a change.

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

    Another dumb question if it's a breaking change should we wait for D12? Or think 11.3 is fine?

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

    It affects one project with an issue and a super easy BC compatible fix, I think we're safe to move forward.

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

    In that case lets do it! Fingers crossed

  • πŸ‡¬πŸ‡§United Kingdom catch

    I think this is OK in a minor - if we add a change record and release note for it. The release note can point to the contrib issue.

  • πŸ‡¨πŸ‡­Switzerland berdir Switzerland

    Like this?

  • πŸ‡¨πŸ‡­Switzerland berdir Switzerland
  • πŸ‡¨πŸ‡­Switzerland berdir Switzerland
  • Pipeline finished with Success
    4 days ago
    Total: 786s
    #575896
  • πŸ‡ΊπŸ‡ΈUnited States nicxvan

    I updated the change record title to be more descriptive of the action.

    I also updated the release notes to match the expected formatting here: https://www.drupal.org/about/core/policies/maintainers/release-notes#s-f... β†’

    • catch β†’ committed 81df3e9c on 11.x
      Issue #3533322 by berdir, smustgrave, nicxvan: Do not fetch default...
  • πŸ‡¬πŸ‡§United Kingdom catch

    I double checked brandfolder and there are only ~5 sites running the Drupal 11 compatible version of the module (and it also supports Drupal 10 so they may still be on Drupal 10 too), so I think we are definitely OK with the change record and issue. The getter has been around for ages and the bc policy excludes protected properties of non-base classes (and especially the values of those properties), can't think of a less intrusive way to make the change.

    Side note: this is one of the first issues where it's affecting performance tests (intentionally) and I had essentially zero involvement at all, so I actually got to see test changes without any previous context or knowledge of the issue, it's nice to see how the coverage shows the relative improvements between different scenarios, see the query diff etc. - so much easier to understand the consequences. Also don't think this would have been obvious to find via profiling, it's really the database query that makes it obvious what's saved.

    Committed/pushed to 11.x, thanks!

Production build 0.71.5 2024