-
Notifications
You must be signed in to change notification settings - Fork 35
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
[Types] Add JsonDecodeDynamicReturnTypeExtension #89
base: 1.1.x
Are you sure you want to change the base?
[Types] Add JsonDecodeDynamicReturnTypeExtension #89
Conversation
33460c9
to
a4ca889
Compare
.github/workflows/build.yml
Outdated
# in PHP 7.1, the json_decode() 2nd parameter requires bool, while PHP 7.2 it's null|int | ||
- name: "Downgrade nette/utils" | ||
if: matrix.php-version == '7.1' | ||
run: "composer require --dev nette/utils:^2.3 nette/forms:^2.4 --update-with-dependencies" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
2nd arg of json_decode()
takes different types in PHP 7.1 and PHP 7.2+
7743920
to
f796944
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What I'd like to see instead is to have this kind of logic for json_decode
in phpstan-src. When that's done, this Nette extension can simply delegate the logic with $scope->getType(new FuncCall('json_decode', $args))
. Of course $args
would be composed based on the FORCE_ARRAY
argument so that json_decode is called with $associative=true
.
I actually tried to add function extension first but PHPStan analysed only non-basic function. Not sure why. Apart that, is the type resolution in tests here correct? |
Instead of the whole |
|
||
// scalar types with stdClass | ||
return new UnionType([ | ||
new ObjectType(stdClass::class), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It can still be an array here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On what input?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess the stdclass could contain a array of other things?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You don't have to guess: https://3v4l.org/qsjVh
BTW I worry that these precise types would become really annoying. Right now people working with json_decode() get some errors on level 9, but with these union types it would be much sonner - on level 7. I think the best course of action is to return "MixedType without stdClass" with FORCE_ARRAY, and normal "MixedType" without FORCE_ARRAY.
MixedType is subtractable - new MixedType(true, new ObjectType(\stdClass::class))
means mixed without stdClass
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
MixedType is subtractable
Cool, that's a new trick to me.
I think the best course of action is to return "MixedType without stdClass" with FORCE_ARRAY, and normal "MixedType" without FORCE_ARRAY.
Do you mean like this?
if ($isForceArray) {
return new MixedType(true, new ObjectType(\stdClass::class));
}
return new MixedType(true);
It might be change with more strict approach, but it would be more correct. The reason we made this extension for our project is to make sure there is only scalar value or stdClass. Mixed includes anything, objects, array of object etc.
Yet the DX is important, I understand. It might be useful to enable this only since level 9+.
Just to reiterate - I still want this to be submitted as a |
@ondrejmirtes I understand that, I just want to make it work and confirm here, so we don't jump back and forth. |
f796944
to
1041390
Compare
@ondrejmirtes As for |
No (https://phpstan.org/developing-extensions/dynamic-return-type-extensions):
|
@ondrejmirtes Thanks 👍 I'm trying to add JsonDecodeDynamicReturnTypeExtension, but the only function that is found is What I'm doing wrong there? |
public function isFunctionSupported(FunctionReflection $functionReflection): bool | ||
{ | ||
// @todo - finds only "assertType", but not "json_decode" :/ | ||
dump($functionReflection->getName()); | ||
|
||
return $functionReflection->getName() === 'json_decode'; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here the json_decode
is never passed :/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because there's already JsonThrowOnErrorDynamicReturnTypeExtension
in phpstan-src whcih changes the result based on JSON_THROW_ON_ERROR
. You need to modify it first.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll try it
By default, the
Nette\Utils\Json::decode()
returns mixed, but in some cases we know there is more specific type.We use this extension internally in private project to separate at
array
andstdClass
, e.g.:It might be useful to have in Nette extension itself. What do you think?
cc @staabm @lulco @matthiasnoback
Based on https://twitter.com/VotrubaT/status/1487434178757074952