[DIC] Better handling of enableable configurations #6852

Closed
wants to merge 12 commits into
from

Conversation

Projects
None yet
5 participants
@vicb
Contributor

vicb commented Jan 23, 2013

Q A
Bug fix? yes
New feature? no
BC breaks? no, this feature has not been released yet
Deprecations? no
Tests pass? yes
Fixed tickets -
License MIT
Doc PR -

My definition of bug fix might be discussable. The thing which I think is not discussable is that this PR fixes the semantic - and I think it is important for a "semantic configuration": before this PR, some nodes had ->canBeDisabled for nodes that were actually disabled by default. Those nodes now have ->canBeEnabled which sounds right.

Edit: Jan 28, 2013 - history:

See the related comments.

I think Symfony must get the configuration right as we can expect of lot of devs to use this as a template when writting their own configuration.

@schmittjoh could you please give me your feedback on this change considering the rationale.

@stof

This comment has been minimized.

Show comment
Hide comment
@stof

stof Jan 23, 2013

Member

@vicb your links are broken as they are pointing to the PR creation page

Member

stof commented Jan 23, 2013

@vicb your links are broken as they are pointing to the PR creation page

@stof

This comment has been minimized.

Show comment
Hide comment
@stof

stof Jan 23, 2013

Member

and to create a TODO list, it has to be a list first

Member

stof commented Jan 23, 2013

and to create a TODO list, it has to be a list first

@@ -91,10 +91,10 @@ private function addFormSection(ArrayNodeDefinition $rootNode)
->children()
->arrayNode('form')
->info('form configuration')
- ->canBeDisabled()
+ ->canBeEnabled()

This comment has been minimized.

@stof

stof Jan 23, 2013

Member

This is not BC with 2.1. You need to add treatNullLike(array('enabled' => true))

@stof

stof Jan 23, 2013

Member

This is not BC with 2.1. You need to add treatNullLike(array('enabled' => true))

This comment has been minimized.

This comment has been minimized.

@stof

stof Jan 23, 2013

Member

ah indeed, I forgot it

@stof

stof Jan 23, 2013

Member

ah indeed, I forgot it

This comment has been minimized.

@vicb

vicb Jan 23, 2013

Contributor

I have tried to keep BC as much as possible (some other configs should probably get updated later) but it still needs a careful review !

@vicb

vicb Jan 23, 2013

Contributor

I have tried to keep BC as much as possible (some other configs should probably get updated later) but it still needs a careful review !

@@ -213,6 +202,10 @@ private function registerRouterProxyConfiguration(array $config, ContainerBuilde
*/
private function registerProfilerConfiguration(array $config, ContainerBuilder $container, XmlFileLoader $loader)
{
+ if (!$this->isConfigEnabled($container, $config)) {
+ return;

This comment has been minimized.

@Tobion

Tobion Jan 23, 2013

Member

I think this doesn't harmonize with what is done in line 254:

if (!$config['enabled']) {
    $container->getDefinition('profiler')->addMethodCall('disable', array());
}
@Tobion

Tobion Jan 23, 2013

Member

I think this doesn't harmonize with what is done in line 254:

if (!$config['enabled']) {
    $container->getDefinition('profiler')->addMethodCall('disable', array());
}

This comment has been minimized.

@vicb

vicb Jan 23, 2013

Contributor

@Tobion could you submit a PR please ?

@vicb

vicb Jan 23, 2013

Contributor

@Tobion could you submit a PR please ?

@@ -161,7 +144,7 @@ public function load(array $configs, ContainerBuilder $container)
private function registerFormConfiguration($config, ContainerBuilder $container, XmlFileLoader $loader)
{
$loader->load('form.xml');
- if (isset($config['csrf_protection'])) {
+ if ($this->isConfigEnabled($container, $config['csrf_protection'])) {

This comment has been minimized.

@Tobion

Tobion Jan 23, 2013

Member

In line 156 you can set $container->setParameter('form.type_extension.csrf.enabled', true); because it's only set when it avaluates to true anyway.

@Tobion

Tobion Jan 23, 2013

Member

In line 156 you can set $container->setParameter('form.type_extension.csrf.enabled', true); because it's only set when it avaluates to true anyway.

@vicb

This comment has been minimized.

Show comment
Hide comment
@vicb

vicb Jan 23, 2013

Contributor

@stof thanks for reporting the broken links, they are fixed /cc @schmittjoh

Contributor

vicb commented Jan 23, 2013

@stof thanks for reporting the broken links, they are fixed /cc @schmittjoh

@vicb

This comment has been minimized.

Show comment
Hide comment
@vicb

vicb Jan 23, 2013

Contributor

@Tobion please submit a PR to my repo, I don't have much time to work on this. Thanks !

Contributor

vicb commented Jan 23, 2013

@Tobion please submit a PR to my repo, I don't have much time to work on this. Thanks !

@@ -4,6 +4,9 @@ CHANGELOG
2.2.0
-----
+ * [BC BREAK] changed ArrayNodeDefinition::canBeEnabled() and ArrayNodeDefinition::canBeDisabled()
+ to set the defaults when the node is not set - the methods were ineffective
+ if ArrayNodeDefinition::setDefaultsIfNotSet() was not explicitely called.

This comment has been minimized.

@schmittjoh

schmittjoh Jan 23, 2013

Contributor

Small typo here add not set.

Regarding the change itself, it seems fine to me.

@schmittjoh

schmittjoh Jan 23, 2013

Contributor

Small typo here add not set.

Regarding the change itself, it seems fine to me.

This comment has been minimized.

@vicb

vicb Jan 23, 2013

Contributor

Thanks for the review, I'll update tomorrow

@vicb

vicb Jan 23, 2013

Contributor

Thanks for the review, I'll update tomorrow

@vicb

This comment has been minimized.

Show comment
Hide comment
@vicb

vicb Jan 25, 2013

Contributor

@fabpot @schmittjoh I'd like your feedback on the latest commit, rationale is in the method phpDoc. It better matches what we do now and seem the most sensible thing to do.

edit: with this you can no more disable the node explicitly, I have to find a better solution

Contributor

vicb commented Jan 25, 2013

@fabpot @schmittjoh I'd like your feedback on the latest commit, rationale is in the method phpDoc. It better matches what we do now and seem the most sensible thing to do.

edit: with this you can no more disable the node explicitly, I have to find a better solution

@schmittjoh

This comment has been minimized.

Show comment
Hide comment
@schmittjoh

schmittjoh Jan 25, 2013

Contributor

Looks good.

On Fri, Jan 25, 2013 at 4:15 PM, Victor Berchet notifications@github.comwrote:

@fabpot https://github.com/fabpot @schmittjohhttps://github.com/schmittjohI'd like your feedback on the latest commit, rationale is in the method
phpDoc. It better matches what we do now and seem the most sensible thing
to do.


Reply to this email directly or view it on GitHubhttps://github.com/symfony/symfony/pull/6852#issuecomment-12704585.

Contributor

schmittjoh commented Jan 25, 2013

Looks good.

On Fri, Jan 25, 2013 at 4:15 PM, Victor Berchet notifications@github.comwrote:

@fabpot https://github.com/fabpot @schmittjohhttps://github.com/schmittjohI'd like your feedback on the latest commit, rationale is in the method
phpDoc. It better matches what we do now and seem the most sensible thing
to do.


Reply to this email directly or view it on GitHubhttps://github.com/symfony/symfony/pull/6852#issuecomment-12704585.

-
- if (isset($config['router_proxy'])) {
- $this->registerRouterProxyConfiguration($config['router_proxy'], $container, $loader);
+ if (isset($config['templating'])) {

This comment has been minimized.

@stof

stof Jan 26, 2013

Member

why keeping a isset here while you use an early return in other methods ?

@stof

stof Jan 26, 2013

Member

why keeping a isset here while you use an early return in other methods ?

This comment has been minimized.

@vicb

vicb Jan 26, 2013

Contributor

@stof For now this PR only fixes the current config and templating is not an enableable node as of today.

@vicb

vicb Jan 26, 2013

Contributor

@stof For now this PR only fixes the current config and templating is not an enableable node as of today.

@vicb

This comment has been minimized.

Show comment
Hide comment
@vicb

vicb Jan 28, 2013

Contributor

@fabpot I know I keep insisting on this one and I am sorry for that but I think this should be considered as a bug fix (see the PR header for details) and should be merged in 2.2. I think the Symfony core should be exemplary as it is used by many developers as a template when creating their own bundle. This PR is no more a WIP and can be merged right now.

In addition to fixing the enableable nodes, this PR contain new UTs and some fixes to the code / tests.

Contributor

vicb commented Jan 28, 2013

@fabpot I know I keep insisting on this one and I am sorry for that but I think this should be considered as a bug fix (see the PR header for details) and should be merged in 2.2. I think the Symfony core should be exemplary as it is used by many developers as a template when creating their own bundle. This PR is no more a WIP and can be merged right now.

In addition to fixing the enableable nodes, this PR contain new UTs and some fixes to the code / tests.

@@ -4,6 +4,9 @@ CHANGELOG
2.2.0
-----
+ * [BC BREAK] changed ArrayNodeDefinition::canBeEnabled() and ArrayNodeDefinition::canBeDisabled()

This comment has been minimized.

@fabpot

fabpot Jan 28, 2013

Member

Actually, this is not a BC break as those methods have been introduced in 2.2 (see #5688). So, this can be replaced with something like "added ArrayNodeDefinition::canBeEnabled() and ArrayNodeDefinition::canBeDisabled() to simplify optional configuration management"

@fabpot

fabpot Jan 28, 2013

Member

Actually, this is not a BC break as those methods have been introduced in 2.2 (see #5688). So, this can be replaced with something like "added ArrayNodeDefinition::canBeEnabled() and ArrayNodeDefinition::canBeDisabled() to simplify optional configuration management"

@fabpot

This comment has been minimized.

Show comment
Hide comment
@fabpot

fabpot Jan 28, 2013

Member

@vicb As explained in a comment, this is not a BC break as this feature does not exist in 2.1. So, I can make the change to the CHANGELOG if you want after merging, or I can let you make the change.

Member

fabpot commented Jan 28, 2013

@vicb As explained in a comment, this is not a BC break as this feature does not exist in 2.1. So, I can make the change to the CHANGELOG if you want after merging, or I can let you make the change.

@vicb

This comment has been minimized.

Show comment
Hide comment
@vicb

vicb Jan 28, 2013

Contributor

I am going to change it right now !

Contributor

vicb commented Jan 28, 2013

I am going to change it right now !

@vicb

This comment has been minimized.

Show comment
Hide comment
@vicb

vicb Jan 28, 2013

Contributor

(and thanks for having checked this)

Contributor

vicb commented Jan 28, 2013

(and thanks for having checked this)

[Config] Fix the changelog
The features has not been released yet -> not a BC break
@vicb

This comment has been minimized.

Show comment
Hide comment
@vicb

vicb Jan 28, 2013

Contributor

@fabpot I have updated the changelog and the PR header.

I am not sure if the commits should be squashed or not. On one side the multiple commits can help understand the changes but on the other side that's a lot of small commits which could pollute history. I let you choose what to do.

Contributor

vicb commented Jan 28, 2013

@fabpot I have updated the changelog and the PR header.

I am not sure if the commits should be squashed or not. On one side the multiple commits can help understand the changes but on the other side that's a lot of small commits which could pollute history. I let you choose what to do.

@fabpot fabpot closed this in c1037b1 Jan 28, 2013

fabpot added a commit to symfony/framework-bundle that referenced this pull request Nov 11, 2014

merged branch vicb/fmwk/config (PR #6852)
This PR was squashed before being merged into the master branch (closes #6852).

Commits
-------

fde7585 [DIC] Better handling of enableable configurations

Discussion
----------

[DIC] Better handling of enableable configurations

| Q             | A
| ------------- | ---
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no, this feature has not been released yet
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | -
| License       | MIT
| Doc PR        | -

My definition of bug fix might be discussable. The thing which I think is not discussable is that this PR fixes the semantic - and I think it is important for a "semantic configuration": before this PR, some nodes had `->canBeDisabled` for nodes that were actually disabled by default. Those nodes now have `->canBeEnabled` which sounds right.

**Edit: Jan 28, 2013** - history:

See [the related comments](symfony/symfony#6829 (comment)).

I think Symfony **must** get the configuration right as we can expect of lot of devs to use this as a template when writting their own configuration.

@schmittjoh could you please give me your feedback on [this change](https://github.com/symfony/symfony/pull/6852/files#L4R224) considering [the rationale](https://github.com/symfony/symfony/pull/6852/files#L3R7).

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

by stof at 2013-01-23T16:10:33Z

@vicb your links are broken as they are pointing to the PR creation page

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

by stof at 2013-01-23T16:10:55Z

and to create a TODO list, it has to be a list first

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

by vicb at 2013-01-23T16:31:10Z

@stof thanks for reporting the broken links, they are fixed /cc @schmittjoh

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

by vicb at 2013-01-23T16:31:50Z

@Tobion please submit a PR to my repo, I don't have much time to work on this. Thanks !

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

by vicb at 2013-01-25T15:14:47Z

@fabpot @schmittjoh I'd like your feedback on the latest commit, rationale is in the method phpDoc. It better matches what we do now and seem the most sensible thing to do.

edit: with this you can no more disable the node explicitly, I have to find a better solution

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

by schmittjoh at 2013-01-25T15:20:13Z

Looks good.

On Fri, Jan 25, 2013 at 4:15 PM, Victor Berchet <notifications@github.com>wrote:

> @fabpot <https://github.com/fabpot> @schmittjoh<https://github.com/schmittjoh>I'd like your feedback on the latest commit, rationale is in the method
> phpDoc. It better matches what we do now and seem the most sensible thing
> to do.
>
> —
> Reply to this email directly or view it on GitHub<symfony/symfony#6852 (comment)>.
>
>

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

by vicb at 2013-01-28T14:37:57Z

@fabpot I know I keep insisting on this one and I am sorry for that but I think this should be considered as a bug fix (see the PR header for details) and should be merged in 2.2. I think the Symfony core should be exemplary as it is used by many developers as a template when creating their own bundle. *This PR is no more a WIP and can be merged right now*.

In addition to fixing the enableable nodes, this PR contain new UTs and some fixes to the code / tests.

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

by fabpot at 2013-01-28T16:43:42Z

@vicb As explained in a comment, this is not a BC break as this feature does not exist in 2.1. So, I can make the change to the CHANGELOG if you want after merging, or I can let you make the change.

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

by vicb at 2013-01-28T16:46:33Z

I am going to change it right now !

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

by vicb at 2013-01-28T16:46:56Z

(and thanks for having checked this)

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

by vicb at 2013-01-28T16:54:37Z

@fabpot I have updated the changelog and the PR header.

I am not sure if the commits should be squashed or not. On one side the multiple commits can help understand the changes but on the other side that's a lot of small commits which could pollute history. I let you choose what to do.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment