Come together with the global Drupal community in Rotterdam, 28 Sept – 1 Oct 2026. Sessions, contribution, connection, and Early Bird savings until 8 June.Problem/Motivation
Multiple contrib modules are broken since ImageStyleDownloadController::deliver() now takes a string of the expected scheme for derivatives as a required parameter
Steps to reproduce
AVIF: #3450240: Drupal 10.3 compatibility
Stage File Proxy: #3450243: Drupal 10.3 compatibility
WebP: Probably also broken
Proposed resolution
Provide default value and throw a deprecation error.
public function deliver(Request $request, $scheme, ImageStyleInterface $image_style, ?string $required_derivative_scheme = null) {
if ($required_derivative_scheme === NULL) {
// @todo throw a deprecation.
$required_derivative_scheme = $scheme;
}
// ...
}Remaining tasks
User interface changes
API changes
Data model changes
Release notes snippet
Issue fork drupal-3450244
Show commands
Start within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
- 3450244-imagestyledownloadcontroller
changes, plain diff MR !8200
Comments
Comment #3
mstrelan commentedComment #4
cmlara(Bringing Slack notes into the issue)
Lack of BC was intentional.
As noted in #3298701: ImageStyleDownloadController routes do not limit schemes served the change was for security reasons related to SA-CORE-2022-012.
The class and method in question are 100% internal. Per Drupal API policy no one should be extending, calling, or otherwise using the method unless they are willing to accept breaking changes in point releases.
A lot of this happens in Drupal Core with module maintainers not realizing that their Core Compatibility should actually be "<=X.Y.Z" or at most "~X.Y.0" rather than the much more common "^X"
Setting back to NW to prevent an accidental fast-track of this into core.
I'm going to take a moment to plug #2986669: Split ImageStyle into the config entity and a separate event-based image processing service as a likely long term solution for many modules that are currently providing their own ImageStyleDownloadController class.
Comment #5
mstrelan commentedI understand the intentional lack of BC and that this is not actually part of the BC promise. The affected contrib modules will need to effectively provide the BC support themselves to support both 10.2 and 10.3, and can potentially start a new branch that is only 10.3+ that drops BC. This also highlights the benefit of
OPT_IN_TEST_NEXT_MINORas all of these modules would have failed immediately.Comment #6
xjmI think this issue in itself is probably wontfix. As noted, this is an intended hard break.
However, a lot of work could be done to better explain in the release note of #3298701: ImageStyleDownloadController routes do not limit schemes served. I vote for reopening that for a better release note instead.
Comment #7
japerryIMHO the parent issue should have never been committed without this. Policy or not, public method signatures shouldn't be changed like they were here in a minor revision without regard to BC. Several modules are extending this controller, causing some pretty big headaches going to the next minor revision.
Disagree. This pattern becomes an unmaintainable mess across the contrib world. Maintainers expect that core methods stay consistent from minor to minor. The "~X.Y.0" pattern has been a pain point for our update processes not something maintainers should be doing.
Marking RTBC, because the MR should be fast tracked into 10.3.1 to mitigate the impact already done by people trying to upgrade to 10.3 and facing broken sites due to modules not being updated fast enough... partially due to this change.
Comment #8
xjmComment #9
cmlaraStill -1 on the grounds it would be adding a known security fault into the code.
It is now also possible some code has already gone as far as to remove any protections they have had added for older releases (<10.3) now that this safety measure is in place and we could be exposing those site owners to renewed risk.
Disagree, public just means the method can be called from another class, it has no implication on the guarantee that method signature will not change. The only “guarantee” of a method not changing is if it’s declared formal
@api.What we really shouldn’t be doing is modifying or utilizing non-api code (however this is near impossible for many modules since typehinting an interface is API but implementing an interface is not) which would allow us as maintainers (with reasonable assurance) to just declare “^X”.
Comment #10
larowlanBack to NR for #8
Comment #11
smustgrave commentedCan the issue summary be updated to include why the backwards compatibility is more important then the security protection. Maybe which contrib modules are affected that haven't been fixed.
Comment #12
xjmAt this point only core versions with the security-hardened API are supported, so we definitely won't be adding any further BC for this security fix. Thanks!