Fix InputBagDynamicReturnTypeExtension #93
Conversation
composer.json
Outdated
| "symfony/console": "^4.0", | ||
| "symfony/framework-bundle": "^4.0", | ||
| "symfony/http-foundation": "^4.0", | ||
| "symfony/http-foundation": "^4.0|^5.0", |
There was a problem hiding this comment.
Why this change? Doesn't seem related to the rest.
There was a problem hiding this comment.
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.
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.
Okay, fair, so just delete the test altogether?
There was a problem hiding this comment.
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.
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.
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.
PR updated, I think that's what you were looking for?
There was a problem hiding this comment.
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:
|
|
@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