-
Notifications
You must be signed in to change notification settings - Fork 96
Add support for env processor #200
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
Conversation
5e928d4 to
e12afee
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.
Isn't basically the same switch that's casting those values somewhere in Symfony that we could call?
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 is this array: https://github.com/symfony/symfony/blob/5.4/src/Symfony/Component/DependencyInjection/EnvVarProcessor.php#L39 that I can use.
But I would end with a type like string, bool or even 'bool|int|float|string|array'.
I assumed there was something to transform string to StringType and 'bool|int|float|string|array' to the UnionType and you could help me about it ?
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 only method I found which seems to transform something like string to the StringType is the method TypeNodeResolver::resolve, but it would require to pass a TypeNode and a NameScope... In order to have a TypeNode I think it would require me to parse some token and I don't know what to do for the NameScope.
That seems way over complicated. I dunno if you have a simpler solution ? @ondrejmirtes
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 need to ask for TypeStringResolver through constructor and use that: https://github.com/phpstan/phpstan-src/blob/master/src/PhpDoc/TypeStringResolver.php
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 seems to work, but I end up with
Calling PHPStan\PhpDoc\TypeStringResolver::resolve() is not covered
by backward compatibility promise. The method might change in a minor
PHPStan version.
Should I add the error to the baseline ?
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.
Thanks ! It works now perfectly ; thanks again for the help :)
21f8ec0 to
7c7ed4f
Compare
Friendly ping @ondrejmirtes ; is this PR ok for you ? :)
@VincentLanglet Sorry, I'm busy with PHPStan 1.0, I'll have a look at this after the release.
@VincentLanglet Sorry, I'm busy with PHPStan 1.0, I'll have a look at this after the release.
No problem. Well done for the 1.0 release.
I'm currently using 0.12 with my fork of the plugin to use this feature and the phpstan/phpstan-doctrine#218 one. But I'll be happy to try the new 1.0 version.
If you have some time to take a look =)
Thanks
@VincentLanglet My top priority right now is PHP 8.1 support (https://phpstan.org/blog/plan-to-support-php-8-1). The main action is now happening in https://github.com/ondrejmirtes/BetterReflection/tree/ng and https://github.com/phpstan/phpstan-src/tree/ng-better-reflection. Once this is finished and released, the operations will return to normal eventually. I now have 187 emails in my inbox worthy of my attention (all related to PHPStan), so please stay tuned.
Friendly ping @ondrejmirtes,
Happy new year, I know you were pretty busy with the support of php 8.1. I might be wrong but I understood you now finished the support. How long is your todo list now before having some time to take a new look to this PR and phpstan/phpstan-doctrine#218 ?
Thanks a lot for all the work you do.
Support for PHP 8.1 is not finished. I now have 213 unread emails in my inbox worth of my attention + still a lot of work to be done on PHP 8.1, like readonly properties, and updated php-8-stubs.
You don't need to ping me, when I get to it, I'll get to it.
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.
This introduces a hard dependency on symfony/dependency-injection in composer.json. I get the idea about not duplicating Symfony's internal code here, but if we want to do it like that, we need to use something like call_user_func(['\Symfony\Component\DependencyInjection\EnvVarProcessor', 'getProvidedTypes']), and probably even wrap it in some class_exists. Might be easier just to copy that array.
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 got the opposite review here: #200 (comment)
So I'll go with the class_exists solution.
Does
if (
is_string($value)
&& preg_match('/%env\((.*)\:.*\)%/U', $value, $matches) === 1
&& strlen($matches[0]) === strlen($value)
&& class_exists(EnvVarProcessor::class)
) {
$providedTypes = EnvVarProcessor::getProvidedTypes();
return $this->typeStringResolver->resolve($providedTypes[$matches[1]] ?? 'bool|int|float|string|array');
}
wouldn't be enough ?
7c7ed4f to
b5a9373
Compare
Support for PHP 8.1 is not finished. I now have 213 unread emails in my inbox worth of my attention + still a lot of work to be done on PHP 8.1, like readonly properties, and updated php-8-stubs.
Hi, I saw this repository got some recent activity back and phpstan 1.4 is released with readonly property support. I'd like to avoid having to update my fork on every new release. Is there something I can do to help this PR to move forward @ondrejmirtes ? Do you need help on something else ?
@lookyman has to give a 👍 and I’ll be happy to merge this.
I will take a final look later this week. I'm sorry, I am currently swamped with other stuff.
@VincentLanglet I have this errors and I found your MR - thank you very much! Before it's merged I would like to ask if it'll support such scenarios like here:
parameters: some.bool: '%env(bool:FOO)%' is.still.bool: '%some.bool%' services: Foo\Bar: public: true arguments: $isBool: '%some.bool%'
- will
$container->getParameter('is.still.bool')be considered asbool? - will
$isBoolinFoo\Bar's constructor be considered asbool?
In general: will type resolved by EnvVarProcessor be propagated to "nested" parameters? We have large legacy app and I'm working on introducing this extension but our DI definition and container usage is... not optimal 😉
Before it's merged I would like to ask if it'll support such scenarios like here:
It's not a blocker to be merged.
- will
$container->getParameter('is.still.bool')be considered asbool?
I would say no, since phpstan is not fully resolving the config.
But you're free to try the fork/or add more tests here https://github.com/phpstan/phpstan-symfony/pull/200/files#diff-1727317d6da5fe452f28690571b9764589dda96fa1f72c31e834a6cfe6c1f591
Also I would recommend to change to $container->getParameter('some.bool')
- will $isBool in Foo\Bar's constructor be considered as bool?
It's irrelevant since phpstan is not validating yaml services configs.
Of course it's not a blocker, I just wanted to take the opportunity to ask before it's finished 🙂 Thanks for quick answer! Looking forward to get this merged but in the meantime I'll try to use fork and check it out.
@ondrejmirtes Let's merge this!
Thank you!
I've installed 1.1.4 and just wanted to inform you that all places that previously was reported as errors (where parameters were accessed with $container->getParameter()) now are OK 🥳 Could remove one inline ignore and 2 "fixes" that I did before. Thank you very much once again @VincentLanglet 🙂
Close #199