Required Menu Bundle

[sebastianblum]

I'm playing with symfony-cmf and I'm only using the BlockBundle (without the MenuBundle).

I got this exception

[Doctrine\Common\Proxy\Exception\UnexpectedValueException]                                                                                    
  The type hint of parameter "menuNode" in method "setMenuNode" in class "Symfony\Cmf\Bundle\BlockBundle\Doctrine\Phpcr\MenuBlock" is invalid. 

https://github.com/symfony-cmf/BlockBundle/blob/master/Doctrine/Phpcr/MenuBlock.php
The MenuBlock has a dependency to Knp\Menu\NodeInterface

So I would change the require-dev dependency to require to solve the issue.

The other possibility is to require the sonata-menu-bundle instead of menu-bundle.

[lsmith77]

we generally only put "require" on things that are required to make any use of the given Bundle. however the MenuBlock is optional, which is why its only a "require-dev" .. but it should be added to the "suggest" as well.

/cc @dbu

[sebastianblum]

@lsmith77 I agree, that the bundle shouldn't be required.

at in fact, the code in the MenuBlock requires Knp\Menu\NodeInterface
see https://github.com/symfony-cmf/BlockBundle/blob/master/Doctrine/Phpcr/MenuBlock.php

I don't know how we can remove the dependency, so my suggest was to require the menu-bundle

[lsmith77]

well at this point, we have not done into the direction of offering bridges like Symfony core does at it complicates testing, release management etc. so we essentially have these options:

1. move the code to the MenuBundle (but then we would have the same issue there)

2. make it a required dependency (however there is a lot of value in the Bundle even without the MenuBundle, so a require seems wrong)

3. create a bridge repo (which complicates testing, releases etc)

4. make the MenuBundle a suggest

5. is what we so far have done in the CMF project and have simply failed to do properly in this case.

[sebastianblum]

@lsmith77 What do you think about this interface
https://github.com/KnpLabs/KnpMenu/blob/master/src/Knp/Menu/NodeInterface.php

possible solutions:

a, we can duplicate the interface in the BlockBundle

b, check the instance of the argument in the method

public function setMenuNode($menuNode = null)
{
    if (null !== $menuNode && $menuNode instanceof NodeInterface) {
        $this->menuNode = $menuNode;
    }
    return $this;
}

If you decide which solution you prefer, I will update the pull request.

greetings, Sebastian

[lsmith77]

hmm changing the method signature like this a a BC break.
also I am not sure how that changes things .. it basically just silences the issue which can be quite confusing. how are you accidentally setting an instance of a different class there?

[dbu]

as long as its possible to ignore that there is a class that has an unknown dependency, we can just ignore this.

but is the problem that the dependency is not really weak, as generating the doctrine proxies reveils that we miss the menu bundle? is there a possibility to make doctrine ignore that class when menu bundle code is not available? like hiding the mapping or something?

[lsmith77]

ah .. now I understand the issue. hiding the mapping is probably only possible via some class meta data laod listener but that seems quite hacky.

[dbu]

well, if that is possible, i would chose that over a separate bundle for just one document. and removing it again and just having a cookbook article how to do it yourself seems awkward...

but i am not sure if that even would work or if doctrine then still would try to load the class to check for annotations. as its not in the default folder, maybe not.

@sebastianblum can you check what happens if you remove the xml mapping file https://github.com/symfony-cmf/BlockBundle/blob/master/Resources/config/doctrine-phpcr/MenuBlock.phpcr.xml ? if that solves the problem, i will look into the metadata listener and see if this is where we can prevent the problem from showing up.

[sebastianblum]

Hello @dbu,

the weird thing is, that I only get the error message in prod-environment

app/console cache:clear --env=prod

@ddu yes it works, if I delete https://github.com/symfony-cmf/BlockBundle/blob/master/Resources/config/doctrine-phpcr/MenuBlock.phpcr.xml

On word to the backward compatibility @lsmith77 says above.

changing a method from

public function setMenuNode(NodeInterface $menuNode = null)
{
    $this->menuNode = $menuNode;

to

public function setMenuNode($menuNode = null)
{
    if ($menuNode instanceof NodeInterface) {
        $this->menuNode = $menuNode;

is backyard compatible because because it works like expected.

@dbu you suggestion is too complicated in my opinion.

I know that I'm a special case, that I want to use the BlockBundle without the MenuBundle, but I would like to remove the hardcodes dependency.

[dbu]

its to be expected that the problem only comes in prod environment. in dev, doctrine proxies are created on the fly - in prod they are pre-generated during warmup. good to know that without metadata it would work. i will see what i can do.

your use case is one that we really want to encourage. that is one of the goals of the cmf, to be modular.

the only place where this change is not BC is when somebody extended the block class and overwrote that method. his code will then become invalid. i think we should do the change you propose if that line is all that is needed - but only for the 1.3 version to not have a potential BC break in a dot upgrade. until then you would need to use 1.3.*@dev as version in your project.

@lsmith77 wdyt, stupid idea or good enough? i am not even sure if the doctrine metadata thing would allow to completely un-map a document.

[sebastianblum]

@ddu but please remember, that

/**
 * @var NodeInterface
 */
private $menuNode;

the menuNode is only private, so it is really hard to overwrite this method.

@dbu you can decide which solution you want, I will create a new pull request then.

thank you very much for your support David

[dbu]

well, the method could be overwritten to do something and then call its parent. but i think its not very likely.

looking forward for the pull request.

[sebastianblum]

@dbu is my solution good enough or do you have any suggestions for me?

[dbu]

i think we should be a bit more strict. something like

if (! (null === $menuNode || $menuNode instanceof NodeInterface)) {
    throw new \InvalidArgumentException('$menuNode must be an instane of NodeInterface');
}

$this-> menuNode = $menuNode;

[dbu]

sorry, seems i completely missed that. merged now.

[lsmith77]

should we tag 1.2.2?

[dbu]

done

[sebastianblum]

thank you very much @dbu