WordPress/WordPress-Coding-Standards

Calling sprintf() with a single argument should not be allowed

anton-vlasenko opened this issue · 6 comments

anton-vlasenko commented

Is your feature request related to a problem?

Linters don't catch cases when sprintf() is called with a single argument:

echo sprintf( 'Some string' );

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

jrfnl commented

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: echo sprintf() in itself already is a little nonsensical as that should be replaced with a call to printf().

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.

  • 👍1
anton-vlasenko commented

Note: echo sprintf() in itself already is a little nonsensical as that should be replaced with a call to printf().

Thank you for the quick reply, @jrfnl .
This was an example to provide some context. Please ignore the echo.
Whether using printf() or sprintf(), it does not change the point I am trying to make.

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 printf().
It would be helpful if linters could alert developers about such cases.
I believe this applies to:

  • printf()
  • sprintf()
  • fprintf()
  • vprintf()
  • vsprintf()
  • vfprintf()
jrfnl commented

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, php -l (lint) will not detect this.

It is a coding error for sure, but I'm not sure it is typically something which should be caught by a CS run.
It also changes the focus of the feature request away from "performance" to "coding error".

If anything, I believe this belongs with PHPCompatibility to detect, as this is a change in PHP behaviour: a warning which is now an error.

Having said that, I'm not convinced this is reliably sniffable. Consider the following:

  • sprintf( $text ) - As the runtime error only occurs when the $text contains a placeholder and adjusts based on the number of placeholders, the sniff would need access to the $text to determine how many arguments are expected and whether there is an error. As the $text may be defined elsewhere and passed around, this is not always possible.
  • sprintf( 'text with %s', ...$replacements ) - Same story with $replacements, which may be passed around, but could also be an empty array, which would yield an error again.
  • Next, a function like vprintf() is not variadic, so yes, we could check that there are two arguments, though we'd then also need to take PHP 8 named parameters into account and again, the $text and/or $replacements may be passed in as a variable, so may not be analyzable as we may not be able to determine how many items are in the array.

Next, let's consider more generically the elevation of the argument count warning to error:

  • For a PHPCompatibility sniff, we'd then want to make sure we take all functions to which this error applies into account. While this will be a lot of work to research, it's doable, but the same considerations as I mention above will apply for other functions as well...

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.

anton-vlasenko commented

Thank you for your response, @jrfnl.

It also changes the focus of the feature request away from "performance" to "coding error".

From my point of view, the second code snippet simply better explains why calling sprintf() with one argument is bad.

Having said that, I'm not convinced this is reliably sniffable. Consider the following:

I fully understand that it's impossible to determine with certainty what is being passed to the sprintf() function as the first parameter and whether there are any conversion specifications. However, I wasn't suggesting that we try to do so. In fact, it's not necessary because calling sprintf() with one parameter is always problematic for the following reasons:

  1. If there is one or more conversion specifications in $text, calling sprintf() will result in an error.
  2. If there are no conversion specifications, then using sprintf() doesn't make sense since no formatting is necessary. In this case, $text can be used as is.

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.
It is a coding error for sure, but I'm not sure it is typically something which should be caught by a CS run.

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.

jrfnl commented

I've just had a look at the ticket and using arbitrary user provided input in a *printf() without any safeguards looks to be the real problem....

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.

unit tests were ineffective in catching them. Unfortunately, it's practically impossible to anticipate all cases and catch such errors with unit tests.

I agree it is impossible to catch all cases, but if a *printf() with arbitrary data is being used in the code under test, the data set for testing should most definitely contain an entry with a % sign.

If you want to do a one-time code base review, I can provide you with a list of *printf() calls with only one argument (which I expect to be small anyway), but I'm still not convinced this should be caught via a WPCS sniff.

anton-vlasenko commented

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.

Thank you for your response, @jrfnl.
I agree that it's probably a security risk (and I would add, an architectural issue).
But I still don't understand why PHPCS can't warn about it, since using *sprintf() with one argument is always bad. However, I won't push for this sniff anymore.
If there is any agreement about what should and shouldn't be in coding standards, I would be glad to familiarize myself with that information (for my better understanding).

If you want to do a one-time code base review, I can provide you with a list of *printf() calls with only one argument (which I expect to be small anyway), but I'm still not convinced this should be caught via a WPCS sniff.

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.

  • 👍1