Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
merged branch kbond/config_dump (PR #1099)
Commits ------- 73ac773 [Config] added ability to set info message and example to node definition Discussion ---------- [2.1] [Config] added ability to set info message to node definition The configuration TreeBuilder lends itself to be hooked into for reference documentation generation. This ``setInfo()`` method allows the addition of a message to a node for use in doc generation. Example (``Symfony/Bundle/WebProfilerBundle/DependencyInjection/Configuration.php``): ```php <?php // ... $treeBuilder = new TreeBuilder(); $rootNode = $treeBuilder->root('web_profiler'); $rootNode ->children() ->booleanNode('verbose')->defaultTrue() ->setInfo('Setting to false hides some secondary information to make the toolbar shorter.')->end() ->booleanNode('toolbar')->defaultFalse() ->setInfo('Enable/Disable the display of the web debug toolbar containing a summary of the profiling data.')->end() ->booleanNode('intercept_redirects')->defaultFalse() ->setInfo('Intercepts the redirects and gives you the opportunity to look at the collected data before following the redirect.')->end() ->end() ; return $treeBuilder; // ... ``` I think a core console command would be great (ie: ``config:reference:dump web_profiler``) or even a frame in the profiler. The way bundle configuration processing is not enforced makes this difficult currently. Perhaps adding a ``getConfiguration()`` method to ``ExtensionInterface``. Thoughts? Certainly this change would allow for a third party bundles or sites (ie symfony.com) to generate bundle reference docs. --------------------------------------------------------------------------- by Seldaek at 2011/05/25 10:36:10 -0700 :+1: finally some effort in this direction, although as @schmittjoh said without generation this won't bring us very far. But if it allows everyone to document their stuff already, I think it's still a plus, otherwise we'll have generation without content. And content without generation is still useful for people looking at the sources. --------------------------------------------------------------------------- by Problematic at 2011/05/25 10:52:42 -0700 +1, even if the only thing it does is save me some WTFs later looking through my own code. --------------------------------------------------------------------------- by weaverryan at 2011/05/25 10:59:25 -0700 @kbond and I talked about this a bit over the weekend and decided that he should at least get this first step going. It *is* of limited use in its current state (thought @Problematic and @Seldaek bring up a good points), but we need some ideas on where to go next. As @kbond says, nothing really ties the DI extension to a config class now. Should we add a `getConfiguration()` or `getTreeBuilder()`? How can we make it so that we *know* which `TreeBuilder` to use for each DI extension config alias? --------------------------------------------------------------------------- by lsmith77 at 2011/05/25 11:21:05 -0700 I think this is great and I dont think we need to require a generator if the API makes sense. One thing the API should also cover is setting a custom error message in case of a validation error. Just something to keep in mind. --------------------------------------------------------------------------- by stof at 2011/05/25 11:26:05 -0700 @weaverryan I think that ``getConfiguration`` would be better than ``getTreeBuilder`` as this is still the method implemented by the ``ConfigurationInterface``. Of course this method should be optionnal (defaulting to return ``null``) --------------------------------------------------------------------------- by kbond at 2011/05/25 13:09:26 -0700 The ``ConfigurationInterface`` API would need to be locked down more. For example, FrameworkBundle's ``Configuration`` class has a constructor. I am afraid if we lock it down too much we could lose its flexibility. --------------------------------------------------------------------------- by stof at 2011/05/25 13:24:51 -0700 @kbond the constructor **cannot** be enforced by an interface. This is why I was talking about adding a ``getConfiguration`` method in the extension. This way, the extension can do whatever it wants to instantiate the Configuration, passing whatever param you want. --------------------------------------------------------------------------- by kbond at 2011/05/25 14:12:03 -0700 Sorry meant the ``ExtensionInterface``. I would like the ``getConfiguration`` method enforced. How would we tackle the FrameworkBundle? To retrieve it's configuration you need the ``ContainerBuilder``. --------------------------------------------------------------------------- by kbond at 2011/05/25 14:38:02 -0700 The only way I can see the bundle method working is if the method was manually overridden for each bundle (returns null by default). The fact that ``Configuration`` classes may or may not have constructors prevent it from being automated. --------------------------------------------------------------------------- by kbond at 2011/05/26 06:24:37 -0700 I think we should avoid *searching* if at all possible. The only real thing you might need to build your configuration is the ``ContainerBuilder``. What about passing it to the ``Extension`` class constructor? Then it could be accessed in a ``getConfiguration()`` method. Looking at the internals, I can't see how I could do that but thought I would throw it out there. --------------------------------------------------------------------------- by stof at 2011/05/26 06:31:46 -0700 The constructor is not a good idea IMO as the ContainerBuilder passed to an extension is not the main one when using the ``load`` method. So passing it in the constructor is bad IMO. Passing it to the ``getConfiguration`` method would be better IMO. --------------------------------------------------------------------------- by kbond at 2011/05/26 08:04:08 -0700 Based on the comments I am seeing 2 ways of implementing a ``getConfiguration()`` method. I have started branches for both ways: 1. ``getConfiguration()`` in Bundle class: kbond@3fb1c88 2. ``getConfiguration()`` in Extension class: kbond@cf05ec2 I have updated the ``FrameworkBundle`` in both. On another note, I think one ``Configuration`` class per Bundle should be enforced. Any reason why we can't do this? The only core bundle that has more than one is ``SecurityBundle`` and I don't get why it needs two (not saying it shouldn't, I just don't know why). --------------------------------------------------------------------------- by stof at 2011/05/26 08:15:52 -0700 One configuration class per bundle does not make any sense as each configuration class is responsible to merge an array of configurations. So a bundle providing several extension (not the common use case at all but supported) would need several configuration classes as each extension would get a different set of configuration. and if you look at what a Configuration class do, it merges several configurations in an intelligent way and normalizes it. Look at why a second one is used in the SecurityBundle and you will see that this is a perfectly valid use of it. The Config component is not limited to DI extensions configuration. And for the 2 implementations, I prefer the one putting it in the extension. Here are my reasons: - a bundle supports having several extensions so returning only one Configuration class for the bundle is broken in this case (for which extension is it ? and what about the others ?) - the extension is the class using the Configuration, not the bundle. So it is logical to put it in it - this removes code duplication to instantiate the Configuration as the extension can use the method. --------------------------------------------------------------------------- by kbond at 2011/05/27 08:41:58 -0700 Created a *rough* initial ``config:dump`` console command. Works for all core bundles. Still needs some work for the more complex Node types. Had some issues getting it to work with ``AsseticBundle`` and ``SecurityBundle``. There is some code duplication I couldn't figure out how to avoid. @weaverryan I tried to mimic the YAML format you have in the reference docs. --------------------------------------------------------------------------- by kbond at 2011/06/02 08:09:57 -0700 I reduced the scope of this PR to just the API to document configuration nodes and access the configuration object from an extension. Like stated initially, if it makes sense we should add it and worry about the generation later. I am still going to work on a console ``config:dump`` command but in another branch. --------------------------------------------------------------------------- by stof at 2011/09/04 05:36:09 -0700 @kbond could you rebase your PR ? It conflicts with the current master branch. Apart from that, the way to add info on the node seems good to me. @schmittjoh what do you think ? --------------------------------------------------------------------------- by kbond at 2011/09/04 06:59:29 -0700 Will do, give me a few days (on vacation) --------------------------------------------------------------------------- by kbond at 2011/09/06 08:48:55 -0700 @stof should i squash the commits into 1? --------------------------------------------------------------------------- by stof at 2011/09/06 09:00:46 -0700 If you want, but it is not necessary here IMO. the history is not a mess as in some other PR with lots of changes. --------------------------------------------------------------------------- by kbond at 2011/09/06 10:14:32 -0700 Ok, rebased. --------------------------------------------------------------------------- by lsmith77 at 2011/09/15 07:49:17 -0700 @kbond will you be around for the IRC meeting in 10mins? --------------------------------------------------------------------------- by kbond at 2011/09/22 08:13:37 -0700 There is a rudimentary config dump command based on this PR available for testing here: https://github.com/kbond/symfony/tree/config_dump_command --------------------------------------------------------------------------- by stof at 2011/10/16 11:03:57 -0700 @fabpot @schmittjoh ping --------------------------------------------------------------------------- by stof at 2011/11/11 07:01:12 -0800 @fabpot ping --------------------------------------------------------------------------- by kbond at 2011/11/22 07:14:58 -0800 I added a new interface as discussed in the irc meeting. Is this ok? --------------------------------------------------------------------------- by stof at 2011/12/12 12:38:38 -0800 @fabpot this PR conflicts with master once again, needing to rebase it. It would be good to review it so that we don't need to keep it pending for each further change in master @kbond can you do the rebase ? --------------------------------------------------------------------------- by kbond at 2011/12/12 19:35:05 -0800 rebased. --------------------------------------------------------------------------- by fabpot at 2011/12/13 02:56:02 -0800 @kbond: Can you squash your commits before I merge this PR? Thanks. --------------------------------------------------------------------------- by kbond at 2011/12/13 03:09:09 -0800 @fabpot done.
- Loading branch information
