-
Notifications
You must be signed in to change notification settings - Fork 96
Fix InputBagDynamicReturnTypeExtension #93
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
Fix InputBagDynamicReturnTypeExtension #93
Conversation
composer.json
Outdated
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.
Why this change? Doesn't seem related to the rest.
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.
The InputBag is new in Symfony 5, didn't exist in 4 yet, so to run the tests we need Symfony 5
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 currently isn't a nice way to test it in a 3rd party package, so you can delete the file tests/Type/Symfony/input_bag_get.php which isn't very useful currently.
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.
Okay, fair, so just delete the test altogether?
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.
Oh sorry, I haven't noticed there's actually a way here in phpstan-symfony with the ExtensionTestCase :) I just thought you create the file input_bag_get.php and let PHPStan analyse it, but right now you're actually checking the return types which is great :) So feel free to add some tests that fail with mixed, string|null etc. and see how my suggested logic fixes that :)
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'm not quite sure what you want me too add there? The $default parameter can technically be mixed, however, anything other than a string will throw a deprecation notice. Besides, falling back on default will say string|null either way, no matter what you put in $default, so I'm not sure if it would matter. I'm fine with adding other types to the test but the result will be the same :P
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.
mixed means that it can be a string or null and we can't be sure, so the return type also has to be string|null. I just want to test the extension for mixed and string|null types as the second parameter.
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.
PR updated, I think that's what you were looking for?
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.
Approximately :) Thanks.
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.
Please fix the build: https://travis-ci.com/github/phpstan/phpstan-symfony/jobs/372130355
@raalderink Anything I can do to help this PR get merged? I'm having the same issue while upgrading to Symfony 5.
Created a PR on your fork to get rid of the failing test. raalderink#1
@ruudk Thanks, I had lost track of this as I went on holiday
Hi, the logic in the extension is solid but:
- I don't get the change in composer.json.
- The build failed.
@raalderink Are you going to fix this or should I take it over?
@ondrejmirtes @raalderink Let's continue in #107 which is ✅ and ready to merge 🎉
For the InputBag::get, if you add a default string parameter, it will always return a string. This is new in Symfony 5.0.
Fixes issue #91