Fix configuration settings order

[kea]

This patch will fix the order of default value, if values are defined in config.yml will override the service defaults.

Possible related to sonata-project/SonataBlockBundle#149

[soullivaneuh]

I don't really see the interest of using closure, the options resolver is already here to determine default, that can be override.

I can't see the concrete difference, could you explain a little bit?

[kea]

@soullivaneuh you have already (elegantly) fix this issue in branch 2.3 of SonataBlockBundle cleaning the option resolver mess but the solution is not backported for the 2.2 branch.
Unfortunately the stable version of Cmf-stuff (1.2) depends on 2.2 version.
If you want to try, install the cmf-sandbox (1.2) and go to the backend, the tree is rooter to "/" instead of "/cms" as wrote in config.yml

sonata_block:
    blocks:
        sonata_admin_doctrine_phpcr.tree_block:
            settings:
                id: '/cms'

With this (dirty) PR the root is fixed to "/cms". The closure postpone the set of the value and let the option resolver check if other default values are already set (ie by config).

[soullivaneuh]

you have already (elegantly) fix this issue in branch 2.3 of SonataBlockBundle

I don't really understand the relation between block bundle and this PR. Could you give me a link of what you are talking about?

[dbu]

i don't exactly understand how the closures are able to fix the problem. out of interest, can you please elaborate what happens? best in a doc comment right before that code, also noting that we don't need the closures anymore when going to version 2.3 of sonata block? the master branch of this bundle is based on sonata 2.3 and will become phpcr-odm admin 2.0...

[kea]

https://github.com/sonata-project/SonataBlockBundle/blob/6dc347de4a74502c5528b16505cfd3ffdeabd87c/Block/BlockContextManager.php#L102

// SonataBlockBundle::Block/BlockContextManager.php
102  $this->setDefaultSettings($optionsResolver, $block);

This code set the default (config values $settingsByType and $settingsByClass) in options resolver.

104  $service = $this->blockService->get($block);
105  $service->setDefaultSettings($optionsResolver, $block);

Here is called the setDefaultSettings of the service, the method I modified in this PR. The actual code will overwrite the defaults property values set on $optionResolver. The closure is set on lazy array property of OptionResolver and leave untouched the previous defaults.

https://github.com/symfony/symfony/blob/2.8/src/Symfony/Component/OptionsResolver/OptionsResolver.php#L159

@dbu I didn't really check the new version of sonata block (2.3), I only read the code and those lines of code still there
https://github.com/sonata-project/SonataBlockBundle/blob/master/Block/BlockContextManager.php#L230

[dbu]

ok, thanks. i guess then we want to merge this.

@soullivaneuh what do you think? is there some misunderstanding in how things are supposed to work? or is this a bug of the block bundle?

[soullivaneuh]

@dbu @kea I don't see yet what this block service should have closure options instead of another block services.

And I don't see where I "already (elegantly) fix" this. Could you provide my related PR link?

To be more simple, could you provide a concrete case?

Or I missing something.

ping @rande

[dbu]

@kea @soullivaneuh so what do we do? i think i more or less understand the explanation and tend to merge this. but we should have a clear path of action what needs to be fixed, unless we consider this the best solution for the problem.

[soullivaneuh]

For my part, I'm still waiting a concrete case.

I would not merge it for now as I don't get the interest of Closure usage here.

[dbu]

@kea can you clarify some more so that we can merge this?

[kea]

@soullivaneuh @dbu
Given

class TreeBlockService extends BaseBlockService
{
    public function setDefaultSettings(OptionsResolverInterface $resolver)
    {
        $resolver->setDefaults(array(
            'id' => '/',
        ));
    }
}

and given in config.yml

sonata_block:
    blocks:
        sonata_admin_doctrine_phpcr.tree_block:
            settings:
                id: '/cms/content'

At the end of BlockContextManager::get($treeBlock, array())
https://github.com/sonata-project/SonataBlockBundle/blob/master/Block/BlockContextManager.php#L130
if I print $blockContext->getSetting('id') which should be the correct value?
"/" or "/cms/content"?

[dbu]

in my opinion, the config.yml should win. do you agree @soullivaneuh ?

[soullivaneuh]

I agree with @dbu, because if you want the default value, then you should write this I think:

sonata_block:
    blocks:
        sonata_admin_doctrine_phpcr.tree_block: ~

And then you will get /.

[kea]

Perfect! The actual code returns / (the service default value), the code in my PR returns /cms/content (the config.yml value).
I would like to write a test for that but I've never wrote a test in a bundle involving config.yml. Could you help me, please?

[soullivaneuh]

@kea Ok, but I'm not sure for closure. Can't understand the process.

For me, it sounds like a workaround. Is not the bug somewhere else?

Ping @rande.

[rande]

if default from config.yml file are not used, then this method should be fixed: https://github.com/sonata-project/SonataBlockBundle/blob/master/Block/BlockContextManager.php#L148-L182

[kea]

@soullivaneuh would you like to merge this workaround for the 1.2 branch? Since the SonataBlockBundle bugfix is likely to be merged in the 2.4 branch and it doesn't effect the branch 1.2 of SonataDoctricePhpcrAdminBundle.

[soullivaneuh]

I don't like to merge workarounds like this. BTW, as I said in sonata-project/SonataBlockBundle#222 this is a fix but this give BC breaks.

I would prefer wait block-bundle PR.

Let @rande decide.

[soullivaneuh]

At least, add a comment to remember that this workaround need to be removed later and why.

[rande]

@soullivaneuh the CMF team is in charge of this bundle. So let @dbu or others main contributors decide.

[dbu]

added a comment about this with fed5a47