[DependencyInjection] fix regression when extending the Container class without a constructor #26427
Conversation
|
Can we add a test case that would fail without the fix? |
|
we definitely should .. I am a bit scared to creating a test case since it seems like such an obscure case. |
|
|
||
| if (null !== $r && (null !== $constructor = $r->getConstructor()) && 0 === $constructor->getNumberOfRequiredParameters()) { | ||
| if (null !== $r | ||
| && $r->getMethod('__construct')->class === $baseClassWithNamespace |
There was a problem hiding this comment.
we use $constructor = $r->getConstructor() to get the constructor (and your code here breaks in case there is no constructor). Add this logic after the retrieval of $constructor.
And I would check instead Container::class !== $contructor->getDeclaringClass(), to exclude only the Container constructor.
There was a problem hiding this comment.
you mean $constructor->getDeclaringClass() === Container::class ?
68483c8 to
aaa737a
Compare
| @@ -981,8 +981,7 @@ public function __construct() | ||
| if ($this->container->isCompiled()) { | ||
| if (Container::class !== $baseClassWithNamespace) { | ||
| $r = $this->container->getReflectionClass($baseClassWithNamespace, false); | ||
|
|
||
| if (null !== $r && (null !== $constructor = $r->getConstructor()) && 0 === $constructor->getNumberOfRequiredParameters()) { | ||
| if (null !== $r && (null !== $constructor = $r->getConstructor()) && 0 === $constructor->getNumberOfRequiredParameters() && $constructor->getDeclaringClass() === Container::class) { |
There was a problem hiding this comment.
should be !==, not === We want to call the parent constructor when it is a custom one.
There was a problem hiding this comment.
that doesn't fix the issue .. since in the MockContainer the declaring class of the constructor is the Container class, since the MockContainer doesn't define its own constructor
There was a problem hiding this comment.
and that's precisely the goal: we don't want to call the parent constructor for the MockContainer case (otherwise, you would not have a bug currently when it is called).
The Container constructor initializes the ParameterBag, and the dumped container wants to do it in its own way.
There was a problem hiding this comment.
but in that case we don't want to call the parent ..
if I do !== the bug remains .. I have confirmed this, because we still end up calling the parent constructor.
There was a problem hiding this comment.
your code is adding the parent::__construct only when the declaring class of the constructor is Container now.
There was a problem hiding this comment.
ah you are right .. the issue was that it needed to be $constructor->getDeclaringClass()->name and not $constructor->getDeclaringClass() in the comparison.
|
travis result are also strange: don't understand how they can be passing on 5.6 an 7.0 but fail on other versions .. |
|
@lsmith77 read the output: |
aaa737a to
74b976a
Compare
|
as the only thing the Container constructor is doing is initializing the ParameterBag, and this is what we absolutely don't want to be done (as it breaks the lazy-initialization in the dumped container), we could simply reset the property to So the code should become - $code .= " parent::__construct();\n\n";
+ $code .= " parent::__construct();\n";
+ $code .= " $this->parameterBag = null;\n\n"; |
ae94ed6 to
864d935
Compare
|
I have included your change @stof .. I also added some adjustments to the tests. |
864d935 to
0beb64a
Compare
|
note the current tests simply do a string comparison on what code we expect to have generated, rather than testing if the generated code behaves the way we expect it to. the adjustments to the tests I did haven't improved things. |
|
@stof ok now? |
| class NoConstructorContainer | ||
| use Symfony\Component\DependencyInjection\Container; | ||
|
|
||
| class NoConstructorContainer extends Container |
There was a problem hiding this comment.
Should we create a new test case to still keep the existing one (a class without a constructor not inheriting one from its base class)?
There was a problem hiding this comment.
I was also thinking about that .. and to me the old test just seemed incomplete since that class didn't even implement the interface.
|
Thank you @lsmith77. |
…ntainer class without a constructor (lsmith77) This PR was merged into the 3.4 branch. Discussion ---------- [DependencyInjection] fix regression when extending the Container class without a constructor | Q | A | ------------- | --- | Branch? | 3.4+ | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #26397 | License | MIT | Doc PR | - fix regression introduced in c026ec6#diff-f7b23d463cba27ac5e4cb677f2be7623R985 Commits ------- 0beb64a fix regression when extending the Container class without a constructor
fix regression introduced in c026ec6#diff-f7b23d463cba27ac5e4cb677f2be7623R985