[PropertyInfo] ConstructorExtractor which has higher priority than PhpDocExtractor and ReflectionExtractor #30128
Conversation
…ctor and ReflectionExtractor
| * | ||
| * @return DocBlock | ||
| */ | ||
| private function filterDocBlockParams(DocBlock $docBlock, $allowedParam) |
There was a problem hiding this comment.
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.
@OskarStark I applied the type hints in #30335
|
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 |
|
@jvasseur |
|
@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. |
|
As discussed in the PR for 3.4, this is a new feature and as such it should target master. |
| * | ||
| * @return Type[]|null | ||
| */ | ||
| public function getTypesFromConstructor($class, $property); |
There was a problem hiding this comment.
you should add typehints and return type
| * | ||
| * @return \ReflectionParameter|null | ||
| */ | ||
| private function getReflectionParameterFromConstructor($property, \ReflectionMethod $reflectionConstructor) |
There was a problem hiding this comment.
| private function getReflectionParameterFromConstructor($property, \ReflectionMethod $reflectionConstructor) | |
| private function getReflectionParameterFromConstructor(string $property, \ReflectionMethod $reflectionConstructor): ?\ReflectionParameter |
|
|
||
| public function testGetTypes() | ||
| { | ||
| $this->assertEquals([new Type(Type::BUILTIN_TYPE_STRING)], $this->extractor->getTypes('Foo', 'bar', [])); |
There was a problem hiding this comment.
Can you use assertSame() here?
There was a problem hiding this comment.
I can't, assertSame asserts that two variables reference the same object.
| * @param int $date Timestamp | ||
| * @param \DateTimeInterface $dateObject | ||
| */ | ||
| public function __construct(\DateTimeZone $timezone, $date, $dateObject, \DateTime $dateTime) |
There was a problem hiding this comment.
| public function __construct(\DateTimeZone $timezone, $date, $dateObject, \DateTime $dateTime) | |
| public function __construct(\DateTimeZone $timezone, int $date, \DateTimeInterface $dateObject, \DateTime $dateTime) |
There was a problem hiding this comment.
but $dateObject isn't used´ .... 🤔
There was a problem hiding this comment.
@OskarStark That's on purpose, they are used for testing phpdoc types extraction from constructor args even if no such property exists.
|
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 | MIT Supersedes #30056 #30128 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 - `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) & #30335 (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: