Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Set current uri #55

Merged
merged 4 commits into from

5 participants

@uwej711

This PR extends #53 to automatically set the current uri from the request in MenuHelper.

benjamindulau and others added some commits
@benjamindulau benjamindulau When rendering the menu through a twig template, the depth option.
Also, the depth default value is now 1 instead of null, which is IMO the most common use case.
fc15b09
@benjamindulau benjamindulau * Removed unused "path" option in MenuExtension and MenuHelper
* Default of the depth option is back to "null"
499b7a3
@uwej711 uwej711 Merge remote-tracking branch 'benjamindulau/FixingDepth' into set_cur…
…rent_uri
e722d2d
@uwej711 uwej711 Automatically set the current request uri to the menu
To make adding the class current to the current menu item work, there
needs to be a way to inform the menu of the current request uri. This
can be done, when the menu is fetched from the menu provider in the
helper.
3741bac
@mbontemps mbontemps merged commit 9ef5600 into from
@stof stof commented on the diff
Templating/Helper/MenuHelper.php
@@ -37,7 +37,9 @@ class MenuHelper extends Helper implements \ArrayAccess
*/
public function get($name)
{
- return $this->provider->getMenu($name);
+ $menu = $this->provider->getMenu($name);
+ $menu->setCurrentUri($this->container->get('request')->getRequestUri());
@stof Owner
stof added a note

this causes an issue when rendering the menu in a subrequest as the request uri will be the wrong one and you force using it.

@uwej711
uwej711 added a note

Would it make sense to make this configurable? I think there are cases where no subrequests are used ... or is there a better way to find out the original uri?

@stof Owner
stof added a note

From a subrequest, you cannot know the main request uri, even more when the subrequest is delayed by using ESI.

@uwej711
uwej711 added a note

So what do you think about making that configurable? From what you said I guess marking the current menu item automatically will never work when using sub request.

@dbu
dbu added a note

did you find a solution to the issue? or how could an ESI sub request for the menu with correct highlighting work? pass a magical parameter to highlight the right entry? maybe we could add support for passing the highlighted path as a get parameter with configurable name?...

@stof Owner
stof added a note

I don't think we should add some magic to choose a get parameter (what if the user passes it as request attribute which is easier when creating subrequests ?)
We should simply make the automatic retrieval of the current uri configurable.

@dbu
dbu added a note

yes, makes more sense than randomly adding something and the users of the lib needing to roll their own thing. shall we create an issue on the issue tracker for this topic?

@stof Owner
stof added a note

yeah probably. It would be easier to track it that way than commenting on another PR.

@dbu
dbu added a note

i think this also has a relation to the issue 48 #48 by @cirpo . should we create a new issue or discuss in 48?

@uwej711: or was there a different reason for needing the request url known in the menu?

@stof Owner
stof added a note

I think this PR is just about lazyness to avoid having to set the current uri when building the menu :)

@dbu
dbu added a note

created #56 to track this further. its a general issue in fact. but i think uwe should still check out #48 because we wanted to solve that problem originally...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@bamarni bamarni referenced this pull request
Closed

Adds a controller voter #133

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Jul 27, 2011
  1. @benjamindulau

    When rendering the menu through a twig template, the depth option.

    benjamindulau authored
    Also, the depth default value is now 1 instead of null, which is IMO the most common use case.
Commits on Jul 28, 2011
  1. @benjamindulau

    * Removed unused "path" option in MenuExtension and MenuHelper

    benjamindulau authored
    * Default of the depth option is back to "null"
Commits on Jul 30, 2011
  1. @uwej711
  2. @uwej711

    Automatically set the current request uri to the menu

    uwej711 authored
    To make adding the class current to the current menu item work, there
    needs to be a way to inform the menu of the current request uri. This
    can be done, when the menu is fetched from the menu provider in the
    helper.
This page is out of date. Refresh to see the latest.
View
5 Resources/views/Menu/item.html.twig
@@ -5,8 +5,9 @@
{% else %}
<span {{ menu.attributes(item.labelAttributes)|raw }}>{{ item.label|raw }}</span>
{% endif %}
- {% if item.hasChildren %}
- {% include 'KnpMenuBundle:Menu:menu.html.twig' with {'item': item, 'menu': menu } %}
+ {% if item.hasChildren and (depth is none or depth > 1) %}
+ {% set depth = depth ? depth - 1 : null %}
+ {% include 'KnpMenuBundle:Menu:menu.html.twig' with {'item': item, 'menu': menu, 'depth': depth} %}
{% endif %}
</li>
{% endif %}
View
8 Templating/Helper/MenuHelper.php
@@ -37,7 +37,9 @@ public function __construct(ProviderInterface $provider, ContainerInterface $con
*/
public function get($name)
{
- return $this->provider->getMenu($name);
+ $menu = $this->provider->getMenu($name);
+ $menu->setCurrentUri($this->container->get('request')->getRequestUri());
@stof Owner
stof added a note

this causes an issue when rendering the menu in a subrequest as the request uri will be the wrong one and you force using it.

@uwej711
uwej711 added a note

Would it make sense to make this configurable? I think there are cases where no subrequests are used ... or is there a better way to find out the original uri?

@stof Owner
stof added a note

From a subrequest, you cannot know the main request uri, even more when the subrequest is delayed by using ESI.

@uwej711
uwej711 added a note

So what do you think about making that configurable? From what you said I guess marking the current menu item automatically will never work when using sub request.

@dbu
dbu added a note

did you find a solution to the issue? or how could an ESI sub request for the menu with correct highlighting work? pass a magical parameter to highlight the right entry? maybe we could add support for passing the highlighted path as a get parameter with configurable name?...

@stof Owner
stof added a note

I don't think we should add some magic to choose a get parameter (what if the user passes it as request attribute which is easier when creating subrequests ?)
We should simply make the automatic retrieval of the current uri configurable.

@dbu
dbu added a note

yes, makes more sense than randomly adding something and the users of the lib needing to roll their own thing. shall we create an issue on the issue tracker for this topic?

@stof Owner
stof added a note

yeah probably. It would be easier to track it that way than commenting on another PR.

@dbu
dbu added a note

i think this also has a relation to the issue 48 #48 by @cirpo . should we create a new issue or discuss in 48?

@uwej711: or was there a different reason for needing the request url known in the menu?

@stof Owner
stof added a note

I think this PR is just about lazyness to avoid having to set the current uri when building the menu :)

@dbu
dbu added a note

created #56 to track this further. its a general issue in fact. but i think uwe should still check out #48 because we wanted to solve that problem originally...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+ return $menu;
}
/**
@@ -89,10 +91,9 @@ public function offsetUnset($name)
* @param string $template (optional)
* @return string
*/
- public function render($name, $path = null, $depth = null, $template = null)
+ public function render($name, $depth = null, $template = null)
{
$item = $this->get($name);
- $item->initialize(array('path' => $path));
return $this->doRender($item, $depth, $template);
}
@@ -126,6 +127,7 @@ public function doRender(MenuItem $item, $depth = null, $template = null)
return trim($this->container->get('templating')->render($template, array(
'item' => $item,
'menu' => $this,
+ 'depth' => $depth,
)));
}
View
4 Twig/MenuExtension.php
@@ -49,9 +49,9 @@ public function get($name)
* @param integer $depth (optional)
* @return string
*/
- public function render($name, $path = null, $depth = null, $template = null)
+ public function render($name, $depth = null, $template = null)
{
- return $this->helper->render($name, $path, $depth, $template);
+ return $this->helper->render($name, $depth, $template);
}
/**
Something went wrong with that request. Please try again.