Make caseless remove robust #440
Conversation
GrahamCampbell
force-pushed
the
robust-caseless-remove
branch
from
a7eeb13
to
605ba96
Compare
GrahamCampbell
force-pushed
the
robust-caseless-remove
branch
from
5ab7ac1
to
b3cbd5d
Compare
There was a problem hiding this comment.
Im happy with this PR but I want to get some more info.
This PR instead skips over integer keys, which I think is the better behaviour, but also technically a breaking change.
How is this a BC break? If a user has an integer key, it will be an error.
With this PR, we will never remove integer keys.
I don't see any application breaking because of this.
| @@ -25,7 +25,7 @@ public static function caselessRemove(array $keys, array $data): array | |||
| } | |||
|
|
|||
| foreach ($data as $k => $v) { | |||
There was a problem hiding this comment.
Do we also want to add @param array<string, mixed> $data on this method?
There was a problem hiding this comment.
No. That would be a breaking change. I'd be happy to do this in a possible future 3.x version, though.
Not according to the function documentation. It says any array can be passed. Indeed people are doing this: #429. Guzzle does have a mechanism for letting them know when they made an error like this, but that exception gets hidden because the code crashes early, here. |
Related to #429. Allows for validation to happen downstream instead of crashing on an internal error. Also, the type information on this function technically says any kind of array key is allowed, sooo. ;)
As to why fix this only on 2.x? On 1.x, it was actually doing casting for us, so the behaviour there was to remove keys with integer value 1 if the string key was '1'. This PR instead skips over integer keys, which I think is the better behaviour, but also technically a breaking change. Since this is not working at all in 2.0.0, we're good to make that change in 2.1.0.