- πΊπΈUnited States xjm
I would go a step further than this: The
$destination
already 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:[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. - First commit to issue fork.
- π§π·Brazil murilohp
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.
- Status changed to Needs review
over 1 year ago 3:22am 27 January 2023 - π¬π§United Kingdom jonathan1055
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.
- Status changed to RTBC
over 1 year ago 11:21pm 18 February 2023 - πΊπΈUnited States smustgrave
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?
- π¬π§United Kingdom longwave UK
Committed and pushed 465482a571 to 10.1.x and 8caea2cf9b to 10.0.x and 37fb0a9950 to 9.5.x. Thanks!
-
longwave β
committed 8caea2cf on 10.0.x
Issue #3328694 by murilohp, rpayanm, jonathan1055, xjm, cilefen:...
-
longwave β
committed 8caea2cf on 10.0.x
-
longwave β
committed 465482a5 on 10.1.x
Issue #3328694 by murilohp, rpayanm, jonathan1055, xjm, cilefen:...
-
longwave β
committed 465482a5 on 10.1.x
- Status changed to Fixed
over 1 year ago 12:23pm 19 February 2023 -
longwave β
committed 37fb0a99 on 9.5.x
Issue #3328694 by murilohp, rpayanm, jonathan1055, xjm, cilefen:...
-
longwave β
committed 37fb0a99 on 9.5.x
- Status changed to Fixed
over 1 year ago 1:36pm 24 March 2023 - π¬π§United Kingdom jonathan1055
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 thesprintf
and so the array is not part of the sprintf. Therefore it is a small problem, and could be fixed quite easily.