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
The exception messages in class Drupal\file\FileRepository use sprintf to feed in a destination parameter. However, instead of the normal %s designated placeholder the string has the full word %destination and the parameter is passed as an array. This produces the exception message: Invalid stream wrapper: 1estination in Drupal\file\FileRepository->writeData()
Proposed resolution
Change
sprintf('Invalid stream wrapper: %destination', ['%destination' => $destination])
into
sprintf('Invalid stream wrapper: %s', $destination)
Remaining tasks
There are three exceptions like this in core/modules/file/src/FileRepository.php
| Comment | File | Size | Author |
|---|
Issue fork drupal-3328694
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:
changes
Comments
Comment #2
jonathan1055 commentedIt also makes me wonder if these exceptions are covered by any automated test, as this would have been picked up before.
Comment #3
cilefen commentedEvidently there is no test. And I guess phpstan hasn't discovered this because it is valid syntax. The $values parameter of sprintf is coerced into the data type '%d' as a '1', in this case.
Comment #4
cilefen commentedComment #5
jonathan1055 commentedI did try a regex search to see if there were many of them - find %d or %s with chars following:
sprintf.*\%[sd]\wand this only found the three in the file mentioned.It needs fixing in D10.1 first as the problem is still there.
Comment #8
sahilgidwani commentedComment #9
jonathan1055 commentedThanks for starting the MR SahilGidwani. It needs a rebase which you can probably do via the MR summary page.
I think this change should also add test coverage for these three exceptions, as clearly they are not currently tested. You can probably find and extend some existing exception tests.
Comment #10
xjmNW for the test coverage. Thanks!
Comment #11
xjmI would go a step further than this: The
$destinationalready has a string parameter typehint, sosprintf()doesn't add any value here. I would just concatenate and/or use a double-quoted string.sprintf()is only really useful for formatting numbers and the like. It's not a string sanitization function and just adds overhead for exception messages.This also made me curious if we have other places in core where
sprintf()was incorrectly treated as if it weret(). I checked:I found lots of things that should probably not use
sprintf(), but nothing else broken per se.Comment #12
xjmComment #14
murilohp commentedHey! I was reviewing this issue and fortunately we do have tests to cover the exceptions itself but we didn't have tests to cover the messages from the exceptions, so I've just added some tests to do that, and I'm also followed @xjm suggestion, instead of using
sprintf(), the code can use a double-quoted string, it makes a lot of sense.Here's a test only patch.
Comment #15
murilohp commentedComment #16
jonathan1055 commentedThanks @murilohp this looks good. Yes, double-quoted string with {} is perfectly adequate. Nice idea @xjm.
These three failures in the patch prove that the additional 3 line checks are doing what we need. Good that you found a simple place to add them.
I'm happy to have this RTBC but will wait for @xjm to also review.
Comment #17
smustgrave commentedThis issue is being reviewed by the kind folks in Slack, #needs-review-queue-initiative. We are working to keep the size of Needs Review queue [2700+ issues] to around 400 (1 month or less), following Review a patch or merge request as a guide.
Change in MR 3159 looks good and points from #11 appear to be addressed
#14 shows valid test coverage
@xjm in #11 you mentioned you found lots. Should a Meta task be opened to address those?
Comment #18
longwaveCommitted and pushed 465482a571 to 10.1.x and 8caea2cf9b to 10.0.x and 37fb0a9950 to 9.5.x. Thanks!
Comment #23
jonathan1055 commentedTo follow up @xjm's work in #11 and @smustgrave's question in #17, attached is the result of
grep -r "sprintf" * | grep '=>' | grep -v "vendor" | grep -v "node_modules"in 10.1.x. There are 35 lines, but some of these are not a problem because the=>is before thesprintfand so the array is not part of the sprintf. Therefore it is a small problem, and could be fixed quite easily.