symfony-cmf / menu-bundle

Extends the KnpMenuBundle to work with PHPCR ODM
https://cmf.symfony.com
32 stars 48 forks source link

Cannot have menus outside of menuRoot #134

Closed benglass closed 11 years ago

benglass commented 11 years ago

I am having problems with the admin interface for Menu's which is caused by the fact that you have to specify a menu_basepath and the current MenuNodeAdmin enforces that all MenuNodes are within this base path.

For example you may want to create menu nodes that live inside your regular document structure (as a child of a page called About in /cms/simple/about). I may want to add a menu node here as a child which is a link to an external page. If I do this and I have the menu basepath set to /cms/menu then MenuNodeAdmin throw an exception when trying to edit that menu node because the basepath cant be found in its path.

I tried to solve this issue by setting the menu_basepath to /cms and now I am running another issue where the edit form doesnt work because the getMenuIdForNodeId method gets the wrong menu. I had created a separate menu for the footer in /cms/menu/footer (footer is the Menu). I added a child and it worked as expected but when I went to edit the child (/cms/menu/footer/foo) the function getMenuIdForNodeId returns /cms/menu instead of /cms/menu/footer. Obviously this would resolved if the menu_basepath was /cms/menu but then I run into the issue I mentioned above.

It seems the issue could be resolved by not requiring a menu to be in the menuRoot in the aforementioned method. Alternately the method could be modified to find the nearest Menu ancestor of the MenuNode.

It would also be nice if you could specify the menu basepath which could be used as a default for where menus are placed when creating new menus but should not prevent you from placing menu nodes in other valid places. The system would need to be able to handle MenuNodes for which a MenuId does not exist because they are not part of a menu.

The issue is primarily in the MenuNodeAdmin->getMenuIdForNodeId method

benglass commented 11 years ago

The code i am mentioning actually seems to be used just to build the breadcrumbs so a potential solution would be to short circuit the method if no menu node was found. This would be line 32 of MenuNodeAdmin->buildBreadcrumbs

if (!$menuAdmin) {
    return $menuNodeNode;
}

This is just a workaround for the fact that I can't set the menu_basepath to /cms/menu without triggering an error whenever I try to edit a MenuNode that is outside the basepath (a child of /cms/simple/about for example).

dbu commented 11 years ago

this indeed is a problem. walking up the parent tree instead of directly accessing the root node is a slight performance issue - although one that we should not be that expensive.

@dantleech wdyt would be the best solution for this issue? i think not showing the breadcrumb when editing a menu node that is not under a menu sounds correct to me. but there is still the issue that if you edit a menu that is not in the configured menu root, editing it in the admin would be a problem as the tree is set to the root path. we could look at the path in the admin and set parent to / if the document is not in the path...

dantleech commented 11 years ago

I think the breadcrumb thing is a bit of a hack anyway - I only did it to add some UI enhancement to the admin, but that seems to have backfired here. I think a more generic way of representing the documents position in the document hierarchy would be good for 1.1.

Defaulting to "/" if the menu item lies outside of the menu path sounds OK, although I am not 100% sure if there isn't a better solution to this.

benglass commented 11 years ago

Regarding the basepath configuration (for menu, content, and root basepath) right now the configuration is split up across a few different bundles and additionally it feels like it forces a certain structure on the content repo. We are looking at using CMF for a multi website cms instance and we will need a more fluid concept of base paths (basically it will change depending on which website you are currently editing). I don't know how common this use case is but even with a single site cms currently the basepath configuration has been something I have been fighting with a bit.

For example on a single site cms install I'd like to put my content in /cms or /cms/main and have the tree browser only display that content in the sidebar.

I'd also like to be able to create arbitrary menus mounted at /cms/menu and i dont want it to appear in the default sidebar tree because it is somewhat confusing to end users and id rather have that be managed through the Menu admin area. Im not sure if there an easy way to do this right now that wont effect the content tree everywhere it is displayed (such as selecting parents). It would be nice to be able to provide a service through configuration that providers the base path for menu/content/cms, etc. (sort of like a knp menu provider). Then I could configure my service to understand multi website contexts or just to know when to display the menu node and when not to. This seems like it might be a more flexible architecture but it could be a significant change.

For this particular bug we are dealing with here it sounds like the proposed solution is...

Climb up the tree looking for the nearest ancestor node that is not a MenuNode (it would be fine if its a Menu or a Page or any other class that sonata admin knows about).

So beginning on line 47 of the MenuNodeAdmin we

  1. Try to find the nearest non MenuNode parent
  2. If we CANNOT get an Admin class for it from the pool, return just the MenuNode
  3. Otherwise, add an edit link (based on permissions as it is currently doing)

If this seems correct I'll take a crack at implementing in a PR.

I'd be interested to hear thoughts on my other proposal regarding a more fluid implementation of the basepath concept. This could possibly be achieved in my own install by overriding certain services to use a this BasepathProvider instead of a hardcoded basepath but I am curious if its something you feel maybe we worth while for the cmf in general.

dbu commented 11 years ago

i am +1 on your proposal how to fix the immediate issue.

regarding multisite: we definitely want to make this possible, it is one of the use cases where the cmf should be a great tool and better than out-of-the-box cms systems that tend to be unflexible in that regard. @fazy was working on having setters for the document manager that services use so that this piece can be changed by listeners. maybe this could be an approach for base paths as well (a listener that changes root paths / base paths according to the domain. if we provide that in a cmf bundle, it should be possible to figure out which services might need to be updated, we can't expect somebody building a multi-cms to figure that out by himself). but lets take this discussion to the symfony-cmf-devs mailinglist. at this point we really must go out with 1.0, and this will be a 1.1 only feature. but it does not hurt to start discussing it now. (according to our schedule, 1.1 should come out by the end of the year or january 2014). @benglass do you want to take the lead on this? a first step could be to update the wiki page with ideas and a concept draft that we update according to discussions on the cmf mailinglist: https://github.com/symfony-cmf/symfony-cmf/wiki/Multi-Domain-Support

benglass commented 11 years ago

Ok I have requested to join the symfony cmf devs mailing list and that is pending approval at which point I will compile my thoughts on an approach for the multi site/domain routing feature.

I will work on the patch PR in the meantime.

benglass commented 11 years ago

This is resolved