[DX] Allow exceptions to be thrown during PhpcrProvider::has

[dantleech]

If the provider cannot find the menu document then the error is thrown back to KnpMenu ChainProvider and the next provider is tried.

When all providers fail you get a rather upaque message:

The menu "pages" is not defined.

PhpcrMenuProvider::has needs to return false if the document does not exist, but currently it will return false if f.e. the document does exist but does not implement the NodeInterface.

It could be taken for granted that if the user has defined the menu base path to be /cms/menu then if a document exists at /cmf/menu/foo then they intended that foo was the menu root.

[dbu]

i guess the problem is that we should not throw the exception in has, as its meant to be just a check. another provider could provide the foo menu, maybe expecting a different class of document at the path, or maybe using something entirely different...

we could log a warning maybe? we could also provide a command to sanity check the repository: make sure there are routes under the route root, make sure there are menu nodes under the menu roots, maybe other similar things.

but i don't see how we could improve this exception message, short of making the chain provider aware of phpcr and check for the specific situation, which feels strange. or expand on the menu providers to provide debugging information, probably with an additional interface that we can check for, if no menu was found and ask providers for debugging hints...

[dantleech]

Hmm, thinking of it, in my own situation I have pages, routes and articles under the menu basepath /cms, of which only pages is a menu item.

Another idea would be to prefix the menu name with cmf/, e.g. {{ knp_menu_render('cmf/main') }} - this would have the added advantage that we could immediately discard requests without the prefix before calling PHPCR.

[dbu]

the prefix feels like leaking information to the application. the appeal of calling it just "main" is that i could switch the provider as needed without having to figure out where in the application i call this.

indeed the sanity check idea probably does not work. but logging a warning could still work - once you request something as a menu, it is indeed supposed to be a menu.

[dantleech]

On Mon, May 23, 2016 at 02:32:08AM -0700, David Buchmann wrote:

the prefix feels like leaking information to the application. the appeal
of calling it just "main" is that i could switch the provider as needed
without having to figure out where in the application i call this.

There is already some pollution here as the template is coupled to the state
of the storage (f.e. that /cmf/menu/foo exists). Which might be an
argument for having a single reference (so you can override a menu).
But, most of the time I think people explicitly want to render a CMF
menu, and they call that menu from one place (e.g. layout.html.twig).

It would make the intention explicit (which I like) and therefore enable
us to throw explicit errors. Also the performance improvement.

But maybe a different issue is required to discuss and it could be
implemented as an option.