Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Sign up
Appearance settings

Report invalid printf placeholder #4478

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

Open
VincentLanglet wants to merge 2 commits into phpstan:2.1.x
base: 2.1.x
Choose a base branch
Loading
from VincentLanglet:invalidPrintf

Conversation

@VincentLanglet
Copy link
Contributor

@VincentLanglet VincentLanglet commented Oct 24, 2025 •
edited
Loading

@VincentLanglet VincentLanglet marked this pull request as draft October 24, 2025 21:54
Copy link
Collaborator

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
Copy link
Contributor Author

@VincentLanglet VincentLanglet Oct 24, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Contributor Author

@VincentLanglet VincentLanglet Oct 24, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was producing an error https://3v4l.org/2dPI6

Copy link
Contributor

@staabm staabm Oct 25, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.',
Copy link
Member

@ondrejmirtes ondrejmirtes Oct 25, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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%.

Copy link
Contributor Author

@VincentLanglet VincentLanglet Oct 25, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ?

=> maybe you already have an idea about the format of the message @ondrejmirtes ?

Copy link
Member

Also maybe this belongs to bleeding edge only?

Copy link
Contributor Author

Also maybe this belongs to bleeding edge only?

I would have consider this kinda as a "bugfix".
Technically the sprintf('%') call should have been reported since there is was missing parameter https://3v4l.org/lAIAAs

I can put this behind bleeding edge, but that would mean adding an extra parameters to getPrintfPlaceholdersCount methods with something like getPrintfPlaceholdersCount($format, $reportInvalidPlaceholder). It's your call !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ondrejmirtes ondrejmirtes Awaiting requested review from ondrejmirtes

1 more reviewer

@staabm staabm staabm left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Missing sprintf error Warning: printf(): Too few arguments

AltStyle によって変換されたページ (->オリジナル) /