Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[DI] Allow auto-configured method calls.#24996
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
Uh oh!
There was an error while loading.Please reload this page.
Conversation
nicolas-grekas left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
thanks for starting this work
| $definition->setInstanceofConditionals(array()); | ||
| $parent =$shared =null; | ||
| $instanceofTags =array(); | ||
| $instanceof =array( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
another variable instead of an array would be better I think
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Fixed inb61d981
| $instanceofDef =clone$instanceofDef; | ||
| $instanceofDef->setAbstract(true)->setParent($parent ?:'abstract.instanceof.'.$id); | ||
| $parent ='instanceof.'.$interface.'.'.$key.'.'.$id; | ||
| $instanceofDef->setAbstract(true)->setParent($parent ?:"abstract.instanceof.$id"); |
nicolas-grekasNov 16, 2017 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
please revert these changes, they are unrelated (did fabbot suggest them? if yes, please ignore it :) )
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Not fabbot, just did it because string interpolation run faster in PHP7, but I can revert it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
yes please do, that's unrelated
| if ($parent) { | ||
| $bindings =$definition->getBindings(); | ||
| $abstract =$container->setDefinition('abstract.instanceof.'.$id,$definition); | ||
| $abstract =$container->setDefinition("abstract.instanceof.$id",$definition); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
same as above
| (newResolveChildDefinitionsPass())->process($container); | ||
| $expected =array( | ||
| array('foo',array('bar')), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
changing an existing test case is always suspicious,
is this really legitimate? won't it introduce BC breaks?
we need to answer these questions carefully
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
This change came from the fact that if a definition has already a method call (line 125:if (!$definition->hasMethodCall($call[0])) {), I do not add it again to avoid call a setter multiple times. Hard to say if we should deduplicate or not method calls...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Hard to say if we should deduplicate or not method calls
we should not:
$twig->addExtension($ext1);$twig->addExtension($ext1);...There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I removed method filtering. Method order has changed, but seems normal since we now allow calls inheritance and they are added at the end of the process.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Test order fixed inda6d7e4
| # calls from instanceof are kept, but this comes later | ||
| calls: | ||
| # first call is from instanceof | ||
| -[setSunshine, [bright]] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
same as above: changing an existing test case needs to be doubly confirmed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
@nicolas-grekas fixed inda6d7e4
| # | ||
| -[enableSummer, [true]] | ||
| -[setSunshine, [warm]] | ||
| # Call from instanceof |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
if the order of calls changes, that means a BC break
GaryPEGEOT commentedApr 1, 2018
Methods calls are now in the right order, error on Travis is unrelated (error with RedisAdapter). |
xabbuh commentedApr 3, 2018
@GaryPEGEOT Why did you close here? |
nicolas-grekas commentedApr 3, 2018
Continued in#26768 |
GaryPEGEOT commentedApr 3, 2018
Lost some of my commits, so openned a new one |
Allow to auto-configure method calls like: