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

mantis · 2019-02-23T16:30:55Z

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

nicolas-grekas · 2019-02-23T16:32:47Z

Hi, thanks. Could you add a test case please?

mantis · 2019-02-23T17:19:25Z

@nicolas-grekas you asked the one question that I hoped you wouldn't ask - anyway, I've added my first attempt at using phpunit ;)

OskarStark

except this small change, I think you made it with the unit test @mantis 👍 congrats

src/Symfony/Component/PropertyInfo/Tests/Extractor/ReflectionExtractorTest.php

OskarStark · 2019-02-23T17:47:04Z

src/Symfony/Component/PropertyInfo/Extractor/ReflectionExtractor.php

@@ -111,7 +111,7 @@ public function getTypes($class, $property, array $context = [])
        }

        if (
-            $context['enable_constructor_extraction'] ?? $this->enableConstructorExtraction &&
+            ($context['enable_constructor_extraction'] ?? $this->enableConstructorExtraction) &&


are you sure your tests only pass if you add these brackets? 🤔

without the brackets:

Symfony\Component\PropertyInfo\Tests\Extractor\ReflectionExtractorTest::testExtractTypeConstructor with data set #0 ('Symfony\Component\PropertyInf...1Dummy', 'string', array(Symfony\Component\PropertyInfo\Type Object (...)))
Undefined variable: fromConstructor

if I knock up a quick script, returning true for $a, the bellows returns null / 1:

$a = $b = $c = 1;

if( $a ?? $b && $d = $c ) {
var_dump($d);
}
if( ($a ?? $b) && $d = $c ) {
var_dump($d);
}

returns null / 1

mantis · 2019-02-23T17:47:36Z

Thanks ;) I made the mistake of just doing this change through github's editor - hence the multiple formatting commits. But equally 'easier' if you don't have everything checked out.

In terms of the failing travis/appveyor builds, i'd used changes to the test fixtures that went in with 4.2 - and applied the fix to 4.1 branch. I'll leave you guys to decide what if anything you want to do there ;)

src/Symfony/Component/PropertyInfo/Tests/Extractor/ReflectionExtractorTest.php

OskarStark · 2019-02-23T17:50:42Z

4.1 only receive security fixes, and this is not a security fix, so 4.2? should be right, as its a bug fix. My question was, if you are sure that this "bug" is not present in 3.4, in this case you must target 3.4`

master is only for new features

You can check the maintained versions on the roadmap

mantis · 2019-02-23T17:59:00Z

The issue was introduced in December 2017 by symfony/property-info@e168420#diff-818203fea9c55d86a0556b0d436126df

That's been tagged as 4.1.0beta1 -> 4.2.3

OskarStark · 2019-02-23T19:08:59Z

Yes then 4.2 is the correct base branch

src/Symfony/Component/PropertyInfo/Tests/Extractor/ReflectionExtractorTest.php

fabpot · 2019-03-04T08:44:33Z

@mantis Can you fix the tests and change the target to 4.2?

mantis · 2019-03-04T08:47:47Z

@fabpot - when i tried to change the target to 4.2, it started to want to pull in changes to the changelog,, and I got confused... IIRC, I think the test failures were only due to the target not being 4.2

OskarStark · 2019-03-04T09:11:47Z

First you should rebase your feature branch against 4.2 and push --force, afterwards change the target base branch to 4.2 in the github UI

This should work

fabpot · 2019-03-04T09:13:23Z

@mantis I can do it for you if you want.

mantis · 2019-03-04T22:59:59Z

@OskarStark @fabpot - I forced it - the tests were failing from adding in one of oskar's review comments. Current patch set passes as above, so I guess over to you guys ;)

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 · 2019-03-05T10:22:36Z

Thank you @mantis.

… 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

carsonbot added Status: Needs Review Bug labels Feb 23, 2019

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

nicolas-grekas changed the title ~~Fix undefined variable fromConstructor when passing context to getTypes~~ [PropertyInfo] Fix undefined variable fromConstructor when passing context to getTypes Feb 23, 2019

OskarStark requested changes Feb 23, 2019

View reviewed changes

src/Symfony/Component/PropertyInfo/Tests/Extractor/ReflectionExtractorTest.php Outdated Show resolved Hide resolved

src/Symfony/Component/PropertyInfo/Tests/Extractor/ReflectionExtractorTest.php Outdated Show resolved Hide resolved

carsonbot added Status: Needs Work and removed Status: Needs Review labels Feb 23, 2019

OskarStark reviewed Feb 23, 2019

View reviewed changes

src/Symfony/Component/PropertyInfo/Tests/Extractor/ReflectionExtractorTest.php Show resolved Hide resolved

OskarStark reviewed Feb 23, 2019

View reviewed changes

src/Symfony/Component/PropertyInfo/Tests/Extractor/ReflectionExtractorTest.php Outdated Show resolved Hide resolved

mantis changed the base branch from 4.1 to 4.2 February 23, 2019 19:16

mantis changed the base branch from 4.2 to 4.1 February 23, 2019 19:17

fabpot approved these changes Mar 4, 2019

View reviewed changes

carsonbot added Status: Reviewed and removed Status: Needs Work labels Mar 4, 2019

mantis force-pushed the mantis-patch-2 branch from 73a2617 to a0aa15a Compare March 4, 2019 22:00

mantis requested review from dunglas, lyrixx, sroze and xabbuh as code owners March 4, 2019 22:00

carsonbot added Status: Needs Review and removed Status: Reviewed labels Mar 4, 2019

mantis changed the base branch from 4.1 to 4.2 March 4, 2019 22:00

mantis force-pushed the mantis-patch-2 branch from 3b13812 to 8e401af Compare March 4, 2019 22:37

fabpot force-pushed the mantis-patch-2 branch from 8e401af to 7e4abee Compare March 5, 2019 10:21

fabpot merged commit 7e4abee into symfony:4.2 Mar 5, 2019

fabpot mentioned this pull request Apr 2, 2019

Release v4.2.5 #30838

Merged

Uh oh!

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

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

Uh oh!

Conversation

mantis commented Feb 23, 2019

Uh oh!

nicolas-grekas commented Feb 23, 2019

Uh oh!

mantis commented Feb 23, 2019

Uh oh!

OskarStark left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

OskarStark Feb 23, 2019

Choose a reason for hiding this comment

Uh oh!

mantis Feb 23, 2019

Choose a reason for hiding this comment

Uh oh!

mantis commented Feb 23, 2019

Uh oh!

Uh oh!

OskarStark commented Feb 23, 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!

OskarStark commented Feb 23, 2019

Uh oh!

Uh oh!

fabpot commented Mar 4, 2019

Uh oh!

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

Uh oh!

Uh oh!

OskarStark commented Mar 4, 2019

Uh oh!

fabpot commented Mar 4, 2019

Uh oh!

mantis commented Mar 4, 2019

Uh oh!

fabpot commented Mar 5, 2019

Uh oh!

Uh oh!

OskarStark commented Feb 23, 2019 •

edited

Loading

mantis commented Mar 4, 2019 •

edited

Loading