GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — [Types] Add JsonDecodeDynamicReturnTypeExtension by TomasVotruba · Pull Request #89 · phpstan/phpstan-nette

Conversation

@TomasVotruba

@TomasVotruba TomasVotruba commented Jan 30, 2022 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

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 and stdClass, e.g.:

$values = Nette\Utils\Json::decode($values);
echo $values['item'];
// not possible - a stdClass type given

$values = Nette\Utils\Json::decode($values, Nette\Utils\Json::FORCE_ARRAY);
echo $values->item ;
// not possible - an array type given

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@TomasVotruba
TomasVotruba force-pushed the tv-nette-json-return-type branch 2 times, most recently from 33460c9 to a4ca889 Compare January 30, 2022 10:35
TomasVotruba
TomasVotruba commented Jan 30, 2022
View reviewed changes
Comment thread phpunit.xml Resolved

Uh oh!

There was an error while loading. Please reload this page.
TomasVotruba
TomasVotruba commented Jan 30, 2022
View reviewed changes
Comment thread composer.json Resolved

Uh oh!

There was an error while loading. Please reload this page.
TomasVotruba
TomasVotruba commented Jan 30, 2022
View reviewed changes
Comment thread .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"

@TomasVotruba TomasVotruba Jan 30, 2022 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor Author

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+

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@TomasVotruba
TomasVotruba force-pushed the tv-nette-json-return-type branch from 7743920 to f796944 Compare January 30, 2022 10:41
@TomasVotruba TomasVotruba changed the title [WIP] Add JsonDecodeDynamicReturnTypeExtension [Json] Add JsonDecodeDynamicReturnTypeExtension Jan 30, 2022
@TomasVotruba TomasVotruba changed the title [Json] Add JsonDecodeDynamicReturnTypeExtension [Types] Add JsonDecodeDynamicReturnTypeExtension Jan 30, 2022
ondrejmirtes
ondrejmirtes requested changes Jan 30, 2022
View reviewed changes

@ondrejmirtes ondrejmirtes left a comment

Copy link
Copy Markdown
Member

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@TomasVotruba

TomasVotruba commented Jan 30, 2022

Copy link
Copy Markdown
Contributor Author

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?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@TomasVotruba
TomasVotruba marked this pull request as ready for review January 30, 2022 11:46
@ondrejmirtes

ondrejmirtes commented Jan 30, 2022

Copy link
Copy Markdown
Member

Instead of the whole resolveConstantStringType method you just should call this class https://github.com/phpstan/phpstan-src/blob/master/src/Type/ConstantTypeHelper.php, otherwise it looks fine.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
staabm
staabm reviewed Jan 30, 2022
View reviewed changes
Comment thread tests/Type/Nette/data/json_decode.php Resolved

Uh oh!

There was an error while loading. Please reload this page.
staabm
staabm reviewed Jan 30, 2022
View reviewed changes
Comment thread tests/Type/Nette/data/json_decode.php Resolved

Uh oh!

There was an error while loading. Please reload this page.
ondrejmirtes
ondrejmirtes requested changes Jan 30, 2022
View reviewed changes
Comment thread src/Type/Nette/JsonDecodeDynamicReturnTypeExtension.php

// scalar types with stdClass
return new UnionType([
new ObjectType(stdClass::class),

@ondrejmirtes ondrejmirtes Jan 30, 2022

Copy link
Copy Markdown
Member

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
TomasVotruba reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

@TomasVotruba TomasVotruba Jan 30, 2022

Copy link
Copy Markdown
Contributor Author

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?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@staabm staabm Jan 30, 2022

Copy link
Copy Markdown
Contributor

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?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@ondrejmirtes ondrejmirtes Jan 30, 2022

Copy link
Copy Markdown
Member

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
TomasVotruba reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

@TomasVotruba TomasVotruba Jan 30, 2022 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor Author

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+.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@ondrejmirtes

ondrejmirtes commented Jan 30, 2022

Copy link
Copy Markdown
Member

Just to reiterate - I still want this to be submitted as a json_decode extension to phpstan-src first. Then we can continue here, keep all the tests but make the extension implementation much simpler, delegating to json_decode logic.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@TomasVotruba

TomasVotruba commented Jan 30, 2022

Copy link
Copy Markdown
Contributor Author

@ondrejmirtes I understand that, I just want to make it work and confirm here, so we don't jump back and forth.
Then function extension port to core and update here.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
TomasVotruba added 4 commits January 30, 2022 21:59
@TomasVotruba
[composer] autoload tests with PSR-4
3f248d8
@TomasVotruba
add JsonDecodeDynamicReturnTypeExtension
6535f0e
@TomasVotruba
[ci] add downgrade of nette/utils to use json_decode() for old PHP
ca0ce35
@TomasVotruba
make use of ConstantTypeHelper
1041390
@TomasVotruba
TomasVotruba force-pushed the tv-nette-json-return-type branch from f796944 to 1041390 Compare January 30, 2022 21:02
TomasVotruba added 4 commits January 30, 2022 22:06
@TomasVotruba
cleanup
024bd78
@TomasVotruba
add array types
43b9215
@TomasVotruba
split unknown types test
62a2ec9
@TomasVotruba
rename JsonDecode... to NetteUtilsJsonDecode..
85221fa
@TomasVotruba

TomasVotruba commented Jan 30, 2022 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor Author

@ondrejmirtes As for json_decode, is FunctionTypeSpecifyingExtension the right interface to implement?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@ondrejmirtes

ondrejmirtes commented Jan 30, 2022

Copy link
Copy Markdown
Member

No (https://phpstan.org/developing-extensions/dynamic-return-type-extensions):

functions using DynamicFunctionReturnTypeExtension interface and phpstan.broker.dynamicFunctionReturnTypeExtension service tag.
TomasVotruba reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@TomasVotruba
propose JsonDecodeDynamicReturnTypeExtension
172e2fb
@TomasVotruba

TomasVotruba commented Jan 31, 2022 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor Author

@ondrejmirtes Thanks 👍

I'm trying to add JsonDecodeDynamicReturnTypeExtension, but the only function that is found is PHPStan\Testing\assertType.
The json_decode is never passed.

What I'm doing wrong there?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
TomasVotruba
TomasVotruba commented Jan 31, 2022
View reviewed changes
Comment thread src/Type/Php/JsonDecodeDynamicReturnTypeExtension.php
Comment on lines +25 to +31
public function isFunctionSupported(FunctionReflection $functionReflection): bool
{
// @todo - finds only "assertType", but not "json_decode" :/
dump($functionReflection->getName());

return $functionReflection->getName() === 'json_decode';
}

@TomasVotruba TomasVotruba Jan 31, 2022

Copy link
Copy Markdown
Contributor Author

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 :/

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@TomasVotruba TomasVotruba Jan 31, 2022 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor Author

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.

To reproduce localy, run PHPUnit:

vendor/bin/phpunit tests/Type/Php/JsonDecodeDynamicReturnTypeExtensionTest.php

↓

image

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@ondrejmirtes ondrejmirtes Jan 31, 2022

Copy link
Copy Markdown
Member

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@TomasVotruba TomasVotruba Feb 4, 2022

Copy link
Copy Markdown
Contributor Author

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@ondrejmirtes ondrejmirtes mentioned this pull request Feb 4, 2022
[ReturnTypeExtension] add test case for narrow typed json_decode phpstan/phpstan-src#993
Merged
@ondrejmirtes ondrejmirtes mentioned this pull request May 4, 2022
Specifiy json_decode default return type phpstan/phpstan-src#1283
Closed
@rajyan rajyan mentioned this pull request Nov 19, 2022
Improve isOffsetAccessible->no phpstan/phpstan#8393
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ondrejmirtes ondrejmirtes ondrejmirtes requested changes
@staabm staabm staabm left review comments

Requested changes must be addressed to merge this pull request.

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

3 participants

@TomasVotruba @ondrejmirtes @staabm
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.