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] ConstructorExtractor which has higher priority than PhpDocExtractor and ReflectionExtractor#30128
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
…ctor and ReflectionExtractor
| * | ||
| * @return DocBlock | ||
| */ | ||
| privatefunctionfilterDocBlockParams(DocBlock$docBlock,$allowedParam) |
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 should use type hints and return types wherever possible as this is for 4.2 and new code if I am right
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.
@OskarStark I applied the type hints in#30335
jvasseur commentedFeb 10, 2019
The problem with this solution is that by replacing the property types with the ones from the constructor we will break updating the entity with the setter (when using I think the only way to solve this problem without breaking anything would be to hold constructor argument metadata separately from property metadata. Maybe we could go with the |
karser commentedFeb 11, 2019
@jvasseur |
jvasseur commentedFeb 11, 2019
@karser I think we souldn't infer the property type from constructor arguments at all, this would leave the property type guessing only to for properties (and setter/getter) and provide a completely separate API for getting constructor types. I was originaly not sure about guessing property types from constructor arguments (#25605) but didn't have an example ready to be sure if there would be a problem or not, I guess now we have one. |
fabpot commentedFeb 21, 2019
As discussed in the PR for 3.4, this is a new feature and as such it should target master. |
| * | ||
| * @return Type[]|null | ||
| */ | ||
| publicfunctiongetTypesFromConstructor($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.
you should add typehints and return type
| * | ||
| * @return \ReflectionParameter|null | ||
| */ | ||
| privatefunctiongetReflectionParameterFromConstructor($property,\ReflectionMethod$reflectionConstructor) |
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.
| privatefunction getReflectionParameterFromConstructor($property,\ReflectionMethod$reflectionConstructor) | |
| privatefunction getReflectionParameterFromConstructor(string$property,\ReflectionMethod$reflectionConstructor): ?\ReflectionParameter |
| publicfunctiontestGetTypes() | ||
| { | ||
| $this->assertEquals([newType(Type::BUILTIN_TYPE_STRING)],$this->extractor->getTypes('Foo','bar', [])); |
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.
Can you useassertSame() here?
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 can't, assertSameasserts that two variables reference the same object.
| * @param int $date Timestamp | ||
| * @param \DateTimeInterface $dateObject | ||
| */ | ||
| publicfunction__construct(\DateTimeZone$timezone,$date,$dateObject,\DateTime$dateTime) |
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.
| publicfunction __construct(\DateTimeZone$timezone,$date,$dateObject,\DateTime$dateTime) | |
| publicfunction __construct(\DateTimeZone$timezone,int$date,\DateTimeInterface$dateObject,\DateTime$dateTime) |
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.
but$dateObject isn't used´ .... 🤔
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.
@OskarStark That's on purpose, they are used for testing phpdoc types extraction from constructor args even if no such property exists.
karser commentedFeb 21, 2019
closing this in favor of#30335 |
…riority than PhpDocExtractor and ReflectionExtractor (karser)This PR was merged into the 5.2-dev branch.Discussion----------[PropertyInfo] ConstructorExtractor which has higher priority than PhpDocExtractor and ReflectionExtractor| Q | A| ------------- | ---| Branch? | master| Bug fix? | yes| New feature? | yes| BC breaks? | hopefully no| Deprecations? | no| Tests pass? | yes| Fixed tickets |#30053| License | MITSupersedes#30056#30128In short, when using PhpDocExtractor, it ignores the constructor argument type, although `argument types from the constructor are the only types that are valid for the class instantiation`.This PR adds a separate extractor - `ConstructorExtractor` which is called first (-999) and it attempts to extract the type from constructor only, either from PhpDoc or using reflection.I added `getTypesFromConstructor` to `PhpDocExtractor` and `ReflectionExtractor` - they implement `ConstructorArgumentTypeExtractorInterface` interface. `ConstructorExtractor` aggregates those extractors using compiler pass.So the flow of control looks like this:```PropertyInfoExtractor::getTypes: - ConstructorExtractor::getTypes - PhpDocExtractor::getTypesFromConstructor - ReflectionExtractor::getTypesFromConstructor - PhpDocExtractor::getTypes - ReflectionExtractor::getTypes```Commits-------5049e25 Added ConstructorExtractor which has higher priority than PhpDocExtractor and ReflectionExtractor
…xtractor` class (HypeMC)This PR was merged into the 7.3 branch.Discussion----------[FrameworkBundle][PropertyInfo] Wire the `ConstructorExtractor` class| Q | A| ------------- | ---| Branch? | 7.3| Bug fix? | no| New feature? | yes| Deprecations? | no| Tickets | -| License | MIT| Doc PR | -A sort of followup to#30335.The original PR removed the wiring to prevent creating a BC break, see#30128 (comment) 癿 (comment).This PR proposes adding a new `framework.property_info.with_constructor_extractor` configuration to allow optionally enabling the framework integration. The configuration defaults to `false` to prevent creating a BC break. Only when it's set to `true` will the `ConstructorExtractor` be registered in the container.Commits-------2b392b1 [FrameworkBundle][PropertyInfo] Wire the `ConstructorExtractor` class
The discussion is in previous PR#30056
In short, when using PhpDocExtractor, it ignores the constructor argument type, although
argument types from the constructor are the only types that are valid for the class instantiation.This PR adds a separate extractor -
ConstructorExtractorwhich is called first (-999) and it attempts to extract the type from constructor only, either from PhpDoc or using reflection.I added
getTypesFromConstructortoPhpDocExtractorandReflectionExtractor- they implementConstructorArgumentTypeExtractorInterfaceinterface.ConstructorExtractoraggregates those extractors using compiler pass.So the flow of control looks like this: