[DX][FrameworkBundle] Show private aliases in debug:container #22385

chalasr · 2017-04-11T21:42:23Z

Q	A
Branch?	master
Bug fix?	no
New feature?	yes
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets	#16388 Haehnchen/idea-php-symfony2-plugin#618
License	MIT
Doc PR	n/a

By 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

services:
    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

chalasr · 2017-04-12T09:36:03Z

Change documented and functionally tested.

chalasr · 2017-04-13T15:05:51Z

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 · 2017-04-14T09:07:03Z

Added some screenshots to the PR body.

javiereguiluz

I can't review this proposal technically, but from the DX point of view is perfect! Thanks @chalasr.

stof · 2017-04-14T11:50:23Z

src/Symfony/Bundle/FrameworkBundle/Command/ContainerDebugCommand.php

            throw new \LogicException('Debug information about the container is only available in debug mode.');
        }

-        if (!is_file($cachedFile = $this->getContainer()->getParameter('debug.container.dump'))) {
-            throw new \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'))) {


AFAIK, debug.container.dump was only added by the compiler pass you deprecated. So this looks suspicious to me

This parameter isn't added by the pass, but loaded from there https://github.com/symfony/symfony/blob/master/src/Symfony/Bundle/FrameworkBundle/Resources/config/debug.xml#L8. Do you think it's time to remove it?

stof · 2017-04-14T11:51:17Z

src/Symfony/Bundle/FrameworkBundle/Command/ContainerDebugCommand.php

-        if (!$this->getApplication()->getKernel()->isDebug()) {
+        $kernel = $this->getApplication()->getKernel();
+
+        if (!$kernel->isDebug()) {


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.

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.

stof · 2017-04-14T11:52:44Z

src/Symfony/Bundle/FrameworkBundle/Command/ContainerDebugCommand.php

+            $container->compile();
+            $filesystem = new Filesystem();
+            try {
+                $filesystem->dumpFile($cachedFile, (new XmlDumper($container))->dump());


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.

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?

except that in dev, I never 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)

updated to use config cache, removed the debug.container.dump parameter also

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

stof · 2017-04-14T14:10:05Z

src/Symfony/Bundle/FrameworkBundle/Command/ContainerDebugCommand.php

+            $container = $buildContainer();
+            $container->getCompilerPassConfig()->setRemovingPasses(array());
+            $container->compile();
+            $cache->write((new XmlDumper($container))->dump(), array(new FileResource($cacheDir.'/'.$containerClass.'.php')));


you should rather register the ContainerBuilder resources here rather than a FileResource for the compiled container

nicolas-grekas · 2017-04-14T14:49:45Z

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?

chalasr · 2017-04-14T15:36:33Z

@nicolas-grekas You're right, the phpstorm plugin relies on.
I managed to make this work using the container dumped by the ContainerBuilderDebugDumpPass, this does not deprecate it anymore. It should even improve the features of the phpstorm plugin.

Haehnchen · 2017-04-14T16:03:36Z

PhpStorm Plugin point of view. as i see this PR just extends the xml files, so i am able to resolve Haehnchen/idea-php-symfony2-plugin#618 on my side

nicolas-grekas · 2017-04-14T17:48:24Z

👍

nicolas-grekas · 2017-04-18T16:08:57Z

ping @symfony/deciders
OK to merge this one for 3.3 as requested?

chalasr · 2017-04-21T14:11:52Z

PR rebased.

weaverryan · 2017-04-21T16:47:17Z

👍 (and 👍 for 3.3 for me, it's really a bug fix)

chalasr · 2017-04-25T16:44:27Z

I think we are good here.

chalasr · 2017-04-28T11:46:59Z

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 · 2017-05-03T19:04:57Z

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 debug:container because almost all of the types aliases are private and don't show up currently.

When I run debug:container --show-private and filter for ids that are class names, I get exactly 4. With this PR I get 36.

Would it be possible to bump this to 3.3 as a bug fix?

fabpot · 2017-05-03T19:26:52Z

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 | #16388 Haehnchen/idea-php-symfony2-plugin#618 | License | MIT | Doc PR | n/a By 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 ------- ```yaml services: 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 ------- ![before](http://image.prntscr.com/image/2a69485a4a764316a90260b4e3dfc2a2.png) After ------ ![after](http://image.prntscr.com/image/ea42daa0e5c94841a28dd256450dc8ef.png) 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 in symfony/symfony-docs#7807 | License | MIT | Doc PR | n/a In 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)

carsonbot added Status: Needs Review FrameworkBundle Feature Deprecation labels Apr 11, 2017

chalasr force-pushed the debug-container-privates branch from d5615bf to d18ba64 Compare April 11, 2017 21:44

chalasr changed the title ~~[FrameworkBundle] Show private aliases in debug:container~~ [DX][FrameworkBundle] Show private aliases in debug:container Apr 11, 2017

chalasr mentioned this pull request Apr 11, 2017

[DI Debug] Private Aliases not shown with debug:container #16388

Closed

chalasr force-pushed the debug-container-privates branch 5 times, most recently from 4581894 to 7d744e0 Compare April 12, 2017 09:35

chalasr force-pushed the debug-container-privates branch 2 times, most recently from 374f779 to a065014 Compare April 12, 2017 09:40

nicolas-grekas added this to the 3.4 milestone Apr 13, 2017

javiereguiluz added the DX DX = Developer eXperience (anything that improves the experience of using Symfony) label Apr 14, 2017

javiereguiluz approved these changes Apr 14, 2017

View reviewed changes

stof requested changes Apr 14, 2017

View reviewed changes

chalasr force-pushed the debug-container-privates branch 2 times, most recently from 43d9aae to badb0dc Compare April 14, 2017 14:05

stof reviewed Apr 14, 2017

View reviewed changes

chalasr force-pushed the debug-container-privates branch 2 times, most recently from 57596c0 to 84b1a47 Compare April 14, 2017 14:12

chalasr force-pushed the debug-container-privates branch 2 times, most recently from 1cad653 to 57db321 Compare April 14, 2017 15:28

chalasr force-pushed the debug-container-privates branch from 8dde8ae to 5810d77 Compare April 14, 2017 17:46

chalasr force-pushed the debug-container-privates branch 2 times, most recently from c0d418d to 051b200 Compare April 14, 2017 17:50

chalasr force-pushed the debug-container-privates branch 7 times, most recently from 051b200 to 6746c06 Compare April 20, 2017 21:14

Show private aliases in debug:container

883723e

chalasr force-pushed the debug-container-privates branch from 6746c06 to 883723e Compare April 20, 2017 22:49

fabpot merged commit 883723e into symfony:master May 3, 2017

chalasr deleted the debug-container-privates branch May 3, 2017 19:27

chalasr mentioned this pull request May 3, 2017

Untraceable private aliases Haehnchen/idea-php-symfony2-plugin#618

Closed

weaverryan mentioned this pull request May 3, 2017

debug:container --types (classes/interfaces) #22624

Closed

fabpot mentioned this pull request May 17, 2017

Release v3.3.0-RC1 #22731

Merged

chalasr mentioned this pull request Mar 4, 2018

Improve misleading error message when controller service is not found #26359

Closed

chalasr mentioned this pull request Feb 24, 2019

Private service not injected anywhere is not found in test #29603

Closed

Uh oh!

[DX][FrameworkBundle] Show private aliases in debug:container #22385

[DX][FrameworkBundle] Show private aliases in debug:container #22385

Uh oh!

Conversation

chalasr commented Apr 11, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Config

Before

After

Uh oh!

chalasr commented Apr 12, 2017

Uh oh!

chalasr commented Apr 13, 2017

Uh oh!

chalasr commented Apr 14, 2017

Uh oh!

javiereguiluz left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nicolas-grekas commented Apr 14, 2017

Uh oh!

chalasr commented Apr 14, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Haehnchen commented Apr 14, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nicolas-grekas commented Apr 14, 2017

Uh oh!

nicolas-grekas commented Apr 18, 2017

Uh oh!

chalasr commented Apr 21, 2017

Uh oh!

weaverryan commented Apr 21, 2017

Uh oh!

chalasr commented Apr 25, 2017

Uh oh!

chalasr commented Apr 28, 2017

Uh oh!

weaverryan commented May 3, 2017

Uh oh!

fabpot commented May 3, 2017

Uh oh!

Uh oh!

chalasr commented Apr 11, 2017 •

edited

Loading

chalasr commented Apr 14, 2017 •

edited

Loading

Haehnchen commented Apr 14, 2017 •

edited

Loading