Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[DX][FrameworkBundle] Show private aliases in debug:container#22385
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
d5615bf tod18ba64Compare4581894 to7d744e0Comparechalasr commentedApr 12, 2017
Change documented and functionally tested. |
374f779 toa065014Comparechalasr commentedApr 13, 2017
I can't imagine any existing feature impacted by this change. Also, the fixed ticket is 2 years old and it seems everyone agree this would be useful, it makes things simpler. That said , I would love to get this in 3.3. |
chalasr commentedApr 14, 2017
Added some screenshots to the PR body. |
javiereguiluz 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 can't review this proposal technically, but from the DX point of view is perfect! Thanks@chalasr.
| if (!is_file($cachedFile =$this->getContainer()->getParameter('debug.container.dump'))) { | ||
| thrownew \LogicException('Debug information about the container could not be found. Please clear the cache and try again.'); | ||
| if (is_file($cachedFile =$this->getContainer()->getParameter('debug.container.dump'))) { |
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.
AFAIK,debug.container.dump was only added by the compiler pass you deprecated. So this looks suspicious to me
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 parameter isn't added by the pass, but loaded from therehttps://github.com/symfony/symfony/blob/master/src/Symfony/Bundle/FrameworkBundle/Resources/config/debug.xml#L8. Do you think it's time to remove it?
| if (!$this->getApplication()->getKernel()->isDebug()) { | ||
| $kernel =$this->getApplication()->getKernel(); | ||
| if (!$kernel->isDebug()) { |
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.
do we actually need to keep this restriction ? Previously, it was because you the compiler pass was only dumping the file in debug mode. But you don't rely on this file anymore.
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 we're sure there is nothing wrong to get these informations available in non debug mode, I would be glad to remove this check.
| $container->compile(); | ||
| $filesystem =newFilesystem(); | ||
| try { | ||
| $filesystem->dumpFile($cachedFile, (newXmlDumper($container))->dump()); |
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.
why dumping it ? If the goal is to use a cache to make next calls to the command faster, you must use the ConfigCache component to invalidate the cache when the container changes. Otherwise, the command will give outdated data once you edit the container.
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.
The goal is indeed to make the next call faster, as it is currently. About invalidation, it seems to work naturally when clearing the cache, maybe due to the fact this dump is named%kernel.cache_dir%/%kernel.container_class%.xml? Not sure if it's worth dumping it at all, I think it's relevant to don't need to clear the cache for getting debug informations. WDYT?
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.
except that in dev, Inever clear the cache fuly (except when running a composer command doing it for me), as cache files are refreshed individually when needed in debug mode (which is the main feature of the debug mode)
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.
updated to use config cache, removed thedebug.container.dump parameter also
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.
parameter removal and compiler pass deprecation reverted, it now leverages the debug mode properly which isn't the case currently. It would be great if you could have a look
43d9aae tobadb0dcCompare| $container =$buildContainer(); | ||
| $container->getCompilerPassConfig()->setRemovingPasses(array()); | ||
| $container->compile(); | ||
| $cache->write((newXmlDumper($container))->dump(),array(newFileResource($cacheDir.'/'.$containerClass.'.php'))); |
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.
you should rather register the ContainerBuilder resources here rather than a FileResource for the compiled container
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.
done
57596c0 to84b1a47Comparenicolas-grekas commentedApr 14, 2017
I may have read too quickly, but doesn't this remove an XML file that maybe of use to some tier tools? Eg the phpstorm SF plugin and the likes? |
1cad653 to57db321Comparechalasr commentedApr 14, 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.
@nicolas-grekas You're right, the phpstorm plugin relies on. |
Haehnchen commentedApr 14, 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.
PhpStorm Plugin point of view. as i see this PR just extends the xml files, so i am able to resolveHaehnchen/idea-php-symfony2-plugin#618 on my side |
8dde8ae to5810d77Comparenicolas-grekas commentedApr 14, 2017
👍 |
c0d418d to051b200Comparenicolas-grekas commentedApr 18, 2017
ping @symfony/deciders |
051b200 to6746c06Compare6746c06 to883723eComparechalasr commentedApr 21, 2017
PR rebased. |
weaverryan commentedApr 21, 2017
👍 (and 👍 for 3.3 for me, it's really a bug fix) |
chalasr commentedApr 25, 2017
I think we are good here. |
chalasr commentedApr 28, 2017
Can we reconsider the milestone here? A lot of services can be made private in 3.3, it would be too bad to miss them. |
weaverryan commentedMay 3, 2017
I also think we should reconsider this a bug fix for 3.3. "Types" are much more important in 3.3... but there's no way to get a list of them. And currently we can't even use When I run Would it be possible to bump this to 3.3 as a bug fix? |
fabpot commentedMay 3, 2017
Thank you@chalasr. |
…ntainer (chalasr)This PR was merged into the 3.3-dev branch.Discussion----------[DX][FrameworkBundle] Show private aliases in debug:container| Q | A| ------------- | ---| Branch? | master| Bug fix? | no| New feature? | yes| BC breaks? | no| Deprecations? | no| Tests pass? | yes| Fixed tickets |#16388Haehnchen/idea-php-symfony2-plugin#618| License | MIT| Doc PR | n/aBy building and compiling the container without `TYPE_REMOVING` passes, private aliases are available (shown when `--show-private` is passed). The ContainerBuilderDebugDumpPass now makes use of the ConfigCache component, removing the need for clearing the cache to get the debug:container output up to date (in sync with latest config).Config-------```yamlservices: AppBundle\Foo: public: false foo_consumer1: class: AppBundle\Foo arguments: [ '@appbundle\Foo' ] foo_consumer2: class: AppBundle\Foo arguments: [ '@appbundle\Foo' ] foo_alias: alias: AppBundle\Foo foo_private_alias: public: false alias: AppBundle\Foo```Before-------After------Commits-------883723e Show private aliases in debug:container
This PR was squashed before being merged into the 3.3-dev branch (closes#22624).Discussion----------debug:container --types (classes/interfaces)| Q | A| ------------- | ---| Branch? | master| Bug fix? | no| New feature? | yes| BC breaks? | no| Deprecations? | no| Tests pass? | yes| Fixed tickets | none, but needed insymfony/symfony-docs#7807| License | MIT| Doc PR | n/aIn Symfony 3.3, the *type* (i.e. class/interface) is the most important thing about a service. But, we don't have a way for the user to know *what* types are available. This builds on top of `debug:container` to make `debug:container --types`:<img width="1272" alt="screen shot 2017-05-03 at 3 42 37 pm" src="https://cloud.githubusercontent.com/assets/121003/25678671/8bebacaa-3018-11e7-9cf6-b7654e2cae88.png">I think we need this for 3.3, so I've made the diff as *small* as possible. We could make improvements for 3.4, but just *having* this is the most important. I could even remove `format` support to make the diff smaller.~~This depends on#22385, which fixes a "bug" where private services aren't really shown.~~Thanks!Commits-------25a39c2 debug:container --types (classes/interfaces)
Uh oh!
There was an error while loading.Please reload this page.
By building and compiling the container without
TYPE_REMOVINGpasses, private aliases are available (shown when--show-privateis passed). The ContainerBuilderDebugDumpPass now makes use of the ConfigCache component, removing the need for clearing the cache to get the debug:container output up to date (in sync with latest config).Config
Before
After