Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[FrameworkBundle] Add debug:autoconfigure command#31183
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
[FrameworkBundle] Add debug:autoconfigure command#31183
Uh oh!
There was an error while loading.Please reload this page.
Conversation
OskarStark commentedApr 20, 2019
Please use short array syntax 😊 |
4057f66 tof2dec47Compared81ee6c to4d7e67fCompareSimperfit commentedApr 25, 2019
cc@chalasr |
Simperfit commentedApr 26, 2019
AppVeyor failure seems not related |
Simperfit commentedJul 19, 2019
ro0NL 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.
im still 👍 fordebug:autoconfiguration
| /** | ||
| * @return ContainerBuilder | ||
| */ | ||
| privatefunctiongetContainerBuilder() |
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.
is the version fromContainerDebugCommand::getContainerBuilder() blocking for this implementation? extending fromContainerDebugCommand gives us things for free
| { | ||
| $tagContainerBuilder =newContainerBuilder(); | ||
| foreach ($tagContainerBuilder->getServiceIds()as$serviceId) { | ||
| $tagContainerBuilder->removeDefinition($serviceId); |
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.
isnt $tagContainerBuilder empty already 🤔
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 think it's to make sure. And we could refactor it to be save empty and clone it on each call, no ?
| $tagContainerBuilder->addDefinitions([$autoconfiguredInstanceofItem]); | ||
| $dumper =newYamlDumper($tagContainerBuilder); | ||
| preg_match('/calls\:\n((?: +- .+\n)+)/',$dumper->dump(),$matches); |
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.
no big fan of this approach, i'd opt for passing e.g.getMethodCalls() and render plain text as needed. Eventually could be inlined inexecute()
| privatefunctiondumpTagAttribute(array$tagAttribute) | ||
| { | ||
| $cloner =newVarCloner(); | ||
| $cliDumper =newCliDumper(null,null, AbstractDumper::DUMP_LIGHT_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.
#28898 is meant for this :)
maxhelias 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.
I was wondering if this kind of command existed, now i have the answer.
I'm totally 👍 for this feature.
I have some question
| { | ||
| $tagContainerBuilder =newContainerBuilder(); | ||
| foreach ($tagContainerBuilder->getServiceIds()as$serviceId) { | ||
| $tagContainerBuilder->removeDefinition($serviceId); |
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 think it's to make sure. And we could refactor it to be save empty and clone it on each call, no ?
| $container->registerForAutoconfiguration(ResetInterface::class) | ||
| ->addTag('kernel.reset', ['method' =>'reset']); | ||
| ->addTag('kernel.reset', ['method' =>'reset']) | ||
| ->addTag('kernel.reset2', ['method' =>'reset2']) |
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.
Would not it be a forgotten ? It seems strange to me
Simperfit commentedAug 13, 2019
will try to fix this tomorrow. |
maxhelias commentedAug 13, 2019
Or maybe we can do like for#32584, add a option like |
4d7e67f to09bb892Comparenicolas-grekas commentedSep 29, 2019
I checked out the PR and here are two things that should be improved to me:
@Simperfit still available to work on this? Otherwise don't hesitate to call for help. |
Simperfit commentedOct 7, 2019
@nicolas-grekas Thanks for the review, ill look into it this week. |
nicolas-grekas commentedDec 12, 2019
Anyone willing to take over this one? |
atailouloute commentedDec 18, 2019
@nicolas-grekas I can take over this if no one did. |
nicolas-grekas commentedDec 18, 2019
Sure, please do! |
atailouloute commentedDec 18, 2019
Should I create a new PR starting from this branch ? I don't think I have write access to Simperfit's repo |
nicolas-grekas commentedDec 18, 2019
Please create another PR. |
chalasr commentedDec 18, 2019
Closing in favor of#35033. |
Uh oh!
There was an error while loading.Please reload this page.
SEE for a complete output and more insight#28730
As far as I'm concerned, i've seen@ro0NL's comments on the PR, I think this is ready and working, we can tweak it, but letting the work from@aaa2000 being dead it not the right call IMHO. This is the the raw commit rebased with master and pushed.