New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Calling sprintf() with a single argument should not be allowed #2213
Comments
While I agree that it's not very useful to do so, I would like to see some data to back up whether this actually makes a real difference in performance. Note: If a sniff for this would be created, I believe it should go into PHPCSExtra as the issue (and potential solution) is not WordPress specific. |
Thank you for the quick reply, @jrfnl . Please take a look at this example: https://3v4l.org/T4mI9#veol This can happen when a developer forgets to add a second argument when calling
|
Ah, but that changes the feature request, as the original example wouldn't throw a PHP error/warning (no placeholders), while the code you are posting now, would. As this is a runtime error, not a compile time error, It is a coding error for sure, but I'm not sure it is typically something which should be caught by a CS run. If anything, I believe this belongs with PHPCompatibility to detect, as this is a change in PHP behaviour: a Having said that, I'm not convinced this is reliably sniffable. Consider the following:
Next, let's consider more generically the elevation of the argument count warning to error:
All in all, I personally think this is more something which belongs in the field of unit tests, though I'm not adverse to a sniff in PHPCompatibility, even though that sniff will probably be severely limited. |
Thank you for your response, @jrfnl.
From my point of view, the second code snippet simply better explains why calling
I fully understand that it's impossible to determine with certainty what is being passed to the
This issue has already caused some bugs, and unit tests were ineffective in catching them. Unfortunately, it's practically impossible to anticipate all cases and catch such errors with unit tests. |
I've just had a look at the ticket and using arbitrary user provided input in a That is not something for PHPCS to safeguard against, as PHPCS cannot determine that arbitrary user provided input is being used. This is something which should have been caught in a code review. More than anything the use of arbitrary user provided input without any safeguards anywhere is a security risk and if it caused problems once, there are bound to be more problems. so, if it were up to me, I'd recommend a security review of Gutenberg.
I agree it is impossible to catch all cases, but if a If you want to do a one-time code base review, I can provide you with a list of |
Thank you for your response, @jrfnl.
I have already tried searching for such function calls and regular expressions seem to work pretty well for this. But thanks for offering your help. |
Is your feature request related to a problem?
Linters don't catch cases when
sprintf()
is called with a single argument:This can be seen as a potential performance issue, as calling
sprintf()
like that makes no sense.Describe the solution you'd like
Linters should warn about such code, as this can lead to fatal errors.
Take a look at this example:
https://3v4l.org/T4mI9#veol
The text was updated successfully, but these errors were encountered: