Skip to content
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

Add array<string, mixed> as return type for getJsonBody #821

Merged
merged 1 commit into from
Jan 8, 2020

Conversation

ruudk
Copy link
Contributor

@ruudk ruudk commented Dec 27, 2019

This makes it easier for PHPStan et all to understand that the array will be mixed.

@remi-stripe
Copy link
Contributor

@ruudk Thanks for doing the work. I'm not sure if we should change this one so I'm going to defer to @ob-stripe who'll get back to you in a few days!

@ob-stripe
Copy link
Contributor

Could you clarify what this helps with exactly?

I was unable to find what array<T> implies for the key type (as opposed to the explicit array<TKey, TValue> notation). Does PHPStan assume that the array is indexed or associative if no type is specified for the keys? The former would be incorrect in this case, and if it's the latter I'm not sure this change is needed as surely array with no other information is the same thing as array<mixed, mixed> for PHPStan?

@ruudk
Copy link
Contributor Author

ruudk commented Jan 8, 2020

See https://phpstan.org/r/466e6dbe-7e8d-4fae-bfce-6c65b9e69df0

PHPStan does not understand what is inside array and wants to know.

Either by changing it to array<mixed> (https://phpstan.org/r/81754f98-ec7f-440d-a126-00a2bea6d019) or array<mixed, mixed> (https://phpstan.org/r/d51cb42e-7f44-44c0-9a3d-86e52b807039) it's solved.

@ob-stripe
Copy link
Contributor

Interesting! Thanks for sharing.

Note that this warning only occurs at level 6 or higher. Currently we use level 1 in our CI pipeline and we'd have many other things to fix before we can get to level 6 (just gave it a try, there are 457 errors at the moment).

Anyway, I'm not opposed to this change, but would you mind making a couple of updates?

  1. changing the type to array<string, mixed>
  2. updating the on the setJsonBody() method as well

This makes it easier for PHPStan et all to understand that the array will be mixed.
@ruudk
Copy link
Contributor Author

ruudk commented Jan 8, 2020

@ob-stripe We run at level 8 🤓 Applied your feedback.

@ob-stripe
Copy link
Contributor

We run at level 8 🤓

I'm jealous 😛

Thanks for the PR! Will release this shortly.

@ob-stripe ob-stripe changed the title Add array<mixed> as return type for getJsonBody Add array<string, mixed> as return type for getJsonBody Jan 8, 2020
@ob-stripe ob-stripe merged commit 460fbcd into stripe:master Jan 8, 2020
@ob-stripe
Copy link
Contributor

Released as 7.17.0.

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

Successfully merging this pull request may close these issues.

4 participants