- Issue 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...
- πΊπΈ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 .
- πΊπΈ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 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.