review MenuBundle for 1.1 release

[lsmith77]

check bundle docs and config reference
also check open tickets and ensure the changelog is uptodate

[lsmith77]

ping @benglass .. we want to announce the stable 1.1 release very soon ..

[lsmith77]

symfony-cmf/menu-bundle@1.0.0...master

[benglass]

@lsmith77 any sense of timeline for the announcement? I can try to review it after work

[lsmith77]

hoping for tomorrow evening or wednesday

[benglass]

@lsmith77 @dbu Just finished reading through 1.1 MenuBundle changes and comparing it to docs. Overall I dont see much issues, I can submit a PR for the ones I mention below if you can confirm my understanding is correct (potential changes 1 under docs and 1 under menu bundle itself). I had a few questions that could lead to additional cmf docs changes.

Potential Docs Changes:

1. Update docs, get/setParent is now get/setParentDocument

   symfony-cmf/menu-bundle@1.0...1.1.0#diff-2ee78e6f52fe83a1b2e51cede685f897L81
   symfony-cmf/menu-bundle@1.0...1.1.0#diff-1302067c4101240caa6d989340da4078L180

   Looks like HierarchyInterface uses getParentDocument but docs might refer to old getParent on MenuNode?

   bundles/menu/introduction.rst

2. Does this require migrations/is it a bc break? If so should we document in docs?

   symfony-cmf/menu-bundle@1.0...1.1.0#diff-aec6df72959d1d118e163801c083f25aL16

3. Why is there a hierarchy as well as a childinterface? Should the docs explain? This doesnt seem like its menu bundle related per se more of a general cmf docs or core or phpcrodm docs

Potential MenuBundle 1.1 Changes:

1. Do we need to require 1.1 of core bundle in composer.json due to use of child interface?

   symfony-cmf/menu-bundle@1.0...1.1.0#diff-1302067c4101240caa6d989340da4078R17

[dbu]

Thanks!
"1." Yeah lets change to getParentObject (child interface is storage agnostic). And we should indeed have bumped the corebundle requirement to 1.1.

"3." The hierarchy interface is in phpcr-odm, allowing to have hierarchy without depending on corebundle (see routingbundle). The sonata extension supports both. We should mention that in the core bundle doc yes.

[dbu]

"2." we moved the field, as parent is not mandatory for a menu node and base should be minimal. i don't think the doc talks about this at all. the changelog however should mention it i think.

[benglass]

@dbu I am looking at the change int he docs for getParent/setParent and I am wondering about the MenuNodeBase

https://github.com/symfony-cmf/MenuBundle/blob/1.1.0/Model/MenuNodeBase.php#L205

This class doesnt inherit from anything and doesnt define a parent property but it is assigning to $this->parent. It has no getters for this property either. This property seems to be defined in the MenuNode class which inherits from MenuNodeBase https://github.com/symfony-cmf/MenuBundle/blob/1.1.0/Model/MenuNode.php#L44

The docs at https://github.com/symfony-cmf/symfony-cmf-docs/blob/master/bundles/menu/menu_documents.rst create a Doctrine\Phpcr\MenuNodeBase which extends Model/MenuNodeBase and then calls setParent on the node. Model/MenuNodeBase does not have a setParent function, only its child MenuNode does.

The example can be made to work just by using Doctrine\Phpcr\MenuNode instead of MenuNodeBase but I am wondering if potentially this represents an issue with the hierarchy?

Here is a doc PR to fix the get/setparent stuff #491

[benglass]

To fix cmf core version symfony-cmf/menu-bundle#189

[dbu]

created symfony-cmf/menu-bundle#190 about the code review. i guess we need to release a 1.1.1 version asap :-(

[dbu]

i think one more missing bit in the doc is the Menu document that serves as root document for the menu.