Skip to content
This repository

Matching current MenuItem #122

Closed
phiamo opened this Issue June 20, 2012 · 23 comments

13 participants

phiamo Christophe Coevoet Tim Nagel Roman Marintšenko ARTACK WebLab TWS Oleg Stepura Nikolay Georgiev Bilal Amarni Peter Rehm shawn-northrop Waldo Arne Wieding
phiamo
phiamo commented June 20, 2012

Hi folks,
nice work!

I just updated to dev-master and dont get menu items matched anymore ...

is there any further change i have to implement, except for removing setCurrentURI calls?

cheers phil

Christophe Coevoet
Collaborator
stof commented June 20, 2012

@phiamo currently, the voters are not registered in the bundle itself (so only the current flag on the item is used to decide). You need to register a voter for the matching (thanks to the knp_menu.voter tag) if you want to apply a voter.
I will add some predefined voters soon, but I updated the bundle late yesterday evening to be able to merge the change in KnpMenu so I had no time to work on the voters themselves

phiamo
phiamo commented June 20, 2012

no prob, i will have a lot of time to wait :)

So you planned to add some default voters like the uri and the route voter?

i didnt yet get the clue how this is done, need to review the code,when having a bit more time ...

Tim Nagel
merk commented June 25, 2012

In an effort to ease other peoples issues here until a PR lands, here is the voter I've written to do matching:

<?php

// src/merk/Voter/RequestVoter.php

namespace merk\Voter;

use Knp\Menu\ItemInterface;
use Knp\Menu\Matcher\Voter\VoterInterface;
use Symfony\Component\DependencyInjection\ContainerInterface;

/**
 * Voter based on the uri
 */
class RequestVoter implements VoterInterface
{
    /**
     * @var \Symfony\Component\DependencyInjection\ContainerInterface
     */
    private $container;

    public function __construct(ContainerInterface $container)
    {
        $this->container = $container;
    }

    /**
     * Checks whether an item is current.
     *
     * If the voter is not able to determine a result,
     * it should return null to let other voters do the job.
     *
     * @param ItemInterface $item
     * @return boolean|null
     */
    public function matchItem(ItemInterface $item)
    {
        if ($item->getUri() === $this->container->get('request')->getRequestUri()) {
            return true;
        }

        return null;
    }
}

And its service definition in config.yml

    merk.voter.request:
        class: merk\Voter\RequestVoter
        arguments:
            - @service_container
        tags:
            - { name: knp_menu.voter }

I'm waiting on some feedback from @Stof if this is appropriate given the Matcher is not request scoped, and once blessed I will prepare a PR.

Roman Marintšenko
Collaborator
Inoryy commented July 02, 2012

@merk thanks a lot :o

ARTACK WebLab
scuben commented July 10, 2012

I follow this new matchers ans made the RouteVoter working for my SF 2.1 app. For that i extended the RouteVoter class with my implementation of RouteVoter:

<?php

namespace Artack\QSDNSBundle\Menu;

use Knp\Menu\Silex\Voter\RouteVoter as BaseRouteVoter;

class RouteVoter extends BaseRouteVoter
{

    public function __construct($container)
    {
        $this->setRequest($container->get('request'));
    }

}

next i made the service configuration:

services:

  artack.qsdns.menu.voter.request:
    class: Artack\QSDNSBundle\Menu\RouteVoter
    arguments:
      - @service_container
    tags:
      - { name: knp_menu.voter }

And my Builder looks like that:

<?php

namespace Artack\QSDNSBundle\Menu;

use Knp\Menu\FactoryInterface;
use Symfony\Component\DependencyInjection\ContainerAware;

class Builder extends ContainerAware
{
    public function mainMenu(FactoryInterface $factory, array $options)
    {
        $menu = $factory->createItem('root');

        $menu->addChild('dashboard', array(
            'label' => 'Dashboard',
            'route' => 'dashboard'
        ));

        $menu->addChild('dns', array(
            'label' => 'Manage DNS',
            'route' => 'dns'
        ));

        $menu->addChild('aboutqsdns', array(
            'label' => 'About QS DNS',
            'route' => 'aboutqsdns'
        ));

        foreach($menu as $key => $item)
        {
            $item->setExtra('routes', array(
                'routes' => $key
            ));
        }

        return $menu;
    }
}

I am pretty sure there are better implementation or there will follow an implementation from stof.

TWS
topweb commented July 12, 2012

I am using the code proposed by merk and it works for me.

Oleg Stepura

Hi!

before update I used this code in template:

    {% set currentMenuItem = knp_menu_get('MySiteBundle:Builder:mainMenu').currentItem %}

to get current item. Now I'm stuck, since all getters and setters for current item removed, new Iterators introduced, but seems like no documentation exists on how to use this new functionality.

To clarify I need to get current menu item to for example get it's label, some meta and build breadcrumbs on it.

Can you please help?

Thanks!

Nikolay Georgiev
zender commented July 17, 2012
    Here is my implementation of finding current item. If anyone finds out a better way please post it. 

    $matcher = $this->container->get('knp_menu.matcher');
    $voter = $this->container->get('knp_menu.voter');
    $voter->setUri($this->container->get('request')->getRequestUri());
    $matcher->addVoter($voter);

    $treeIterator = new \RecursiveIteratorIterator(
        new \Knp\Menu\Iterator\RecursiveItemIterator(
            new \ArrayIterator(array($menu))
        ),
        \RecursiveIteratorIterator::SELF_FIRST
    );

    $iterator = new \Knp\Menu\Iterator\CurrentItemFilterIterator($treeIterator, $matcher);


    $current = null;

    foreach ($iterator as $item) {
        $item->setCurrent(true);
        $current = $item;
        break;
    }
Bilal Amarni

I'm using the implementation suggested by @scuben (passing the container then setting the request manually), I think it's the best solution, there is no scope issue for not being sure to have the fresh response each time, because if you do a subrequest you'll still want to match against the real request, how would you do with the internal request?

Edit: maybe we could have this service configured in the knp bundle? instead of having to create it ourselves.

But if you don't access the service on the main request, you'll have the subrequest if you access it from a subrequest, not sure what would be the best implementation then...

Kenny Debrauwer Mopster referenced this issue in Kunstmaan/KunstmaanAdminBundle October 10, 2012
Closed

Use KnpMenu to build menu's #147

Christophe Coevoet stof closed this October 11, 2012
Christophe Coevoet
Collaborator

The RouteVoter is now enabled by default and uses against the master request (which is more sensible as it is the one coming from the client)

Peter Rehm

To use the code snippet from @zender there needs the knp_menu.voter.router to be used instead
of knp_menu.voter.

    $menu = $this->mainMenu($factory, $options);

    $matcher = $this->container->get('knp_menu.matcher');
    $voter = $this->container->get('knp_menu.voter.router');
    $matcher->addVoter($voter);

    $treeIterator = new \RecursiveIteratorIterator(
        new \Knp\Menu\Iterator\RecursiveItemIterator(
            new \ArrayIterator(array($menu))
        ),
        \RecursiveIteratorIterator::SELF_FIRST
    );

    $iterator = new \Knp\Menu\Iterator\CurrentItemFilterIterator($treeIterator, $matcher);


    $current = null;

    foreach ($iterator as $item) {
        $item->setCurrent(true);
        $current = $item;
        break;
    }

    return $current;
Peter Rehm peterrehm referenced this issue in KnpLabs/KnpMenu December 08, 2012
Closed

Removing the getCurrentItem method #32

shawn-northrop

Have any solutions been merged into the master branch for this bundle?

I have run ./bin/vendors update and .current is still not being applied to the menu item

Christophe Coevoet
Collaborator

the RouterVoter is registered by default

Peter Rehm

@stof is there any way to register invisible items to the menu tree for edit/delete actions which have a route which is based upon the object like /1/show/ or so? I want to see them in the breadcrumb but not in the default menu. What would be your suggestion?

Christophe Coevoet
Collaborator

@peterrehm items have a display flag

Peter Rehm

@stof I dont think this actually helps me. I have the following route:

article_edit                        ANY      /article/{id}/edit

So when addidng the child to the menu tree it looks like that:

    $menu['Admin']['Article']->addChild('ArticleEdit', array('route' => 'article_edit', 'routeParameters' => array('id' => 1)));
    $menu['Admin']['Article']['ArticleEdit']->setDisplay(false);

The issue is that I am just using this for the breadcrumb functionality. So the id needs to be flexible as well, building the menu as above is useless.

How could I try to still recognize such items from the menu tree with a routeParameter?

Since you have removed the currentItem functionality I now have a function for building the menu which returns the menu and an additional breadcrumMenu function which is taking the output from mainMenu and parsing it as follows:

public function mainBreadcrumb(FactoryInterface $factory, array $options)
{

    //TODO: Match the current according to parts of the route if concrete menus are being shown
    $menu = $this->mainMenu($factory, $options);

    $matcher = $this->container->get('knp_menu.matcher');
    $voter = $this->container->get('knp_menu.voter.router');
    $matcher->addVoter($voter);

    $treeIterator = new \RecursiveIteratorIterator(
        new \Knp\Menu\Iterator\RecursiveItemIterator(
            new \ArrayIterator(array($menu))
        ),
        \RecursiveIteratorIterator::SELF_FIRST
    );

    $iterator = new \Knp\Menu\Iterator\CurrentItemFilterIterator($treeIterator, $matcher);

    // Set Current as an empty Item in order to avoid exceptions on knp_menu_get
    $current = new \Knp\Menu\MenuItem('', $factory);

    foreach ($iterator as $item) {
        $item->setCurrent(true);
        $current = $item;
        break;
    }

    return $current;

}

The disadvantage I am seeing is that the menu has to be rendered twice, with is a performance disadvantage which is negotiable because I am just having a small menu tree. What is your recommendation?

Thank you in advance

Peter Rehm

@stof Can you give me any recommendations on that? By the way, what would you think about an entry in the docs about that? I guess breadcrumb is an important topic. I could help out with this if you are interested.

Waldo

Hi @peterrehm

I've the same issue like as your.

For fix that problem I have had a new condition in the @merk Voter

class RequestVoter implements VoterInterface {
//...
public function matchItem(ItemInterface $item)  {
        /* @var $request \Symfony\Component\HttpFoundation\Request */
        $request = $this->container->get('request');

        if ($item->getUri() === $request->getRequestUri()) {
            return true;
        }
        if ($item->getExtra('routes') !== null && in_array($request->attributes->get('_route'), $item->getExtra('routes'))) {
            return true;
        }
        return null;
    }

In the condition I search if the current route exist in the item.

In my menu builder I use this type of code

class Builder extends ContainerAware {
//...
 public function menu(FactoryInterface $factory, array $options) {

        $menu = $factory->createItem('menu');

            $menu->addChild('Menu Level 1', array('route' => '_an_amazing_route'));
            $menu['Menu Level 1']->addChild('Menu Level 2.1', array('route' => '_route_for_edit_something', 'routeParameters' => array('something' => null)))
                    ->setDisplay(false);
            $menu['Menu Level 1']->addChild('Menu Level 2.2', array('route' => '_route_for_add_something'))
                    ->setDisplay(false);

        return $menu;
    }
}

I hope that will help you.

Peter Rehm

@waldo2188 I just found time to look at it and found a solution based on yours.
However I have created a custom voter with a slight modification of the original RouteVoter.

/**
 * Voter based on the route with optional parameters
 */
class RouteVoter implements VoterInterface
{
    /**
     * @var Request
     */
    private $request;

    public function setRequest(Request $request)
    {
        $this->request = $request;
    }

    public function matchItem(ItemInterface $item)
    {

        if (null === $this->request) {
            return null;
        }

        $route = $this->request->attributes->get('_route');
        if (null === $route) {
            return null;
        }

        $routes = (array) $item->getExtra('routes', array());
        $parameters = (array) $item->getExtra('routesParameters', array());
        foreach ($routes as $testedRoute) {
            if ($route !== $testedRoute) {
                continue;
            }

            if (isset($parameters[$route])) {
                foreach ($parameters[$route] as $name => $value) {
                    if ($this->request->attributes->get($name) != $value) {
                        /* if value is set to 0 in the builder it is the wildcard for any parameter */
                        if($value !== 0) {
                            return null;
                        }
                    }
                }
            }

            return true;
        }

        return null;
    }
}

The only disadvantage is that I cant go with null as parameter, I had to use 0 instead to avoid the error

        $menu['Tools']
            ->addChild(
                'ItemShow', 
                array('route' => 'item_show', 'routeParameters' => array('id' => 0))
            )->setDisplay(false);

If I use null as you do, I am getting the error:

An exception has been thrown during the rendering of a template ("Parameter "id" for route "item_show" must match "[^/]++" ("" given) to generate a corresponding URL.")

@stof I really think somehow such behaviour should get into the KnpMenu Core. A loto of people seem to need that. I would be willing to work on this and create the docs If you want to support and give me guidance.

Christophe Coevoet
Collaborator
stof commented April 09, 2013

@peterrehm If you want to allow any parameter, there is no need to set it to 0. Simply omit it from the array

Peter Rehm

@stof Is is a required parameter in the route. I am talking about the typical usecase of creating a breadcrumb tree. You always have routes to edit objects. like /article/{id}/show where I want to show the breadcrumb navigation based on any {id}. so that I still see > Article > EditArticle.

If I omit the parameter get the following exception:

An exception has been thrown during the rendering of a template ("Some mandatory parameters are missing ("id") to generate a URL for route "article_show".") in "ArticleBundle:Article:show.html.twig".

I think this is a generic requirement. To make this happen I need to have a way of setting a route with a dynamic parameter like

    $menu['Tools']
        ->addChild(
            'ItemShow', 
            array('route' => 'item_show', 'routeParameters' => array('id' => null))
        )->setDisplay(false);

And I need to use the above in my comment #122 (comment) mentioned breadCrumb function with a voter which allows the dynamic parameter.

What do you think about finding a generic solution to make the currentItem accessible and to support such dynamic routes as well as provide the documentation for this? I think this would be very helpful for a lot of users.

Arne Wieding

Just for reference as i just spend a couple of hours on breadcrumbs and had the same problems as @peterrehm

I ended up taking a different approach which is described here:
http://obtao.com/blog/2012/11/create-breadcrumb-menu-with-knpmenubundle/

Obviously this doesnt work if you want to use the same Menu Class for your Navigation and breadcrumbs, but in my case it wasnt required. If youd want that, i guess you could still build some kind of mixture of both approaches.

Waldo

Hi,
I had some problems for make a breadcrumb with KnpMenuBundle.
I made a Bundle for that. You can check this bundle on github : https://github.com/AgrosupDijon-Eduter/BreadcrumbBundle
or here : http://www.mon-beulogue.com/en/2013/10/11/knpmenubundle-easy-way-breadcrumb/

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.