Incorrect sprintf parameter usage

I would go a step further than this: The $destination already has a string parameter typehint, so sprintf() 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 were t(). I checked:

[ayrton:drupal | Thu 11:24:15] $ grep -r "sprintf" * |  grep '=>' | grep -v "vendor" | grep -v "node_modules"

I found lots of things that should probably not use sprintf(), but nothing else broken per se.

Hey! 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.

Thanks @murilohp this looks good. Yes, double-quoted string with {} is perfectly adequate. Nice idea @xjm.

1) Drupal\Tests\file\Kernel\MoveTest::testInvalidStreamWrapper
Failed asserting that exception message 'Invalid stream wrapper: 1estination' contains 'Invalid stream wrapper: foo://'

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.

This 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?

Committed and pushed 465482a571 to 10.1.x and 8caea2cf9b to 10.0.x and 37fb0a9950 to 9.5.x. Thanks!

To 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 the sprintf and so the array is not part of the sprintf. Therefore it is a small problem, and could be fixed quite easily.