Skip to content

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

Open
@nreynis

Description

@nreynis

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.

Activity

changed the title [-]ParameterBag->keys return type is incorrect[/-] [+]`ParameterBag->keys()` return type is incorrect[/+] on Apr 7, 2025
staabm

staabm commented on May 1, 2025

@staabm
Contributor

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

stof

stof commented on May 13, 2025

@stof
Contributor

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

staabm commented on May 13, 2025

@staabm
Contributor

a case where using strict_types in PHP is actually breaking things

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

nreynis

nreynis commented on May 13, 2025

@nreynis
Author

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

stof commented on May 14, 2025

@stof
Contributor

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

nreynis commented on May 14, 2025

@nreynis
Author

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.

linked a pull request that will close this issue on May 20, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions

      `ParameterBag->keys()` return type is incorrect · Issue #439 · phpstan/phpstan-symfony