Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[PropertyInfo] Added support for extracting type from constructor#25605
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
lyrixx commentedDec 26, 2017
| Q | A |
|---|---|
| Branch? | master |
| Bug fix? | no |
| New feature? | yes |
| BC breaks? | no |
| Deprecations? | no |
| Tests pass? | yes |
| Fixed tickets | - |
| License | MIT |
| Doc PR | - |
8a421e4 to4bc0bdbCompare| privatefunctionextractFromConstructor(string$class,string$property): ?array | ||
| { | ||
| try { | ||
| $class =new \ReflectionClass($class); |
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.
Instead of this:
$class =new \ReflectionClass($class);$constructor =$class->getConstructor();
maybe we can use this:
$constructor = (new \ReflectionClass($class))->getConstructor();
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.
fixed.
| returnnull; | ||
| } | ||
| foreach ($constructor->getParameters()as$name =>$parameter) { |
javiereguiluzDec 26, 2017 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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.
Is the$name of the foreach different than the$parameter->name used below or we can reuse 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.
$name === '$string' (note the leading$).
So I removed the$name assignment. Thanks
4bc0bdb toe1672aaComparejaviereguiluz left a comment
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 for this contribution!
| returnnull; | ||
| } | ||
| if (!$constructor) { |
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.
Should we take parent classes into account too?
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.
@lyrixx WDYT? How does phpstorm handle overridden constructors? It could help decide.
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 think we should take parents into account.
But I don't know how phpstorm handle that. I don't use it. You should know it ;)
I will check
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.
Just checked, PHP storm is using parent constructor too. I will update the PR
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.
Actually, it's already supported ;) I added a new test case to ensure it works as expected.
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 was thinking about this situation:
class a{public$foo;function__construct(string$foo) {$this->foo =$foo; }}class bextends a{function__construct() {parent::__construct('hello'); }}
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.
Ah ok. I have added support for such case with a test case.
I guess the PR is now READY \o/
nicolas-grekas commentedDec 27, 2017
@jvasseur please comment when adding a 👎 , we're all curious about all opinions to enlighten decisions. |
e1672aa to4dcdbb2Comparejvasseur commentedDec 27, 2017
Sorry. I'm mostly against this change since having a constructor argument with the same name as a property doesn't necessarily mean it will be saved inside that property. This feels less reliable that using the types from getter/setters and I don't fell fine having this guess inside the same extractor. Maybe putting it inside a separate extractor that could be assigned a to a lower priority. |
lyrixx commentedDec 27, 2017
@jvasseur I agree with you, and it already have a lower priority. So there are no issue ;) |
jvasseur commentedDec 27, 2017
@lyrixx it has a lower priority inside the reflection extractor, but it still is the same extractor meaning you can't run another extractor between the accessor and the constructor guess. |
lyrixx commentedDec 27, 2017
You are right, indeed ;) But I'm not sure it worth a dedicated extractor. Do youreally think it worth this complexity? |
jvasseur commentedDec 27, 2017
Not necessary I am just thinking this is something that could append but I don't have a real case were it would append. |
4dcdbb2 to7ba526bComparelyrixx commentedDec 28, 2017
👍 |
| returnarray($this->extractFromReflectionType($parameter->getType())); | ||
| } | ||
| if ($reflectionClass->getParentClass()) { |
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.
$parent = to prevent creating two instances of ReflectionClass
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.
done.
7ba526b to2b1c2b3Compare| */ | ||
| publicfunctiongetTypes($class,$property,array$context =array()) | ||
| { | ||
| if ($fromMutator =$this->extractFromMutator($class,$property)) { |
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.
Is is really necessary to change this (it will make it harder to merge bug fixes later)?
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.
Nope, it was just done in order to optimize the code. I can revert it if you want
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.
let's revert yes please :)
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's reverted
dunglas commentedDec 29, 2017
Shouldn't you also update the Also, if we start supporting this, it would be nice to also support constructors in the PhpDocExtractor (it is more powerful and precise than the |
dunglas commentedDec 29, 2017 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Regarding@jvasseur's comment, maybe can we add a flag in the constructor to disable this feature? |
javiereguiluz commentedDec 29, 2017
@dunglas please, let's not complicate things with a constructor flag. I'd like to understand better the problem explained by@jvasseur. Imagine that this is your code: private/* bool */$stock;publicfunction__construct(int$stock){$this->stock =$stock >10;} The |
nicolas-grekas left a comment
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.
apparently this needs to be opt-in, isn't it?
lyrixx commentedJan 12, 2018
What do you mean ? |
| * | ||
| * @return Type[]|null | ||
| */ | ||
| privatefunctionextractFromConstructor(string$class,string$property): ?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.
why?array, and not?Type ?
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 it returns an array a not a Type
nicolas-grekas left a comment• edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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.
@dunglas, please advise: as is, context option, or constructor flag?
| */ | ||
| publicfunctiongetTypes($class,$property,array$context =array()) | ||
| { | ||
| if ($fromMutator =$this->extractFromMutator($class,$property)) { |
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.
let's revert yes please :)
2b1c2b3 to264f58fComparedunglas commentedFeb 9, 2018
Both would be great (we already do that in the serializer component), the constructor flag allowing to set the default value, and the context option taking priority. |
264f58f toc5b3f45Comparelyrixx commentedFeb 16, 2018
This PR is now ready |
| return$fromAccessor; | ||
| } | ||
| if ( |
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.
$context['enable_constructor_extraction'] ?? $this->enableConstructorExtraction?
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's not the same, because if one want todisable it, with your code it will fallback on theenableConstructorExtraction value.
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 are right. I fixed it.
c5b3f45 toadcb25eComparedunglas commentedFeb 19, 2018
Thanks@lyrixx. |
…constructor (lyrixx)This PR was merged into the 4.1-dev branch.Discussion----------[PropertyInfo] Added support for extracting type from constructor| Q | A| ------------- | ---| Branch? | master| Bug fix? | no| New feature? | yes| BC breaks? | no| Deprecations? | no| Tests pass? | yes| Fixed tickets | -| License | MIT| Doc PR | -Commits-------adcb25e [PropertyInfo] Added support for extracting type from constructor