Improving the way the current item is determined

[stof]

Currently, the library still uses the behavior coming from KnpMenuBundle:

• an item can be set explicitly as being (or not being) current.
• if the flag is not set, the uri is checked against the current uri set for the item (and if there is no current uri, it is not a current item)

There is some improvements needed here:

• there can be multiple current items in a tree so the getCurrentItem method is lying by giving only the first found (should it be removed entirely ?)
• this part of the library should be more flexible to support other behaviors (for instance matching several uris on the same item)

[stof]

An idea I have is to have a matcher responsible to determine whether an item is current or not. This would allow replacing the implementation with a more complex one.
Adding a way to store extra informations in the item should be implemented too to allow the matcher to use them (for instance, a matcher could look at alternative uris and check them too).

The question here is: should the matcher be injected into the item (allowing to keep the isCurrent() method and the boolean flag bypassing the matching) or should it be kept outside of it and passed to the renderer to determine the current item ?

Comments are welcome

[benjamindulau]

I think each solution has some pros and cons.

I think it's more consistent if the matcher is injected into the item, since it changes the item state (current state).

But, injecting the matcher into the renderer is more performance friendly. We could suppose that the menu is rendered in each page anyway, so injecting the matcher into the item is not really a problem.

I think i'd go for injecting the matcher into the item.

Now, i don't know why, but injecting a matcher into an item seems a bit weird to me. Does another object on the top of the items should not be responsible of that ?

[stof]

yeah, injecting the matcher seems weird to me (which is why I'm trying to improve the idea to get rid of this need).
It is exactly the reason why I removed the renderer from the item: the renderer is responsible to render the item whereas before the item was responsible to create a renderer responsible to render the item... And also why the instantiation of the item has been moved to the factory (which allows having a different constructor btw)

Regarding your concern about the state, one point is here: should we keep the determination of the current item as a state of the item or move it out of the item (to the matcher).

[benjamindulau]

I think we need the "current" flag into the item, but if a matcher is added somewhere in the process, the item should definitly not be responsible of using it nor setting its own current state.

IMHO, the matcher should not keep any instance of the current items. It should just find the current items in the tree and set the current state onto them.

But injecting the matcher into the renderer seems weird too, and having the matching process executed when the menu is built doesn't seem right either since it would add an useless overload.

Don't know where is the good solution there :/
Maybe the matcher should be injected into the provider, don't know.

[benjamindulau]

I wonder if the current state would be ever useful for anything else than the rendering. If not so, using the matcher during the rendering process seems relevant after all.

[rubensayshi]

I'd strongly prefer having it in the item since being able to check which items are active (isCurrent) can be very handy.
Putting the matcher into the renderer will result in a lot of solutions developers come up with being (near) imposible.

Unfortunatly the project is private but I have a project with sf2 and the KnpMenuBundle where I actually use isCurrent to set showChildren and use that to handle displaying a submenu in a different menu.

Just a small example, but the current structure (which remains in tact if you place the matcher within the item) gives a developer such freedom, while moving it to the renderer wouldn't.

Apart from that I think that creating a seperated matcher would be very nice and would be a better solution then my PR on the KnpMenuBundle which simply puts a regexp in the item.

[stof]

well, the matcher would not be a private object inside the renderer. You could still use $matcher->match($item) (or whatever the interface looks like) in another part of the code.

[stof]

Btw, what do you think about the other issue pointed in the original post: multiple current item. Should we allow only one current item in the whole tree (thus making it meaningful to retrieve the current item) or should we keep allowing multiple current items (and thus deleting the method to get the current one as it is lying by giving only the first found) ? This decision will impact the way the matcher is implemented

[benjamindulau]

@stof: I think the multiple current item is a common case, especially for submenus (when an item from a submenu is current, i want all its parents to be current too).

[stof]

the parents of a current item as considered a currentAncestor currently, and receive the current_ancestor class in the rendering.

[rubensayshi]

you should allow multiple current items, again to avoid disallowing that option.
and it wouldn't be the first time I've seen multiple current items (I think the example ben means is having the top level item actually linking to the first submenu item eg)

[stof]

Then we have to remove the getCurrentItem method as it does not make sense if there is multiple ones in the tree. I'm fine with removing it as the library itself does not use it anywhere.

Note that getting all the current item can be done by using the power of PHP iterators.

[phiamo]

i would also like to see a solution here.

i have a action which shows a paginator, and if i go to next page my menu is missing
@route("/show/{page}", name="show_something")

the paginator is also stuck (Knp Paginator Bundle) showing the correct page...
this really doesnt look nice, if i take out the {page} from the route at least the paginator works ...

any ideas ?

cheers phil

PS: i love you bundles anyways!

[phiamo]

sorry got to revorke...
solution to get this was easy:

    public function createMainMenu(Request $request)
    {
....
        $menu->addChild('something pagable', array('route' => 'show_pagable', 'routeParameters'=>array('page'=>$request->get('page', 1)))); 

}

and for zend paginator used in knppaginatorbundle

    /**
     * @Route("/show_pages/{page}", name="some_thing_paged")
     * @Template()
     */
    public function pagedListAction($page = 1)
    {
        $paginator->setCurrentPageNumber($this->get('request')->query->get('page', $page));
}

[zhil]

Right now I have just added such a code to the menu builder

switch($this->container->get('request')->attributes->get('_route')) {
            case "videoView":
            case "videoList":
                $menu['mainmenuVideo']->setCurrent(true);
                break;
            case "mediaView":
            case "mediaList":
                $menu['mainmenuMedia']->setCurrent(true);
                break;
            case "newsView":
            case "newsList":
                $menu['mainmenuNews']->setCurrent(true);
                break;
        }

Its simple and can implement any extensive logic.

But I think, that the best solution - add a couple options for determining current page. So developer will have choice - how to detect current page.
Option #1 - some kind of variable attachedRoutes would make everything a lot more clean. something like

$menu->addChild('mainmenuVideo',array('route'=>'videoIndex'),'attachedRoutes'=>array('videoView','videoRelated','videoTagsSearch'));

So whenever some of the routes listed in attachedRoutes used, menu item will be selected.

Option #2. Sometimes more complex logic may be required. For example, I have News entity in my site. News can belong to 1 of 3 groups - Public Release, Review, Article.
All news pages is almost the same - so I use single routes group for them (like /news/view/{slug}/{id}, /news/related/{slug}/{id}, etc)
Top menu have items Public Release, Review, Article and they need to be selected based on type of the entity.
"Switch-style" code cant be used for this purpose, because menu builder cant access related entity on page.
Looks like some DI trick required in such a situation.

[jonathaningram]

@zhil, can I just make a note that your switch solution does not work when using ESI because the value of _route will be _internal.

[stof]

yeah, for ESI request, you probably need to propagate the main attribute through a GET attribute and use it instead.

[zhil]

thanks for the note and possible solution. already do the same :)

[pluff]

It seems like this discussion is a bit in "sleep" mode? Did you came to any conclusion? This issue is very common and it will be really nice to see it solved.
As for me, I'd like to see matcher injected to item since it's very useful to know current menu items before rendering. And I believe this issue is pretty critical, since some parts of menu can be not created at all. Matcher in renderer will disallow individual matchers for items, correct me if I wrong, please. Multiple current items will not be a big deal with "matcher in menu" implementation.

[PayteR]

I have solved customisation of current item in this way:

kmpmenu i have it as a service, i need router in builder so i need to send it into service as argument

arguments: ["@request", "@router"]

in MenuBuilder i will need to add:

use Symfony\Component\Routing\Router;

change:

public function createMainMenu(Request $request, Router $router )
    {
        $menu = $this->factory->createItem('root');
        $menu->setCurrentUri($router->generate($request->get('_route')));

so now will be current item set correctly even when in url will be some junk ... and when i want to set some other item as current i can do it in controller very simply:

$menu = $this->get(mybundle.menu.main');
$menu->setCurrentUri($this->generateUrl('my_route'));

maybe this can help someone...

[c33s]

any official plans for that? it is really a common requirement. having a simple crud for, let me say articles, with a single main menu.
accessing /articles/* should highlight the main menu item "articles" no matter how long or how deep the link is.

this feature is really important.

[stof]

@c33s This issue is still in my mind. But I haven't had a flash showing me how to solve it in a good way. The idea I have in mind is not a working solution currently. And I unfortunately don't have enough time to work on all stuff I would want to and other stuff jumped higher in my todo-list

[c33s]

what about a temp solution? i don't have to be perfect, also if it may not be the final solution, it is a better solution, than have this feature missing.

what about the solutions already posted:

• added activeMask property and using it to preg_match the currentUri KnpMenuBundle#49
• Improving the way the current item is determined #2 (comment)

@stof what do you think?

[stof]

@c33s the activeMask solution requires changing the ItemInterface. Modifying the interface for a temp solution is not acceptable (as interfaces should stay BC except for good reasons)

and the solution in the comment above is something done in the builder code, i.e. in userland so you can use it

[baikunz]

I think that i'm far from being an expert but what about creating a matcher service, as suggested by @stof, the current route would be injected inside it as would be the menu. If you don't want the MenuItemInterface to be modified, what about configuring the matcher service with a specific config file ? The idea is to associate a menu entry with multiple route name.
Then the matcher could be used inside any class as a service instead of having to deal with the switch trick.

Sorry for my poor english!

[stof]

@baikunz My issue is about the way the matcher service should be used. I see several solutions

1. a totally separate matcher, which would simply call setCurrent on the items and would be called at the end of the form building or something like that
2. injecting the matcher in the renderer to call it at the beginning of the rendering with the same design for the matcher (simply avoiding to require calling it by hand)
3. making the renderer use the matcher when wanting to know if an item is current instead of calling the getter of the item directly (harder because the Twig template will need a way to access the matcher but cleaner as the matcher does not modify the item, and so keep the explicit choice of the user)
4. injecting the matcher in the item to use it in the getter (the closest from the current way, but also the crappiest IMO)

[baikunz]

Oh ok I think i missed that point ! by the way IMHO and for what it worth I was thinking of something like the first solution, I remember I had to use something like that while dealing with Android and the user was responsible to call explicitly the matcher to match the Uri.

[c33s]

i think it is importent, that not every route have to be defined. often you only have a single level menu visible like:

home | events | news | ....

from the routing side its there are a lot of "subroutes"

event_new
event_view
event_edit
event_delete
events_list
event_gallery_view
event_gallery_...

so i can continue this list, i am sure you get the point.

i sould really be easy to simply define each event route should mark the 1st level event element as ancestor.

maybe it is easy to define a routename wildcut matcher (would be better than a url matcher).

using such a menu system should help the developer to reach his goal fast and easy. not having to create tons of services, enter every route which is in his project,...

RAD is a thing which is really important. i allways loved the input from @fzaninotto to @symfony, because he was the link between high end, high quality code and fast and easy usability for the end-developer.

[baikunz]

@c33s yeah you are right having to specify each routes seems too much work. A routename matcher could be a great alternative but it then become dependant of the routenames so what about routenames defined in third party bundles?

[stof]

@baikunz My idea is to have an item matcher, i.e. allowing to match on anything you want available in the item (and it is one of the reason I added the support for extras in the item a few weeks ago)

[baikunz]

@stof doesn't it mean that in some way the item itself is aware of how it have to be treated?

[stof]

@baikunz no, what it means is that the matcher receives the whole item. The extra array is simply a container for additional stuff. the item does not know anything about what they are. It only stores them.

[baikunz]

@stof Ok I got it, it seems quite good, but I still don't get how you could easily make the link between an item and a route/url with that solution. I see 2 ways of doing it

• The item should have an extra parameter with the associated routes: but it seems to much work if you have dozens of routes that will trigger then same item.
• The item should have a unique extra parameter that could be guessed from the routes/urls and we still have to find a way how to do that

[stof]

@baikunz $item->setExtra('routes', array('event_show', 'event_edit')) for instance. And for urls, you could use the same way, or simply use the url of the item (which is the only current solution)

[baikunz]

@stof yeah this is what I tought, but don't you think it's much work like said @c33s ? maybe could we define just a route prefix i.e. event_ that would match all event_* routes, or even route wildcards i.e. events?_.*

[stof]

@baikunz if your matcher expects this, it is possible too

[baikunz]

Ok, I'll try something when I have some time

[stof]

Well, I have a full week now before the start of my internship. So I plan working on it (and on some other bundles)

[baikunz]

Ok then i'll be glad to bring you some help if you need.

[stof]

Please look at #49