Improving extracting type from constructor #10969

LS05 · 2019-02-07T06:44:48Z

No description provided.

javiereguiluz · 2019-02-07T08:24:14Z

@LS05 thanks for your contribution! However, it seems that this is more complicated than it looks. First, this option is configurable. Maybe we should mention that? And if possible, know why this is configurable (maybe it' too slow?)

Second, the extractFromConstructor() method used in the example seems to be private, so can't be used directly, right? See https://github.com/symfony/symfony/blob/974cab3604abf6f5b033d67a7bf4ebcabf509c09/src/Symfony/Component/PropertyInfo/Extractor/ReflectionExtractor.php#L231

LS05 · 2019-02-07T08:33:54Z

@javiereguiluz Oh, my bad! Did it early this morning :)

LS05 · 2019-02-07T08:44:26Z

I think that is configurable because of this recursive call
https://github.com/symfony/symfony/blob/974cab3604abf6f5b033d67a7bf4ebcabf509c09/src/Symfony/Component/PropertyInfo/Extractor/ReflectionExtractor.php#L255

Can this take some time for long subclass relationships?

LS05 · 2019-02-18T14:23:58Z

Besides me being blind about a private method 😄 I'm running a script on the CLI to understand how the ReflectionExtractor::getTypes works.

Here the example script:

use Symfony\Component\PropertyInfo\Extractor\ReflectionExtractor;
 
require_once "./vendor/autoload.php";
 
class Test {
    private $name;
    private $size;
 
    public function __construct(string $name, int $size)
    {
        $this->name = $name;
        $this->size = $size;
    }
};
 
$reflectionExtractor = new ReflectionExtractor();
 
$result = $reflectionExtractor->getTypes(Test::class,'name', ['enable_constructor_extraction' => true]);
 
var_dump($result);

And I get the following output:

php index.php
 
PHP Notice:  Undefined variable: fromConstructor in /property-info/vendor/symfony/property-info/Extractor/ReflectionExtractor.php on line 118
PHP Stack trace:
PHP   1. {main}() /projects/property-info/index.php:0
PHP   2. Symfony\Component\PropertyInfo\Extractor\ReflectionExtractor->getTypes() /projects/property-info/index.php:20
 
Notice: Undefined variable: fromConstructor in /projects/property-info/vendor/symfony/property-info/Extractor/ReflectionExtractor.php on line 118
 
Call Stack:
    0.0019     355184   1. {main}() /projects/property-info/index.php:0
    0.0045     495416   2. Symfony\Component\PropertyInfo\Extractor\ReflectionExtractor->getTypes() /projects/property-info/index.php:20
 
/projects/property-info/index.php:22:
NULL

Am I using the $context array the wrong why?
Because if I call ReflectionExtractor::getTypes this way:

$result = $reflectionExtractor->getTypes(Test::class, "name", ['enable_constructor_extraction' => true]);

var_dump($result);

It gives me my desired output:

cli: php index.php 

projects/property-info/index.php:22:
array(1) {
  [0] =>
  class Symfony\Component\PropertyInfo\Type#8 (6) {
    private $builtinType =>
    string(6) "string"
    private $nullable =>
    bool(false)
    private $class =>
    NULL
    private $collection =>
    bool(false)
    private $collectionKeyType =>
    NULL
    private $collectionValueType =>
    NULL
  }
}

mantis · 2019-02-23T10:49:18Z

@javiereguiluz @LS05 - hi guys, just saw this on slack and thought i'd take a look - I think this might be a symfony bug - in property-info\Extractor\ReflectionExtractor.php, there is a line that reads:

$context['enable_constructor_extraction'] ?? $this->enableConstructorExtraction &&
$fromConstructor = $this->extractFromConstructor($class, $property)

I think this might have been meant to read:

($context['enable_constructor_extraction'] ?? $this->enableConstructorExtraction) &&
$fromConstructor = $this->extractFromConstructor($class, $property)

If passing context to getTypes, it checks value of $context['enable_constructor_extraction'] for true/false or the constructor value of enableConstructorExtraction and should then populate fromConstructor if necessary. The missing brackets around the first part of this check mean that fromConstructor is only applied if context is not set. This fixes the issuse described at [symfony/symfony-docs#10969](symfony/symfony-docs#10969)

LS05 · 2019-02-25T19:41:24Z

@mantis Thanks! I had that error too, but I thought I was calling the class the wrong way :)

@javiereguiluz Could it be that is configurable because in the constructor is possible to set $enableConstructorExtraction to false?
So maybe if the user wants to extract constructor information, is able to do so thanks to the $context parameter?

If passing context to getTypes, it checks value of $context['enable_constructor_extraction'] for true/false or the constructor value of enableConstructorExtraction and should then populate fromConstructor if necessary. The missing brackets around the first part of this check mean that fromConstructor is only applied if context is not set. This fixes the issuse described at [symfony/symfony-docs#10969](symfony/symfony-docs#10969)

… passing context to getTypes (mantis, OskarStark) This PR was merged into the 4.2 branch. Discussion ---------- [PropertyInfo] Fix undefined variable fromConstructor when passing context to getTypes | Q | A | ------------- | --- | Branch? | 4.1, 4.2, master | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | symfony/symfony-docs#10969 | License | MIT | Doc PR | If passing context to getTypes, it checks value of $context['enable_constructor_extraction'] for true/false or the constructor value of enableConstructorExtraction and should then populate fromConstructor if necessary. The missing brackets around the first part of this check mean that fromConstructor is only applied if context is not set. This fixes the issue described at [symfony/symfony-docs#10969](symfony/symfony-docs#10969) Commits ------- 8e401af Allow 3rd argument to be null 04dc692 Remove whitespace (tab on blank line) a0aa15a Update src/Symfony/Component/PropertyInfo/Tests/Extractor/ReflectionExtractorTest.php c2986d5 Update src/Symfony/Component/PropertyInfo/Tests/Extractor/ReflectionExtractorTest.php 42995c8 Update src/Symfony/Component/PropertyInfo/Tests/Extractor/ReflectionExtractorTest.php 2d88298 Update src/Symfony/Component/PropertyInfo/Tests/Extractor/ReflectionExtractorTest.php e43a3bc Update ReflectionExtractorTest.php 2c91c75 Update ReflectionExtractorTest.php 5acc85c Update ReflectionExtractorTest.php d0a2dc0 Update ReflectionExtractorTest.php be8d14a Fix undefined variable fromConstructor when passing context to getTypes

… passing context to getTypes (mantis) This PR was merged into the 4.2 branch. Discussion ---------- [PropertyInfo] Fix undefined variable fromConstructor when passing context to getTypes | Q | A | ------------- | --- | Branch? | 4.1, 4.2, master | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | symfony/symfony-docs#10969 | License | MIT | Doc PR | If passing context to getTypes, it checks value of $context['enable_constructor_extraction'] for true/false or the constructor value of enableConstructorExtraction and should then populate fromConstructor if necessary. The missing brackets around the first part of this check mean that fromConstructor is only applied if context is not set. This fixes the issue described at [symfony/symfony-docs#10969](symfony/symfony-docs#10969) Commits ------- 7e4abee Fix undefined variable fromConstructor when passing context to getTypes

If passing context to getTypes, it checks value of $context['enable_constructor_extraction'] for true/false or the constructor value of enableConstructorExtraction and should then populate fromConstructor if necessary. The missing brackets around the first part of this check mean that fromConstructor is only applied if context is not set. This fixes the issuse described at [symfony/symfony-docs#10969](symfony/symfony-docs#10969)

LS05 · 2019-03-15T19:52:21Z

I have read the discussion in symfony/symfony#25605
And this comment summarized it all symfony/symfony#25605 (comment) about why the option is configurable, @javiereguiluz What do you think?

Plus, @javiereguiluz after your comment symfony/symfony#25605 (comment) there's a "Load More" button but if I click it, I get a 403. So I can't read further comments/explanations 😅

HeahDude

ping @dunglas, would you have a look at this one? Thanks!

HeahDude · 2020-02-07T19:51:04Z

components/property_info.rst

@@ -399,9 +402,15 @@ constructor. It supports return and scalar types for PHP 7.
    $reflectionExtractor->isReadable($class, $property);
    $reflectionExtractor->isWritable($class, $property);

-    // Initializable information
+    // Initializable information.


Can be reverted

HeahDude · 2020-02-07T19:51:22Z

components/property_info.rst

    $reflectionExtractor->isInitializable($class, $property);

+    // Constructor type information.


The dot can also be removed here

wouterj · 2020-10-03T20:09:26Z

Hi @LS05! Thanks for creating the issue and providing value information + bug fixes around this topic!

Seems like the bug you where describing is now fixed. After reading the original PR, I think this option is opt-in as it might create wrong type information (the argument name in the constructor doesn't have to be equal to the property name in the object - although that might be an edge-case imho).

Are you by any chance able to update this PR with all new information you gathered? (so creating a working example and maybe showing both ways of enabling this option?)

Improving extracting type from constructor

d3cc8a9

carsonbot added the Status: Needs Review label Feb 7, 2019

getTypes example added

f8c7890

javiereguiluz added PropertyInfo Waiting feedback labels Feb 7, 2019

mantis mentioned this pull request Feb 23, 2019

Fix undefined variable fromConstructor when passing context to getTypes symfony/property-info#2

Closed

mantis mentioned this pull request Feb 23, 2019

[PropertyInfo] Fix undefined variable fromConstructor when passing context to getTypes symfony/symfony#30361

Merged

HeahDude reviewed Feb 7, 2020

View reviewed changes

HeahDude requested a review from xabbuh February 7, 2020 19:53

wouterj closed this Oct 21, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

Improving extracting type from constructor #10969

Improving extracting type from constructor #10969

Uh oh!

LS05 commented Feb 7, 2019

Uh oh!

javiereguiluz commented Feb 7, 2019

Uh oh!

LS05 commented Feb 7, 2019

Uh oh!

LS05 commented Feb 7, 2019

Uh oh!

LS05 commented Feb 18, 2019 •

edited

Loading

Uh oh!

mantis commented Feb 23, 2019

Uh oh!

LS05 commented Feb 25, 2019

Uh oh!

LS05 commented Mar 15, 2019 •

edited

Loading

Uh oh!

HeahDude left a comment

Uh oh!

HeahDude Feb 7, 2020

Uh oh!

HeahDude Feb 7, 2020

Uh oh!

wouterj commented Oct 3, 2020

Uh oh!

Uh oh!

		$reflectionExtractor->isInitializable($class, $property);

		// Constructor type information.

Uh oh!

Improving extracting type from constructor #10969

Improving extracting type from constructor #10969

Uh oh!

Conversation

LS05 commented Feb 7, 2019

Uh oh!

javiereguiluz commented Feb 7, 2019

Uh oh!

LS05 commented Feb 7, 2019

Uh oh!

LS05 commented Feb 7, 2019

Uh oh!

LS05 commented Feb 18, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mantis commented Feb 23, 2019

Uh oh!

LS05 commented Feb 25, 2019

Uh oh!

LS05 commented Mar 15, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

HeahDude left a comment

Choose a reason for hiding this comment

Uh oh!

HeahDude Feb 7, 2020

Choose a reason for hiding this comment

Uh oh!

HeahDude Feb 7, 2020

Choose a reason for hiding this comment

Uh oh!

wouterj commented Oct 3, 2020

Uh oh!

Uh oh!

LS05 commented Feb 18, 2019 •

edited

Loading

LS05 commented Mar 15, 2019 •

edited

Loading