[DI] : Fix bad generation of proxy class when use overriden getter on class with constructor #21508
Conversation
| @@ -344,6 +344,27 @@ public function testDumpOverridenGetters() | |||
| $this->assertSame('baz', $r->invoke($baz)); | |||
| } | |||
|
|
|||
| public function testDumpOverridenGettersWithConstructor() | |||
| { | |||
| $container = include self::$fixturesPath.'/containers/container34.php'; | |||
There was a problem hiding this comment.
please give it a non numeric name (new policy for added test cases :) )
There was a problem hiding this comment.
maybe can you merge this test case with the existing one on the topic?
There was a problem hiding this comment.
Ok, should I do this in a new commit or modify my existing commit?
There was a problem hiding this comment.
we will squash anyway, so same commit is ok
| $dump = $dumper->dump(array('class' => 'Symfony_DI_PhpDumper_Test_Overriden_Getters_With_Constructor')); | ||
| $this->assertStringEqualsFile(self::$fixturesPath.'/php/services34.php', $dump); | ||
| $res = $container->getResources(); | ||
| $this->assertSame(realpath(self::$fixturesPath.'/containers/container34.php'), array_pop($res)->getResource()); |
There was a problem hiding this comment.
this looks wrong to me, as we can have other resources, and the on added for this file may not be the first one
There was a problem hiding this comment.
You suggest that I should only check the presence of this resource in the array instead of what I'm doing here?
There was a problem hiding this comment.
I followed your advice
Resolved
04ae996 to
baad4b6
Compare
|
|
||
| $dump = $dumper->dump(array('class' => 'Symfony_DI_PhpDumper_Test_Overriden_Getters_With_Constructor')); | ||
| $this->assertStringEqualsFile(self::$fixturesPath.'/php/services_dump_overriden_getters_with_constructor.php', $dump); | ||
| $resources = array_map( |
There was a problem hiding this comment.
array_map('strval', $resources) would be better I think, because getResource is not part of ResourceInterface
There was a problem hiding this comment.
Ok, thanks for your return and the tip you gave me
There was a problem hiding this comment.
@nicolas-grekas we actually need to check for a file resource, as you did in #21505
There was a problem hiding this comment.
the string version of a resource is supposed to uniquely identify it, so that's not strictly required to me
… class with constructor - [X] Add test fail - [X] Fix bug - [X] Run test pass
baad4b6 to
2440b0f
Compare
|
Thank you @jean-pasqualini. |
…n getter on class with constructor (jean-pasqualini) This PR was merged into the 3.3-dev branch. Discussion ---------- [DI] : Fix bad generation of proxy class when use overriden getter on class with constructor [DI] : Fix bad generation of proxy class when use overriden getter on class with constructor | Q | A | ------------- | --- | Branch? | master | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #21507 | License | MIT - [X] Add test fail - [X] Fix bug - [X] Run test pass Commits ------- 2440b0f [DI] : Fix bad generation of proxy class when use overriden getter on class with constructor
[DI] : Fix bad generation of proxy class when use overriden getter on class with constructor