Skip to content

ParameterBag->keys() return type is incorrect #439

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
nreynis opened this issue Apr 7, 2025 · 6 comments
Open

ParameterBag->keys() return type is incorrect #439

nreynis opened this issue Apr 7, 2025 · 6 comments

Comments

@nreynis
Copy link

nreynis commented Apr 7, 2025

In the ParameterBag stub the return type is improperly narrowed to list<string>.

The implementation use a simple array_keys call. Therefore the return type MUST be list<array-key>.

With the current type narrowing it's impossible to correctly parse a query string with numeric keys (ie: https://domain.tld/search?0=value&78=something). Worse, trusting PHPStan than the key is a string might lead to a runtime TypeError.

@nreynis nreynis changed the title ParameterBag->keys return type is incorrect ParameterBag->keys() return type is incorrect Apr 7, 2025
@staabm
Copy link
Contributor

staabm commented May 1, 2025

//cc @stof as you added the more precise type with b055cab back then. wdyt?

@stof
Copy link
Contributor

stof commented May 13, 2025

The ParameterBag is documented as having string keys. See also @implements \IteratorAggregate<string, mixed> in the same stub.

btw, this case where PHP automatically casts numeric-string keys into an integer is a case where using strict_types in PHP is actually breaking things (without strict_types, PHP would safely cast that integer back to a numeric string when using it as a string).

@staabm
Copy link
Contributor

staabm commented May 13, 2025

a case where using strict_types in PHP is actually breaking things

could you give a 3v4l.org example of what you mean?

@nreynis
Copy link
Author

nreynis commented May 13, 2025

The ParameterBag is documented as having string keys. See also @implements \IteratorAggregate<string, mixed> in the same stub.

Which is also incorrect. It would be more accurate to use \IteratorAggregate<string|int, mixed>. While the original intent may have been to use strings as keys, in practice the code allows both strings and integers at runtime.

From a static analysis perspective, runtime behavior is the only thing that should matter. I want PHPStan to help me identify defects not to obscure them by relying on idealized type annotations that doesn't reflect actual behavior.

This might warrant a follow-up issue in symfony/http-foundation to either:

  • Force cast the key to strings (a potentially breaking change), or
  • Document the keys as string|int

btw, this case where PHP automatically casts numeric-string keys into an integer is a case where using strict_types in PHP is actually breaking things (without strict_types, PHP would safely cast that integer back to a numeric string when using it as a string).

This nuance is irrelevant in this context. The fact that you don’t get a TypeError without strict_types doesn’t change the underlying issue: the returned value is of a different type, and PHPStan should account for that.

@stof
Copy link
Contributor

stof commented May 14, 2025

This nuance is irrelevant in this context. The fact that you don’t get a TypeError without strict_types doesn’t change the underlying issue: the returned value is of a different type, and PHPStan should account for that.

Are you then forbidding all usages of array<string, T> in your projects ? Technically, there is no way to ensure keys are always strings in a PHP array, unless you forbid those strings to be numeric strings.

@nreynis
Copy link
Author

nreynis commented May 14, 2025

Yes, if I cannot ensure the keys will be strings I do not typehint them as strings.

This PHP behaviour is unfortunate but we have to deal with the language quirks. Array keys that are external/unsafe inputs should always be treated as string|int.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants