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

[PropertyInfo] ConstructorExtractor which has higher priority than PhpDocExtractor and ReflectionExtractor #30128

Closed
wants to merge 3 commits into
base: 4.2
from

Conversation

Projects
None yet
6 participants
@karser
Copy link
Contributor

karser commented Feb 9, 2019

Q A
Branch? 4.2
Bug fix? yes
New feature? no
BC breaks? hopefully no
Deprecations? no
Tests pass? yes
Fixed tickets #30053
License MIT

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 - 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

@nicolas-grekas nicolas-grekas added this to the 4.2 milestone Feb 10, 2019

*
* @return DocBlock
*/
private function filterDocBlockParams(DocBlock $docBlock, $allowedParam)

This comment has been minimized.

@OskarStark

OskarStark Feb 10, 2019

Contributor

You should use type hints and return types wherever possible as this is for 4.2 and new code if I am right

This comment has been minimized.

@karser

karser Feb 21, 2019

Author Contributor

@OskarStark I applied the type hints in #30335

@jvasseur

This comment has been minimized.

Copy link
Contributor

jvasseur commented Feb 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 object_to_populate for example) because the type will be wrong there instead.

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 ConstructorArgumentTypeExtractorInterface interface introduced in this PR but instead of using a ConstructorExtractor we could call the getTypesFromConstructor directly from the denormalizer.

@karser

This comment has been minimized.

Copy link
Contributor Author

karser commented Feb 11, 2019

@jvasseur
How should we approach these controversial cases?
We could restrict inferring the property type from constructor to the original case only (see #30053) where the property is private and doesn't have getter/setter.
If we did that, would it still break updating the entity with the setter?

@jvasseur

This comment has been minimized.

Copy link
Contributor

jvasseur commented Feb 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

This comment has been minimized.

Copy link
Member

fabpot commented Feb 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
*/
public function getTypesFromConstructor($class, $property);

This comment has been minimized.

@OskarStark

OskarStark Feb 21, 2019

Contributor

you should add typehints and return type

*
* @return \ReflectionParameter|null
*/
private function getReflectionParameterFromConstructor($property, \ReflectionMethod $reflectionConstructor)

This comment has been minimized.

@OskarStark

OskarStark Feb 21, 2019

Contributor
Suggested change Beta
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', []));

This comment has been minimized.

@OskarStark

OskarStark Feb 21, 2019

Contributor

Can you use assertSame() here?

This comment has been minimized.

@karser

karser Feb 21, 2019

Author Contributor

I can't, assertSame asserts that two variables reference the same object.
image

* @param int $date Timestamp
* @param \DateTimeInterface $dateObject
*/
public function __construct(\DateTimeZone $timezone, $date, $dateObject, \DateTime $dateTime)

This comment has been minimized.

@OskarStark

OskarStark Feb 21, 2019

Contributor
Suggested change Beta
public function __construct(\DateTimeZone $timezone, $date, $dateObject, \DateTime $dateTime)
public function __construct(\DateTimeZone $timezone, int $date, \DateTimeInterface $dateObject, \DateTime $dateTime)

This comment has been minimized.

@OskarStark

OskarStark Feb 21, 2019

Contributor

but $dateObject isn't used´ .... 🤔

This comment has been minimized.

@karser

karser Feb 21, 2019

Author Contributor

@OskarStark That's on purpose, they are used for testing phpdoc types extraction from constructor args even if no such property exists.

@karser

This comment has been minimized.

Copy link
Contributor Author

karser commented Feb 21, 2019

closing this in favor of #30335

@karser karser closed this Feb 21, 2019

@karser karser deleted the karser:4-2-constructor-extractor branch Feb 21, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment