symfony/symfony-docsPublic

NotificationsYou must be signed in to change notification settings
Fork5.3k
Star2.3k

Improving extracting type from constructor#10969

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

Jump to bottom

Closed

LS05 wants to merge2 commits intosymfony:masterfromLS05:improve_property_info_constructor_example

Closed

Improving extracting type from constructor#10969

LS05 wants to merge2 commits intosymfony:masterfromLS05:improve_property_info_constructor_example

Conversation

Copy link

Contributor

LS05 commentedFeb 7, 2019

No description provided.

Improving extracting type from constructor

d3cc8a9

carsonbot added the Status: Needs Review label

Feb 7, 2019

getTypes example added

f8c7890

Copy link

Member

javiereguiluz commentedFeb 7, 2019

@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, theextractFromConstructor() method used in the example seems to beprivate, so can't be used directly, right? Seehttps://github.com/symfony/symfony/blob/974cab3604abf6f5b033d67a7bf4ebcabf509c09/src/Symfony/Component/PropertyInfo/Extractor/ReflectionExtractor.php#L231

javiereguiluz added PropertyInfo Waiting feedback labels

Feb 7, 2019

Copy link

ContributorAuthor

LS05 commentedFeb 7, 2019

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

Copy link

ContributorAuthor

LS05 commentedFeb 7, 2019

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?

Copy link

ContributorAuthor

LS05 commentedFeb 18, 2019•
edited
Loading

Besides me being blind about a private method 😄 I'm running a script on the CLI to understand how theReflectionExtractor::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 118PHP Stack trace:PHP   1. {main}() /projects/property-info/index.php:0PHP   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 callReflectionExtractor::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  }}

Copy link

mantis commentedFeb 23, 2019

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

mantis mentioned this pull request

Feb 23, 2019

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

Closed

mantis added a commit to mantis/symfony that referenced this pull request

Feb 23, 2019

Fix: undefined variable fromConstructor when passing context to getTypes

f06b76f

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)

mantis added a commit to mantis/symfony that referenced this pull request

Feb 23, 2019

Fix undefined variable fromConstructor when passing context to getTypes

2cd24f2

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)

mantis mentioned this pull request

Feb 23, 2019

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

Merged

mantis added a commit to mantis/symfony that referenced this pull request

Feb 23, 2019

Fix undefined variable fromConstructor when passing context to getTypes

be8d14a

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)

Copy link

ContributorAuthor

LS05 commentedFeb 25, 2019

@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 tofalse?
So maybe if the user wants to extract constructor information, is able to do so thanks to the$context parameter?

fabpot pushed a commit to mantis/symfony that referenced this pull request

Mar 5, 2019

Fix undefined variable fromConstructor when passing context to getTypes

7e4abee

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)

fabpot added a commit to symfony/symfony that referenced this pull request

Mar 5, 2019

bug#30361[PropertyInfo] Fix undefined variable fromConstructor when…

af52f6e

… 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 null04dc692 Remove whitespace (tab on blank line)a0aa15a Update src/Symfony/Component/PropertyInfo/Tests/Extractor/ReflectionExtractorTest.phpc2986d5 Update src/Symfony/Component/PropertyInfo/Tests/Extractor/ReflectionExtractorTest.php42995c8 Update src/Symfony/Component/PropertyInfo/Tests/Extractor/ReflectionExtractorTest.php2d88298 Update src/Symfony/Component/PropertyInfo/Tests/Extractor/ReflectionExtractorTest.phpe43a3bc Update ReflectionExtractorTest.php2c91c75 Update ReflectionExtractorTest.php5acc85c Update ReflectionExtractorTest.phpd0a2dc0 Update ReflectionExtractorTest.phpbe8d14a Fix undefined variable fromConstructor when passing context to getTypes

fabpot added a commit to symfony/symfony that referenced this pull request

Mar 5, 2019

bug#30361[PropertyInfo] Fix undefined variable fromConstructor when…

e74e0f9

… 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

symfony-splitter pushed a commit to symfony/property-info that referenced this pull request

Mar 5, 2019

Fix undefined variable fromConstructor when passing context to getTypes

22d5dff

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)

Copy link

ContributorAuthor

LS05 commentedMar 15, 2019•
edited
Loading

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

Plus,@javiereguiluz after your commentsymfony/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 reviewed

Feb 7, 2020

View reviewed changes

Copy link

Contributor

HeahDude left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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

components/property_info.rst

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

		// Initializable information
		// Initializable information.

Copy link

Contributor

HeahDudeFeb 7, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Can be reverted

components/property_info.rst

		// Initializable information.
		$reflectionExtractor->isInitializable($class, $property);

		// Constructor type information.

Copy link

Contributor

HeahDudeFeb 7, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

The dot can also be removed here

HeahDude requested a review fromxabbuh

February 7, 2020 19:53

Copy link

Member

wouterj commentedOct 3, 2020

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