Skip to content

[ReturnTypeExtension] add test case for narrow typed json_decode#993

Merged
ondrejmirtes merged 21 commits intophpstan:1.6.xfrom
TomasVotruba:tv-json-decode-narrow-type
May 4, 2022
Merged

[ReturnTypeExtension] add test case for narrow typed json_decode#993
ondrejmirtes merged 21 commits intophpstan:1.6.xfrom
TomasVotruba:tv-json-decode-narrow-type

Conversation

@TomasVotruba
Copy link
Contributor

@TomasVotruba TomasVotruba commented Feb 4, 2022 •
edited
Loading

Port of https://github.com/phpstan/phpstan-nette/pull/89/files for json_decode()

Any feedback appreciated 👍

How to test?

vendor/bin/phpunit tests/PHPStan/Analyser/NodeScopeResolverTest.php --filter "json"

@TomasVotruba TomasVotruba marked this pull request as ready for review February 4, 2022 14:15
ondrejmirtes
ondrejmirtes requested changes Feb 4, 2022
View reviewed changes
@TomasVotruba TomasVotruba force-pushed the tv-json-decode-narrow-type branch from 96ca0b7 to f682afa Compare February 4, 2022 16:29
ondrejmirtes
ondrejmirtes requested changes Feb 6, 2022
View reviewed changes
@TomasVotruba TomasVotruba force-pushed the tv-json-decode-narrow-type branch 3 times, most recently from 5547fb8 to 55cc2d4 Compare February 6, 2022 12:40
@TomasVotruba
Copy link
Contributor Author

TomasVotruba commented Feb 6, 2022

I'm working on the static errors...

@ondrejmirtes ondrejmirtes force-pushed the 1.5.x branch 3 times, most recently from ddd20b4 to 95d480b Compare March 18, 2022 19:53
@ondrejmirtes
Copy link
Member

ondrejmirtes commented Mar 24, 2022

Feel free to open a new PR if you ever decide to work on this again 👍 Thanks.

@TomasVotruba
Copy link
Contributor Author

TomasVotruba commented Mar 24, 2022

It's ready from my side 👍

I'm actually waiting for feedback last 2 months:

image

@ondrejmirtes
Copy link
Member

ondrejmirtes commented Mar 24, 2022

It is not - there were conflicts and build failures.

@ondrejmirtes ondrejmirtes reopened this Mar 24, 2022
ondrejmirtes
ondrejmirtes requested changes Mar 24, 2022
View reviewed changes
Copy link
Member

@ondrejmirtes ondrejmirtes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

and please fix the conflict by cleanly rebasing the branch on top of 1.5.x. I'll review it once the build is green. Thanks.

@TomasVotruba
Copy link
Contributor Author

TomasVotruba commented Mar 24, 2022

I'm on it 👍

@TomasVotruba TomasVotruba force-pushed the tv-json-decode-narrow-type branch 3 times, most recently from 2e46788 to aafd2a3 Compare March 24, 2022 14:14
@TomasVotruba
Copy link
Contributor Author

TomasVotruba commented Mar 24, 2022

I've rebased. Tests, PHPStan and coding standards are passing localy.

Seems GitHub Actions are crashing now because cache is stuck: https://github.com/phpstan/phpstan-src/runs/5677735280?check_suite_focus=true

I'll check it today/tomorrow

@TomasVotruba TomasVotruba force-pushed the tv-json-decode-narrow-type branch 5 times, most recently from 1cac3b6 to 834eff1 Compare March 24, 2022 17:39
@ondrejmirtes ondrejmirtes force-pushed the tv-json-decode-narrow-type branch from 834eff1 to 2aa201a Compare March 25, 2022 06:31
@TomasVotruba
Copy link
Contributor Author

TomasVotruba commented Mar 26, 2022

CI is passing 👍
What is now missing from my side?

@herndlm herndlm force-pushed the tv-json-decode-narrow-type branch from b8357ff to 961f9c2 Compare May 2, 2022 08:43
TomasVotruba and others added 21 commits May 3, 2022 17:56
@TomasVotruba @ondrejmirtes
add test case for narrow typed json_decode
c4e495a
@TomasVotruba @ondrejmirtes
Extend JsonThrowOnErrorDynamicReturnTypeExtension to detect knonw typ…
38ca032 
…e from contssant string value
@TomasVotruba @ondrejmirtes
update type in LegacyNodeScopeResolverTest
6d4e3b3
@TomasVotruba @ondrejmirtes
return mixed type
581f51a
@TomasVotruba @ondrejmirtes
add json_decode false
d665722
@TomasVotruba @ondrejmirtes
check for JSON_OBJECT_AS_ARRAY, in case of null and array
b2b979b
@TomasVotruba @ondrejmirtes
add test case for invalid json string
665e9da
@TomasVotruba @ondrejmirtes
add test for multiple flags
66e3aa0
@TomasVotruba @ondrejmirtes
decopule type resolution to static
1a124ea
@TomasVotruba @ondrejmirtes
check if JSON_THROW_ON_ERROR exists before infer
bea4f29
@TomasVotruba @ondrejmirtes
[ci] add json to composer require checker json
8748e9c
@TomasVotruba @ondrejmirtes
use bitwiseFlagAnalyser
43eb542
@TomasVotruba @ondrejmirtes
fixup! use bitwiseFlagAnalyser
99debb9
@TomasVotruba @herndlm @ondrejmirtes
Update src/Type/Php/JsonThrowOnErrorDynamicReturnTypeExtension.php
d707038 
Co-authored-by: Martin Herndl <martin@herndl.org>
@TomasVotruba @herndlm @ondrejmirtes
Update src/Type/Php/JsonThrowOnErrorDynamicReturnTypeExtension.php
0255425 
Co-authored-by: Martin Herndl <martin@herndl.org>
@TomasVotruba @ondrejmirtes
fixup! fixup! use bitwiseFlagAnalyser
d586c6e
@TomasVotruba @ondrejmirtes
fixup! fixup! fixup! use bitwiseFlagAnalyser
98abc6f
@TomasVotruba @ondrejmirtes
fixup! fixup! fixup! fixup! use bitwiseFlagAnalyser
8a841fe
@herndlm @ondrejmirtes
Simplify isForceArrayWithoutStdClass
3ad68aa
@herndlm @ondrejmirtes
Simplify fallback type handling
741abd2
@herndlm @ondrejmirtes
Remove unneeded var in narrowTypeForJsonDecode
7d306fb
@ondrejmirtes ondrejmirtes force-pushed the tv-json-decode-narrow-type branch from 961f9c2 to 7d306fb Compare May 3, 2022 15:56
@ondrejmirtes ondrejmirtes merged commit 231990a into phpstan:1.6.x May 4, 2022
@ondrejmirtes
Copy link
Member

ondrejmirtes commented May 4, 2022

Thank you!

@TomasVotruba TomasVotruba deleted the tv-json-decode-narrow-type branch May 4, 2022 06:49
@TomasVotruba
Copy link
Contributor Author

TomasVotruba commented May 4, 2022

👍

@herndlm Thank you for finishing 👏

@herndlm
Copy link
Contributor

herndlm commented May 4, 2022

Nice, thank you too. I was waiting for this getting merged, have a tiny follow-up idea :)

@herndlm herndlm mentioned this pull request May 4, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@staabm staabm staabm left review comments

@ondrejmirtes ondrejmirtes ondrejmirtes approved these changes

+1 more reviewer

@herndlm herndlm herndlm left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@TomasVotruba @ondrejmirtes @herndlm @staabm