-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
[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 our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[FrameworkBundle] Add debug:autoconfigure command #31183
Conversation
Please use short array syntax 😊 |
4057f66
to
f2dec47
Compare
d81ee6c
to
4d7e67f
Compare
cc @chalasr |
AppVeyor failure seems not related |
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 👍 for debug:autoconfiguration
/** | ||
* @return ContainerBuilder | ||
*/ | ||
private function getContainerBuilder() |
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 from ContainerDebugCommand::getContainerBuilder()
blocking for this implementation? extending from ContainerDebugCommand
gives us things for free
{ | ||
$tagContainerBuilder = new ContainerBuilder(); | ||
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 = new YamlDumper($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 in execute()
private function dumpTagAttribute(array $tagAttribute) | ||
{ | ||
$cloner = new VarCloner(); | ||
$cliDumper = new CliDumper(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 :)
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 = new ContainerBuilder(); | ||
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 ?
@@ -376,7 +376,9 @@ public function load(array $configs, ContainerBuilder $container) | |||
$container->registerForAutoconfiguration(LocaleAwareInterface::class) | |||
->addTag('kernel.locale_aware'); | |||
$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
will try to fix this tomorrow. |
Or maybe we can do like for #32584, add a option like |
4d7e67f
to
09bb892
Compare
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. |
@nicolas-grekas Thanks for the review, ill look into it this week. |
Anyone willing to take over this one? |
@nicolas-grekas I can take over this if no one did. |
Sure, please do! |
Should I create a new PR starting from this branch ? I don't think I have write access to Simperfit's repo |
Please create another PR. |
Closing in favor of #35033. |
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.