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] Added support for extracting type from constructor #25605

Merged
merged 1 commit into from Feb 19, 2018

Conversation

@lyrixx
Member

lyrixx commented Dec 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 -
private function extractFromConstructor(string $class, string $property): ?array
{
try {
$class = new \ReflectionClass($class);

This comment has been minimized.

@javiereguiluz

javiereguiluz Dec 26, 2017

Member

Instead of this:

$class = new \ReflectionClass($class);
$constructor = $class->getConstructor();

maybe we can use this:

$constructor = (new \ReflectionClass($class))->getConstructor();

This comment has been minimized.

@lyrixx

lyrixx Dec 26, 2017

Member

fixed.

return null;
}
foreach ($constructor->getParameters() as $name => $parameter) {

This comment has been minimized.

@javiereguiluz

javiereguiluz Dec 26, 2017

Member

Is the $name of the foreach different than the $parameter->name used below or we can reuse it?

This comment has been minimized.

@lyrixx

lyrixx Dec 26, 2017

Member

$name === '$string' (note the leading $).

So I removed the $name assignment. Thanks

@javiereguiluz

Thanks for this contribution!

return null;
}
if (!$constructor) {

This comment has been minimized.

@xabbuh

xabbuh Dec 26, 2017

Member

Should we take parent classes into account too?

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Dec 27, 2017

Member

@lyrixx WDYT? How does phpstorm handle overridden constructors? It could help decide.

This comment has been minimized.

@lyrixx

lyrixx Dec 27, 2017

Member

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

This comment has been minimized.

@lyrixx

lyrixx Dec 27, 2017

Member

Just checked, PHP storm is using parent constructor too. I will update the PR

This comment has been minimized.

@lyrixx

lyrixx Dec 27, 2017

Member

Actually, it's already supported ;) I added a new test case to ensure it works as expected.

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Dec 28, 2017

Member

I was thinking about this situation:

class a
{
  public $foo;
  function __construct(string $foo)
  {
    $this->foo = $foo;
  }
}

class b extends a
{
  function __construct()
  {
    parent::__construct('hello');
  }
}

This comment has been minimized.

@lyrixx

lyrixx Dec 28, 2017

Member

Ah ok. I have added support for such case with a test case.

I guess the PR is now READY \o/

@nicolas-grekas nicolas-grekas added this to the 4.1 milestone Dec 27, 2017

@nicolas-grekas

This comment has been minimized.

Member

nicolas-grekas commented Dec 27, 2017

@jvasseur please comment when adding a 👎 , we're all curious about all opinions to enlighten decisions.

@jvasseur

This comment has been minimized.

Contributor

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

This comment has been minimized.

Member

lyrixx commented Dec 27, 2017

@jvasseur I agree with you, and it already have a lower priority. So there are no issue ;)

@jvasseur

This comment has been minimized.

Contributor

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

This comment has been minimized.

Member

lyrixx commented Dec 27, 2017

You are right, indeed ;)

But I'm not sure it worth a dedicated extractor. Do you really think it worth this complexity?

@jvasseur

This comment has been minimized.

Contributor

jvasseur commented Dec 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.

@lyrixx

This comment has been minimized.

Member

lyrixx commented Dec 28, 2017

👍

return array($this->extractFromReflectionType($parameter->getType()));
}
if ($reflectionClass->getParentClass()) {

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Dec 28, 2017

Member

$parent = to prevent creating two instances of ReflectionClass

This comment has been minimized.

@lyrixx

lyrixx Dec 28, 2017

Member

done.

@@ -100,12 +100,16 @@ public function getProperties($class, array $context = array())
*/
public function getTypes($class, $property, array $context = array())
{
if ($fromMutator = $this->extractFromMutator($class, $property)) {

This comment has been minimized.

@dunglas

dunglas Dec 29, 2017

Member

Is is really necessary to change this (it will make it harder to merge bug fixes later)?

This comment has been minimized.

@lyrixx

lyrixx Dec 30, 2017

Member

Nope, it was just done in order to optimize the code. I can revert it if you want

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Feb 4, 2018

Member

let's revert yes please :)

This comment has been minimized.

@lyrixx

lyrixx Feb 9, 2018

Member

It's reverted

@dunglas

This comment has been minimized.

Member

dunglas commented Dec 29, 2017

Shouldn't you also update the getProperties() method to add properties accessible trough the constructor to the list (write only).

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 ReflectionExtractor).

@dunglas

This comment has been minimized.

Member

dunglas commented Dec 29, 2017

Regarding @jvasseur's comment, maybe can we add a flag in the constructor to disable this feature?

@javiereguiluz

This comment has been minimized.

Member

javiereguiluz commented Dec 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;

public function __construct(int $stock)
{
    $this->stock = $stock > 10;
}

The $stock property is private and it doesn't have any getter/setter. The type of the property would be guessed as int instead of bool. Questions: 1) Is this code realistic or an academic example that nobody would use in practice? 2) Which parts of Symfony would stop working because of this wrong guess? Thanks!

@dunglas

This comment has been minimized.

Member

dunglas commented Dec 29, 2017

I can reply to 2): The Symfony Serializer (at least in the full stack framework).

To me in your example, $stock is not private, it is "write only".

@nicolas-grekas

apparently this needs to be opt-in, isn't it?

@lyrixx

This comment has been minimized.

Member

lyrixx commented Jan 12, 2018

apparently this needs to be opt-in, isn't it?

What do you mean ?

*
* @return Type[]|null
*/
private function extractFromConstructor(string $class, string $property): ?array

This comment has been minimized.

@Taluu

Taluu Jan 16, 2018

Contributor

why ?array, and not ?Type ?

This comment has been minimized.

@lyrixx

lyrixx Jan 22, 2018

Member

Because it returns an array a not a Type

@nicolas-grekas

@dunglas, please advise: as is, context option, or constructor flag?

@@ -100,12 +100,16 @@ public function getProperties($class, array $context = array())
*/
public function getTypes($class, $property, array $context = array())
{
if ($fromMutator = $this->extractFromMutator($class, $property)) {

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Feb 4, 2018

Member

let's revert yes please :)

@dunglas

This comment has been minimized.

Member

dunglas commented Feb 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.
If we must choose, I tend to prefer the constructor flag.

@lyrixx

This comment has been minimized.

Member

lyrixx commented Feb 16, 2018

This PR is now ready

@@ -107,6 +109,13 @@ public function getTypes($class, $property, array $context = array())
if ($fromAccessor = $this->extractFromAccessor($class, $property)) {
return $fromAccessor;
}
if (

This comment has been minimized.

@dunglas

dunglas Feb 19, 2018

Member

$context['enable_constructor_extraction'] ?? $this->enableConstructorExtraction?

This comment has been minimized.

@lyrixx

lyrixx Feb 19, 2018

Member

It's not the same, because if one want to disable it, with your code it will fallback on the enableConstructorExtraction value.

This comment has been minimized.

@lyrixx

lyrixx Feb 19, 2018

Member

You are right. I fixed it.

@dunglas

This comment has been minimized.

Member

dunglas commented Feb 19, 2018

Thanks @lyrixx.

@dunglas dunglas merged commit adcb25e into symfony:master Feb 19, 2018

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
fabbot.io Your code looks good.
Details

dunglas added a commit that referenced this pull request Feb 19, 2018

feature #25605 [PropertyInfo] Added support for extracting type from …
…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

@lyrixx lyrixx deleted the lyrixx:property-info-constructor branch Feb 20, 2018

@komik966 komik966 referenced this pull request Apr 13, 2018

Closed

value object as ApiResource #1843

0 of 7 tasks complete

@fabpot fabpot referenced this pull request May 7, 2018

Merged

Release v4.1.0-BETA1 #27181

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