Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

[2.1] [Config] added ability to set info message to node definition #1099

Merged
merged 1 commit into from

7 participants

@kbond

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
// ...
$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.

@Seldaek
Collaborator

:+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.

@Problematic

+1, even if the only thing it does is save me some WTFs later looking through my own code.

@weaverryan
Collaborator

@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?

@lsmith77
Collaborator

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.

@stof
Collaborator

@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)

@kbond

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.

@stof
Collaborator

@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.

@kbond

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.

@kbond

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.

@kbond

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.

@stof
Collaborator

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.

@kbond

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).

@stof
Collaborator

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.
@kbond

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.

...sseticBundle/DependencyInjection/AsseticExtension.php
@@ -228,4 +229,9 @@ class AsseticExtension extends Extension
{
return 'http://symfony.com/schema/dic/assetic';
}
+
+ public function getConfiguration(ContainerInterface $container)
@stof Collaborator
stof added a note

You should use this method to instantiate the Configuration in the load method.

@kbond
kbond added a note

AsseticExtension: The processing of the config is done in a static function - is there a reason for this?

SecurityExtension: the $configs array passed to Extension::load is required to build the configuration - should I make the $configs array an optional parameter for Extension::getConfiguration.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
...trineBundle/DependencyInjection/DoctrineExtension.php
@@ -412,4 +413,9 @@ class DoctrineExtension extends AbstractDoctrineExtension
{
return 'http://symfony.com/schema/dic/doctrine';
}
+
@stof Collaborator
stof added a note

You need to remove trailing whitespaces.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
src/Symfony/Bundle/FrameworkBundle/Command/Command.php
@@ -34,4 +37,27 @@ abstract class Command extends BaseCommand
{
$this->container = $this->getApplication()->getKernel()->getContainer();
}
+
+ /**
+ * Loads the ContainerBuilder from the cache.
+ *
+ * @return ContainerBuilder
+ */
+ protected function getContainerBuilder()
@stof Collaborator
stof added a note

I don't think this should be in the base command. The ContainerBuilder is only needed for some debugging commands, not for the user-land commands.

@kbond
kbond added a note

Should I add a DebugCommand base class that extends Command that both ContainerDebug and ConsoleDebug extend from?

@stof Collaborator
stof added a note

it would be better, yes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
.../Bundle/FrameworkBundle/Command/ConfigDumpCommand.php
((120 lines not shown))
+ * @param int $indent
+ */
+ protected function outputLine($text, $indent = 0)
+ {
+ $indent = strlen($text) + $indent;
+
+ $format = '%'.$indent.'s';
+
+ $this->output->writeln(sprintf($format, $text));
+ }
+
+ /**
+ * @param NodeInterface $node
+ * @param int $depth
+ */
+ protected function outputNode(NodeInterface $node, $depth = 0)
@stof Collaborator
stof added a note

these method should be private.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
.../Bundle/FrameworkBundle/Command/ConfigDumpCommand.php
((70 lines not shown))
+ $name = $input->getArgument('name');
+
+ $extension = null;
+
+ if (preg_match('/Bundle$/', $name)) {
+ // input is bundle name
+ $extension = $kernel->getBundle($name)->getContainerExtension();
+
+ if (!$extension) {
+ throw new \LogicException('No extensions with configuration available for "'.$name.'"');
+ }
+
+ $message = 'Default configuration for "'.$name.'"';
+ } else {
+ foreach ($kernel->getBundles() as $bundle) {
+ $extension = $bundle->getContainerExtension();
@stof Collaborator
stof added a note

this is wrong as it only allows to use the default extensions. You should use ContainerBuilder::getExtension instead (and it will give you the good extension directly.

@kbond
kbond added a note

Yeah I tried to to that but got bogged down as the Extensions are not registered yet in this instance of the ContainerBuilder

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
.../Bundle/FrameworkBundle/Command/ConfigDumpCommand.php
((83 lines not shown))
+ } else {
+ foreach ($kernel->getBundles() as $bundle) {
+ $extension = $bundle->getContainerExtension();
+
+ if ($extension && ($extension->getAlias() == $name)) {
+ break;
+ }
+
+ $extension = null;
+ }
+
+ $message = 'Default configuration for extension with alias: "'.$name.'"';
+ }
+
+ if (!$extension) {
+ throw new \LogicException('No extension with alias "'.$name.'" is enabled');
@stof Collaborator
stof added a note

This is not necessary here. using the bundle name still throw the LogicException, and getting it by the alias would also throw it if you were using the good way to go.
And otherwise, it should be placed inside the else to avoid a useless check when using the bundle name.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
.../Bundle/FrameworkBundle/Command/ConfigDumpCommand.php
((133 lines not shown))
+ * @param int $depth
+ */
+ protected function outputNode(NodeInterface $node, $depth = 0)
+ {
+ $text = $node->getName().':';
+ $default = '';
+
+ // default value
+ if (!($node instanceof ArrayNode)) {
+ $default = '~';
+ // set default
+ if ($node->hasDefaultValue()) {
+ $default = $node->getDefaultValue();
+
+ if ($default === true) $default = 'true';
+ if ($default === false) $default = 'false';
@stof Collaborator
stof added a note

This does not follow the Sf2 CS. You need to put the curly braces in all cases. See http://symfony.com/doc/current/contributing/code/standards.html

and you should also inverse the order of the check, putting the variable on the right.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
.../Bundle/FrameworkBundle/Command/ConfigDumpCommand.php
((166 lines not shown))
+
+ $this->outputLine($text, $depth * 4);
+
+ if ($node instanceof ArrayNode) {
+ $this->outputArrayNode($node, $depth);
+ }
+ }
+
+ /**
+ * @param ArrayNode $arrayNode
+ * @param int $depth
+ */
+ protected function outputArrayNode(ArrayNode $arrayNode, $depth)
+ {
+ foreach ($arrayNode->getChildren() as $node) {
+ $this->outputNode($node, $depth + 1);
@stof Collaborator
stof added a note

I don't see the point of using another method for the loop as it is used only in one place, especially when it is only 3 LOC.
This adds unnecessary method calls.

@kbond
kbond added a note

Yeah this entire command needs refactoring. What do you think about moving the generation logic to a ReferenceBuilder (name/location?) class? That way it could be used outside the console command (ie profiler tab). Also, it currently just generates the yaml config, it should be able to output xml/php as well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@kbond

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.

@stof
Collaborator

@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 ?

@kbond

Will do, give me a few days (on vacation)

@kbond

@stof should i squash the commits into 1?

@stof
Collaborator

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.

@kbond

Ok, rebased.

@lsmith77
Collaborator

@kbond will you be around for the IRC meeting in 10mins?

@kbond

There is a rudimentary config dump command based on this PR available for testing here: https://github.com/kbond/symfony/tree/config_dump_command

@stof
Collaborator
@stof
Collaborator

@fabpot ping

.../DependencyInjection/Extension/ExtensionInterface.php
@@ -62,4 +64,14 @@ interface ExtensionInterface
* @api
*/
function getAlias();
+
+ /**
+ * Returns extension configuration
+ *
+ * @param array $config $config An array of configuration values
+ * @param ContainerBuilder $container A ContainerBuilder instance
+ *
+ * @return ConfigurationInterface|null The configuration or null
+ */
+ function getConfiguration(array $config, ContainerBuilder $container);
@fabpot Owner
fabpot added a note

This class has the @api tag, which means that we cannot change it anymore. Is it possible to create another interface and only allow doc generation if you implement this new interface. I know that it will complexify things a bit, but we cannot break the stable API anymore.

@stof Collaborator
stof added a note

And this new interface should logically be located in the Config component

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@kbond

I added a new interface as discussed in the irc meeting. Is this ok?

@stof stof commented on the diff
...jection/Extension/ConfigurationExtensionInterface.php
((6 lines not shown))
+ * (c) Fabien Potencier <fabien@symfony.com>
+ *
+ * For the full copyright and license information, please view the LICENSE
+ * file that was distributed with this source code.
+ */
+
+namespace Symfony\Component\DependencyInjection\Extension;
+
+use Symfony\Component\DependencyInjection\ContainerBuilder;
+
+/**
+ * ConfigurationExtensionInterface is the interface implemented by container extension classes.
+ *
+ * @author Kevin Bond <kevinbond@gmail.com>
+ */
+interface ConfigurationExtensionInterface
@stof Collaborator
stof added a note

this interface has nothing to do with the DI extension (it could be implemented by other code using the Config\Definition component) so it should be placed in the Config component and renamed (maybe ConfigurableInterface ?)

@stof Collaborator
stof added a note

hmm, in fact, it is linked to the DI component because of the signature. Do we really need to pass a ContainerBuilder to the method ?

@kbond
kbond added a note

I originally didn't have the ContainerBuilder passed but if you look at how the configuration is retrieved in the various core bundle extensions, it is needed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@stof stof commented on the diff
...omponent/HttpKernel/DependencyInjection/Extension.php
@@ -21,7 +24,7 @@ use Symfony\Component\DependencyInjection\Container;
*
* @author Fabien Potencier <fabien@symfony.com>
*/
-abstract class Extension implements ExtensionInterface
+abstract class Extension implements ExtensionInterface, ConfigurationExtensionInterface
@stof Collaborator
stof added a note

now that it is a separate interface, I would find it more logical to implement it only when we really have the Configuration class instead of implementing the interface and returning null.

@kbond
kbond added a note

I like it as is because if your bundle's configuration is just a basic Configurationclass with no special constructor it automatically will be able to be retrieved by a future documentation generator.

Having users add implements ConfigurationExtensionInterface to their bundle's Extension class seems a little verbose, no? This way many bundles be able to have their configuration documentation generated with no additions on their end.

@stof Collaborator
stof added a note

as the DI extension is generated by SensioGeneratorBundle when creating a bundle, this is not really an issue. We simply need to update the bundle too.

But currently, the command needs to do 2 checks: does it implement the interface ? And if yes, does it return null or a Configuration instance ?

@kbond
kbond added a note

You are right, the command would need both checks.

I suppose I was trying to make it as BC as possible. Most 3rd party bundles would work out of the box currently.

@stof Collaborator
stof added a note

If you implement the interface here, it will break any third party bundle passing extra parameters.

If you implement the interface in the actual extension class, there will be no issue for third party bundles. They will simply have to update their code to be able to use the doc generator.

@kbond
kbond added a note

I don't see how it will break the 3rd party bundle. The default getConfiguration will just return null if extra parameters are passed (the __constructor check). It is documented in the interface that the return can be null or ConfigurationInterface.

I currently check for that in the command and give an error if null (kbond@a647540#L0R105)

I am not trying to be a stick in the mud about this I just would like to avoid having to rely on the third party bundles to update their code for this to work.

@stof Collaborator
stof added a note

ah, I forgot the constructor check.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@stof
Collaborator

@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 ?

@kbond

rebased.

@fabpot
Owner

@kbond: Can you squash your commits before I merge this PR? Thanks.

@kbond

@fabpot done.

@fabpot fabpot referenced this pull request from a commit
@fabpot fabpot 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.
4c00660
@fabpot fabpot merged commit 73ac773 into symfony:master
@fabpot fabpot referenced this pull request from a commit
@fabpot fabpot merged branch kbond/config_dump_command (PR #3187)
Commits
-------

4847d3a renamed command
e97af0b code fixes
df94282 [FrameworkBundle] removed unnecessary DebugCommand
fa32885 [SecurityBundle] added configuration info
2f8ad93 [MonologBundle] added configuration info
9757958 [FrameworkBundle] added configuration info
58939f1 [TwigBundle] added configuration docs
8dc40e4 [FrameworkBundle] added config:dump console command

Discussion
----------

Config dump command

Bug fix: no
Feature addition: yes
Backwards compatibility break: no
Symfony2 tests pass: yes
Fixes the following tickets: #1663
Todo: add more config info/examples

[![Build Status](https://secure.travis-ci.org/kbond/symfony.png?branch=config_dump_command)](http://travis-ci.org/kbond/symfony)

This is a config dump command based on the additions of PR #1099.  This was initially part of that PR and there is some discussion there about it (#1099)

### Usage:

1. dump by root node: ``app/console config:dump framework``
2. dump by bundle name: ``app/console config:dump FrameworkBundle``

A few issues/notes:

* Only dumps to yaml
* Only 1 configuration per bundle (this was brought by @stof here: #1099 (comment))
* Works out of the box for most bundles but not ones that use a non-standard ``Configuration`` class (such as the assetic bundle).  In this case ``Extension::getConfiguration()`` must be configurated.

I have used it to create some (most) of the config reference docs.  It works fine but I find it somewhat crude, any suggestions to improve it would be appreciated.

---------------------------------------------------------------------------

by kbond at 2012-01-24T21:00:43Z

Few more issues:

1. Should I abstract the logic to a "normalizer" class that converts the Configuration class into a manageable array?  I struggle with this idea because isn't that what ``TreeBuilder`` basically is?
2. @stof made a good point that ``config:dump`` doesn't really describe what this does.  Would dumping your config be useful?  Perhaps ``config:dump framework`` dumps the config for your project while ``config:dump --ref framework`` dumps the default reference?

---------------------------------------------------------------------------

by stof at 2012-01-24T21:18:15Z

@kbond you cannot really dump the config. Part of it does not go through these extensions at all. And it does not make much sense anyway IMO.
the command as is does the right job IMO (i.e. dumping a reference for the extension). But its name should be improved

---------------------------------------------------------------------------

by kbond at 2012-01-24T21:20:51Z

``config:reference`` perhaps?

---------------------------------------------------------------------------

by fabpot at 2012-02-02T10:05:19Z

This command is about displaying the default configuration for a given bundle. So, what about `config:dump-reference`? As I understand, the command name is the last element to figure out before merging, right?

---------------------------------------------------------------------------

by stof at 2012-02-02T10:19:49Z

@fabpot indeed.

---------------------------------------------------------------------------

by stof at 2012-02-02T10:34:16Z

and +1 for ``config:dump-reference``

---------------------------------------------------------------------------

by Tobion at 2012-02-02T12:08:03Z

why not use the words you chose yourself: `config:dump-default`
I think it's more explicit.
79a957b
@mmucklo mmucklo referenced this pull request from a commit
@fabpot fabpot 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.
30989ee
@mmucklo mmucklo referenced this pull request from a commit
@fabpot fabpot merged branch kbond/config_dump_command (PR #3187)
Commits
-------

4847d3a renamed command
e97af0b code fixes
df94282 [FrameworkBundle] removed unnecessary DebugCommand
fa32885 [SecurityBundle] added configuration info
2f8ad93 [MonologBundle] added configuration info
9757958 [FrameworkBundle] added configuration info
58939f1 [TwigBundle] added configuration docs
8dc40e4 [FrameworkBundle] added config:dump console command

Discussion
----------

Config dump command

Bug fix: no
Feature addition: yes
Backwards compatibility break: no
Symfony2 tests pass: yes
Fixes the following tickets: #1663
Todo: add more config info/examples

[![Build Status](https://secure.travis-ci.org/kbond/symfony.png?branch=config_dump_command)](http://travis-ci.org/kbond/symfony)

This is a config dump command based on the additions of PR #1099.  This was initially part of that PR and there is some discussion there about it (#1099)

### Usage:

1. dump by root node: ``app/console config:dump framework``
2. dump by bundle name: ``app/console config:dump FrameworkBundle``

A few issues/notes:

* Only dumps to yaml
* Only 1 configuration per bundle (this was brought by @stof here: #1099 (comment))
* Works out of the box for most bundles but not ones that use a non-standard ``Configuration`` class (such as the assetic bundle).  In this case ``Extension::getConfiguration()`` must be configurated.

I have used it to create some (most) of the config reference docs.  It works fine but I find it somewhat crude, any suggestions to improve it would be appreciated.

---------------------------------------------------------------------------

by kbond at 2012-01-24T21:00:43Z

Few more issues:

1. Should I abstract the logic to a "normalizer" class that converts the Configuration class into a manageable array?  I struggle with this idea because isn't that what ``TreeBuilder`` basically is?
2. @stof made a good point that ``config:dump`` doesn't really describe what this does.  Would dumping your config be useful?  Perhaps ``config:dump framework`` dumps the config for your project while ``config:dump --ref framework`` dumps the default reference?

---------------------------------------------------------------------------

by stof at 2012-01-24T21:18:15Z

@kbond you cannot really dump the config. Part of it does not go through these extensions at all. And it does not make much sense anyway IMO.
the command as is does the right job IMO (i.e. dumping a reference for the extension). But its name should be improved

---------------------------------------------------------------------------

by kbond at 2012-01-24T21:20:51Z

``config:reference`` perhaps?

---------------------------------------------------------------------------

by fabpot at 2012-02-02T10:05:19Z

This command is about displaying the default configuration for a given bundle. So, what about `config:dump-reference`? As I understand, the command name is the last element to figure out before merging, right?

---------------------------------------------------------------------------

by stof at 2012-02-02T10:19:49Z

@fabpot indeed.

---------------------------------------------------------------------------

by stof at 2012-02-02T10:34:16Z

and +1 for ``config:dump-reference``

---------------------------------------------------------------------------

by Tobion at 2012-02-02T12:08:03Z

why not use the words you chose yourself: `config:dump-default`
I think it's more explicit.
38304a6
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
This page is out of date. Refresh to see the latest.
Showing with 236 additions and 11 deletions.
  1. +6 −1 src/Symfony/Bundle/DoctrineBundle/DependencyInjection/DoctrineExtension.php
  2. +8 −1 src/Symfony/Bundle/FrameworkBundle/DependencyInjection/FrameworkExtension.php
  3. +1 −1  src/Symfony/Bundle/MonologBundle/DependencyInjection/MonologExtension.php
  4. +9 −3 src/Symfony/Bundle/SecurityBundle/DependencyInjection/SecurityExtension.php
  5. +6 −1 src/Symfony/Bundle/SwiftmailerBundle/DependencyInjection/SwiftmailerExtension.php
  6. +1 −1  src/Symfony/Bundle/TwigBundle/DependencyInjection/TwigExtension.php
  7. +1 −1  src/Symfony/Bundle/WebProfilerBundle/DependencyInjection/WebProfilerExtension.php
  8. +10 −0 src/Symfony/Component/Config/Definition/ArrayNode.php
  9. +42 −0 src/Symfony/Component/Config/Definition/BaseNode.php
  10. +36 −1 src/Symfony/Component/Config/Definition/Builder/NodeDefinition.php
  11. +20 −0 src/Symfony/Component/Config/Definition/PrototypedArrayNode.php
  12. +32 −0 src/Symfony/Component/DependencyInjection/Extension/ConfigurationExtensionInterface.php
  13. +24 −1 src/Symfony/Component/HttpKernel/DependencyInjection/Extension.php
  14. +35 −0 tests/Symfony/Tests/Component/Config/Definition/Builder/TreeBuilderTest.php
  15. +5 −0 tests/Symfony/Tests/Component/DependencyInjection/Fixtures/includes/ProjectExtension.php
View
7 src/Symfony/Bundle/DoctrineBundle/DependencyInjection/DoctrineExtension.php
@@ -31,7 +31,7 @@ class DoctrineExtension extends AbstractDoctrineExtension
{
public function load(array $configs, ContainerBuilder $container)
{
- $configuration = new Configuration($container->getParameter('kernel.debug'));
+ $configuration = $this->getConfiguration($configs, $container);
$config = $this->processConfiguration($configuration, $configs);
if (!empty($config['dbal'])) {
@@ -412,4 +412,9 @@ public function getNamespace()
{
return 'http://symfony.com/schema/dic/doctrine';
}
+
+ public function getConfiguration(array $config, ContainerBuilder $container)
+ {
+ return new Configuration($container->getParameter('kernel.debug'));
+ }
}
View
9 src/Symfony/Bundle/FrameworkBundle/DependencyInjection/FrameworkExtension.php
@@ -13,6 +13,8 @@
use Symfony\Component\Config\Loader\LoaderInterface;
use Symfony\Component\DependencyInjection\ContainerBuilder;
+use Symfony\Component\DependencyInjection\ContainerInterface;
+use Symfony\Component\DependencyInjection\Definition;
use Symfony\Component\DependencyInjection\DefinitionDecorator;
use Symfony\Component\DependencyInjection\Reference;
use Symfony\Component\DependencyInjection\Loader\XmlFileLoader;
@@ -56,7 +58,7 @@ public function load(array $configs, ContainerBuilder $container)
$container->setAlias('debug.controller_resolver', 'controller_resolver');
}
- $configuration = new Configuration($container->getParameter('kernel.debug'));
+ $configuration = $this->getConfiguration($configs, $container);
$config = $this->processConfiguration($configuration, $configs);
if (isset($config['charset'])) {
@@ -146,6 +148,11 @@ public function load(array $configs, ContainerBuilder $container)
));
}
+ public function getConfiguration(array $config, ContainerBuilder $container)
+ {
+ return new Configuration($container->getParameter('kernel.debug'));
+ }
+
/**
* Loads Form configuration.
*
View
2  src/Symfony/Bundle/MonologBundle/DependencyInjection/MonologExtension.php
@@ -36,7 +36,7 @@ class MonologExtension extends Extension
*/
public function load(array $configs, ContainerBuilder $container)
{
- $configuration = new Configuration();
+ $configuration = $this->getConfiguration($configs, $container);
$config = $this->processConfiguration($configuration, $configs);
if (isset($config['handlers'])) {
View
12 src/Symfony/Bundle/SecurityBundle/DependencyInjection/SecurityExtension.php
@@ -48,9 +48,9 @@ public function load(array $configs, ContainerBuilder $container)
if (!array_filter($configs)) {
return;
}
-
- // normalize and merge the actual configuration
- $mainConfig = new MainConfiguration($this->factories, $this->userProviderFactories);
+
+ $mainConfig = $this->getConfiguration($configs, $container);
+
$config = $this->processConfiguration($mainConfig, $configs);
// load services
@@ -569,5 +569,11 @@ public function getNamespace()
{
return 'http://symfony.com/schema/dic/security';
}
+
+ public function getConfiguration(array $config, ContainerBuilder $container)
+ {
+ // first assemble the factories
+ return new MainConfiguration($this->factories, $this->userProviderFactories);
+ }
}
View
7 src/Symfony/Bundle/SwiftmailerBundle/DependencyInjection/SwiftmailerExtension.php
@@ -43,7 +43,7 @@ public function load(array $configs, ContainerBuilder $container)
$loader->load('swiftmailer.xml');
$container->setAlias('mailer', 'swiftmailer.mailer');
- $configuration = new Configuration($container->getParameter('kernel.debug'));
+ $configuration = $this->getConfiguration($configs, $container);
$config = $this->processConfiguration($configuration, $configs);
if (null === $config['transport']) {
@@ -145,4 +145,9 @@ public function getNamespace()
{
return 'http://symfony.com/schema/dic/swiftmailer';
}
+
+ public function getConfiguration(array $config, ContainerBuilder $container)
+ {
+ return new Configuration($container->getParameter('kernel.debug'));
+ }
}
View
2  src/Symfony/Bundle/TwigBundle/DependencyInjection/TwigExtension.php
@@ -36,7 +36,7 @@ public function load(array $configs, ContainerBuilder $container)
$loader = new XmlFileLoader($container, new FileLocator(__DIR__.'/../Resources/config'));
$loader->load('twig.xml');
- $configuration = new Configuration();
+ $configuration = $this->getConfiguration($configs, $container);
$config = $this->processConfiguration($configuration, $configs);
$container->setParameter('twig.exception_listener.controller', $config['exception_controller']);
View
2  src/Symfony/Bundle/WebProfilerBundle/DependencyInjection/WebProfilerExtension.php
@@ -39,7 +39,7 @@ class WebProfilerExtension extends Extension
*/
public function load(array $configs, ContainerBuilder $container)
{
- $configuration = new Configuration();
+ $configuration = $this->getConfiguration($configs, $container);
$config = $this->processConfiguration($configuration, $configs);
$loader = new XmlFileLoader($container, new FileLocator(__DIR__.'/../Resources/config'));
View
10 src/Symfony/Component/Config/Definition/ArrayNode.php
@@ -49,6 +49,16 @@ public function __construct($name, NodeInterface $parent = null)
$this->allowNewKeys = true;
$this->performDeepMerging = true;
}
+
+ /**
+ * Retrieves the children of this node.
+ *
+ * @return array The children
+ */
+ public function getChildren()
+ {
+ return $this->children;
+ }
/**
* Sets the xml remappings that should be performed.
View
42 src/Symfony/Component/Config/Definition/BaseNode.php
@@ -29,6 +29,8 @@
protected $allowOverwrite;
protected $required;
protected $equivalentValues;
+ protected $info;
+ protected $example;
/**
* Constructor.
@@ -51,6 +53,46 @@ public function __construct($name, NodeInterface $parent = null)
$this->required = false;
$this->equivalentValues = array();
}
+
+ /**
+ * Sets info message
+ *
+ * @param string $info The info text
+ */
+ public function setInfo($info)
+ {
+ $this->info = $info;
+ }
+
+ /**
+ * Returns info message
+ *
+ * @return string The info text
+ */
+ public function getInfo()
+ {
+ return $this->info;
+ }
+
+ /**
+ * Sets the example configuration for this node.
+ *
+ * @param array $example
+ */
+ public function setExample($example)
+ {
+ $this->example = $example;
+ }
+
+ /**
+ * Retrieves the example configuration for this node.
+ *
+ * @return mixed The example
+ */
+ public function getExample()
+ {
+ return $this->example;
+ }
/**
* Adds an equivalent value.
View
37 src/Symfony/Component/Config/Definition/Builder/NodeDefinition.php
@@ -32,6 +32,8 @@
protected $trueEquivalent;
protected $falseEquivalent;
protected $parent;
+ protected $info;
+ protected $example;
/**
* Constructor
@@ -60,6 +62,34 @@ public function setParent(NodeParentInterface $parent)
return $this;
}
+
+ /**
+ * Sets info message
+ *
+ * @param string $info The info text
+ *
+ * @return NodeDefinition
+ */
+ public function setInfo($info)
+ {
+ $this->info = $info;
+
+ return $this;
+ }
+
+ /**
+ * Sets example configuration
+ *
+ * @param example $example
+ *
+ * @return NodeDefinition
+ */
+ public function setExample($example)
+ {
+ $this->example = $example;
+
+ return $this;
+ }
/**
* Returns the parent node.
@@ -91,8 +121,13 @@ public function getNode($forceRootNode = false)
if (null !== $this->validation) {
$this->validation->rules = ExprBuilder::buildExpressions($this->validation->rules);
}
+
+ $node = $this->createNode();
+
+ $node->setInfo($this->info);
+ $node->setExample($this->example);
- return $this->createNode();
+ return $node;
}
/**
View
20 src/Symfony/Component/Config/Definition/PrototypedArrayNode.php
@@ -79,6 +79,16 @@ public function setKeyAttribute($attribute, $remove = true)
}
/**
+ * Retrieves the name of the attribute which value should be used as key.
+ *
+ * @return string The name of the attribute
+ */
+ public function getKeyAttribute()
+ {
+ return $this->keyAttribute;
+ }
+
+ /**
* Sets the default value of this node.
*
* @param string $value
@@ -122,6 +132,16 @@ public function setPrototype(PrototypeNodeInterface $node)
{
$this->prototype = $node;
}
+
+ /**
+ * Retrieves the prototype
+ *
+ * @return PrototypeNodeInterface The prototype
+ */
+ public function getPrototype()
+ {
+ return $this->prototype;
+ }
/**
* Disable adding concrete children for prototyped nodes.
View
32 src/Symfony/Component/DependencyInjection/Extension/ConfigurationExtensionInterface.php
@@ -0,0 +1,32 @@
+<?php
+
+/*
+ * This file is part of the Symfony package.
+ *
+ * (c) Fabien Potencier <fabien@symfony.com>
+ *
+ * For the full copyright and license information, please view the LICENSE
+ * file that was distributed with this source code.
+ */
+
+namespace Symfony\Component\DependencyInjection\Extension;
+
+use Symfony\Component\DependencyInjection\ContainerBuilder;
+
+/**
+ * ConfigurationExtensionInterface is the interface implemented by container extension classes.
+ *
+ * @author Kevin Bond <kevinbond@gmail.com>
+ */
+interface ConfigurationExtensionInterface
@stof Collaborator
stof added a note

this interface has nothing to do with the DI extension (it could be implemented by other code using the Config\Definition component) so it should be placed in the Config component and renamed (maybe ConfigurableInterface ?)

@stof Collaborator
stof added a note

hmm, in fact, it is linked to the DI component because of the signature. Do we really need to pass a ContainerBuilder to the method ?

@kbond
kbond added a note

I originally didn't have the ContainerBuilder passed but if you look at how the configuration is retrieved in the various core bundle extensions, it is needed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+{
+ /**
+ * Returns extension configuration
+ *
+ * @param array $config $config An array of configuration values
+ * @param ContainerBuilder $container A ContainerBuilder instance
+ *
+ * @return ConfigurationInterface|null The configuration or null
+ */
+ function getConfiguration(array $config, ContainerBuilder $container);
+}
View
25 src/Symfony/Component/HttpKernel/DependencyInjection/Extension.php
@@ -5,6 +5,9 @@
use Symfony\Component\Config\Definition\Processor;
use Symfony\Component\Config\Definition\ConfigurationInterface;
use Symfony\Component\DependencyInjection\Extension\ExtensionInterface;
+use Symfony\Component\DependencyInjection\Extension\ConfigurationExtensionInterface;
+use Symfony\Component\DependencyInjection\ContainerInterface;
+use Symfony\Component\DependencyInjection\ContainerBuilder;
use Symfony\Component\DependencyInjection\Container;
/*
@@ -21,7 +24,7 @@
*
* @author Fabien Potencier <fabien@symfony.com>
*/
-abstract class Extension implements ExtensionInterface
+abstract class Extension implements ExtensionInterface, ConfigurationExtensionInterface
@stof Collaborator
stof added a note

now that it is a separate interface, I would find it more logical to implement it only when we really have the Configuration class instead of implementing the interface and returning null.

@kbond
kbond added a note

I like it as is because if your bundle's configuration is just a basic Configurationclass with no special constructor it automatically will be able to be retrieved by a future documentation generator.

Having users add implements ConfigurationExtensionInterface to their bundle's Extension class seems a little verbose, no? This way many bundles be able to have their configuration documentation generated with no additions on their end.

@stof Collaborator
stof added a note

as the DI extension is generated by SensioGeneratorBundle when creating a bundle, this is not really an issue. We simply need to update the bundle too.

But currently, the command needs to do 2 checks: does it implement the interface ? And if yes, does it return null or a Configuration instance ?

@kbond
kbond added a note

You are right, the command would need both checks.

I suppose I was trying to make it as BC as possible. Most 3rd party bundles would work out of the box currently.

@stof Collaborator
stof added a note

If you implement the interface here, it will break any third party bundle passing extra parameters.

If you implement the interface in the actual extension class, there will be no issue for third party bundles. They will simply have to update their code to be able to use the doc generator.

@kbond
kbond added a note

I don't see how it will break the 3rd party bundle. The default getConfiguration will just return null if extra parameters are passed (the __constructor check). It is documented in the interface that the return can be null or ConfigurationInterface.

I currently check for that in the command and give an error if null (kbond@a647540#L0R105)

I am not trying to be a stick in the mud about this I just would like to avoid having to rely on the third party bundles to update their code for this to work.

@stof Collaborator
stof added a note

ah, I forgot the constructor check.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
{
private $classes = array();
@@ -100,4 +103,24 @@ public function getAlias()
return $processor->processConfiguration($configuration, $configs);
}
+
+ /**
+ * {@inheritDoc}
+ */
+ public function getConfiguration(array $config, ContainerBuilder $container)
+ {
+ $reflected = new \ReflectionClass($this);
+ $namespace = $reflected->getNamespaceName();
+
+ $class = $namespace . '\\Configuration';
+ if (class_exists($class)) {
+ if (!method_exists($class, '__construct')) {
+ $configuration = new $class();
+
+ return $configuration;
+ }
+ }
+
+ return null;
+ }
}
View
35 tests/Symfony/Tests/Component/Config/Definition/Builder/TreeBuilderTest.php
@@ -89,4 +89,39 @@ public function testAnExtendedNodeBuilderGetsPropagatedToTheChildren()
->end()
->end();
}
+
+ public function testDefinitionInfoGetsTransferedToNode()
+ {
+ $builder = new TreeBuilder();
+
+ $builder->root('test')->setInfo('root info')
+ ->children()
+ ->node('child', 'variable')->setInfo('child info')->defaultValue('default')
+ ->end()
+ ->end();
+
+ $tree = $builder->buildTree();
+ $children = $tree->getChildren();
+
+ $this->assertEquals('root info', $tree->getInfo());
+ $this->assertEquals('child info', $children['child']->getInfo());
+ }
+
+ public function testDefinitionExampleGetsTransferedToNode()
+ {
+ $builder = new TreeBuilder();
+
+ $builder->root('test')
+ ->setExample(array('key' => 'value'))
+ ->children()
+ ->node('child', 'variable')->setInfo('child info')->defaultValue('default')->setExample('example')
+ ->end()
+ ->end();
+
+ $tree = $builder->buildTree();
+ $children = $tree->getChildren();
+
+ $this->assertTrue(is_array($tree->getExample()));
+ $this->assertEquals('example', $children['child']->getExample());
+ }
}
View
5 tests/Symfony/Tests/Component/DependencyInjection/Fixtures/includes/ProjectExtension.php
@@ -33,4 +33,9 @@ public function getAlias()
{
return 'project';
}
+
+ public function getConfiguration(array $config, ContainerBuilder $container)
+ {
+ return null;
+ }
}
Something went wrong with that request. Please try again.