Report invalid printf placeholder #4478
Conversation
VincentLanglet
force-pushed
the
invalidPrintf
branch
from
e13d386 to
5603955
Compare
|
This pull request has been marked as ready for review. |
| sprintf('%s %s', 'foo'); // one parameter missing | ||
| sprintf('foo', 'foo'); // one parameter over | ||
| sprintf('foo %s', 'foo', 'bar'); // one parameter over | ||
| sprintf('%2$s %1$s %% %1$s %%%', 'one'); // one parameter missing |
There was a problem hiding this comment.
Adding a second parameter does not solve the error since %%% was invalid placeholder.
| sprintf('%2$s %1$s %% %1$s %%', 'one'); // one parameter missing | ||
| sprintf('%2$s %%'); // two parameters required | ||
| sprintf('%2$s %1$s %1$s %s %s %s %s'); // four parameters required | ||
| sprintf('%2$s %1$s %% %s %s %s %s %%% %%%%', 'one', 'two', 'three', 'four'); // ok |
There was a problem hiding this comment.
This was producing an error https://3v4l.org/2dPI6
There was a problem hiding this comment.
we had such test problems in the past. I wonder whether we can somehow detect such bogus tests
(in the general case, not just in (s)printf)
| if ($count === null) { | ||
| return [ | ||
| RuleErrorBuilder::message(sprintf( | ||
| 'Call to %s contains an invalid placeholder.', |
There was a problem hiding this comment.
Would be nice to know what the invalid placeholder is and its position. Otherwise it'd be hard to debug with long format strings as %c%c%c%c%c%c%c%c%c%c%c%c%c%c%c%c%c%c%c%c%c%c%c%c%c%c%c%c%c%c%c%c%c%c%c%c%c%c%c%c%c%c%c%.
There was a problem hiding this comment.
I thought about this and I was wondering if we should iterate and add more details with acceptsReasonsTip later because:
-
This would require some weird signature for getPrintfPlaceholdersCount (Returning the count + the invalid placeholders/positions)
-
This would require to handle error message for
- Union string
%c%|%c%c%=> What would be the message ? 3rd position ? 5th position ? - String with multiple wrong placeholder like
%%% %%%=> Should we report all the invalid ones ?
- Union string
=> maybe you already have an idea about the format of the message @ondrejmirtes ?
|
Also maybe this belongs to bleeding edge only? |
I would have consider this kinda as a "bugfix". I can put this behind bleeding edge, but that would mean adding an extra parameters to |
4 participants
Closes phpstan/phpstan#1889
Closes phpstan/phpstan#8547 (cc @staabm)