Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Zend\Navigation\Page\Mvc::getHref does not use RouteMatch parameters #4095

Merged
merged 1 commit into from

4 participants

@adamlundrigan

The MVC page type in Navigation does not pull matched parameters from the provided RouteMatch object, which results in an error when using the breadcrumbs view helper:

Fatal error: Zend\Mvc\Router\Exception\InvalidArgumentException: Missing parameter "school" in <zf>/library/Zend/View/Helper/Navigation/AbstractHelper.php on line 466

My route hierarchy:

'router' => array(
    'routes' => array(
        'registration' => array(
            'type' => 'Literal',
            'options' => array(
                'route' => '/registration',
                'defaults' => array(
                    'controller' => 'CdliPortal\Controller\Registration\Dashboard',
                    'action' => 'index',
                ),
            ),
            'may_terminate' => true,
            'child_routes' => array(
                'school' => array(
                    'type' => 'Segment',
                    'options' => array(
                        'route' => '/school/:school',   
                        'constraints' => array(
                            'school' => '[0-9]+',
                        ),
                        'defaults' => array(
                            'controller' => 'CdliPortal\Controller\Registration\School\Dashboard',
                            'action'     => 'index',
                        ),
                    ),
                ),
            ),
        ),
    ),
),

The simplest fix i've found is to change Zend\Navigation\Page\Mvc::getHref to use the parameters from the RouteMatch object (if they are set) as the defaults and then merge the parameters set directly on the Zend\Navigation\Page\Mvc into that:

if ($this->getRouteMatch() !== null) {
    $params = array_merge($this->getRouteMatch()->getParams(), $this->getParams());
} else {
    $params = $this->getParams();
}

Though I am not 100% certain this is a comprehensive fix, or that it doesn't break / cause unexpected results with the other navigation view helpers (my application only uses bredcrumbs currently)

@tranba

@adamlundrigan
@weierophinney
I'm consider this pull request has cause unexpected results with the menu view helpers. For example, with the Album module:

//module.config.php:
'router' => array(
        'routes' => array(
            'album' => array(
                'type'    => 'segment',
                'options' => array(
                    'route'    => '/album[/:action][/:id]',
                    'constraints' => array(
                        'action' => '[a-zA-Z][a-zA-Z0-9_-]*',
                        'id'     => '[0-9]+',
                    ),
                    'defaults' => array(
                        'controller' => 'Album\Controller\Album',
                        'action'     => 'index',
                    ),
                ),
            ),
        ),
    ),
//navigation:
'navigation' => array(
    'default' => array(
        array(
            'label' => 'Home',
            'route' => 'home',
        ),
        array(
            'label' => 'Album',
            'route' => 'album',
            'pages' => array(
                array(
                    'label' => 'Add',
                    'route' => 'album',
                    'action' => 'add',
                ),
                array(
                    'label' => 'Edit',
                    'route' => 'album',
                    'action' => 'edit',
                ),
                array(
                    'label' => 'Delete',
                    'route' => 'album',
                    'action' => 'delete',
                ),
            ),
        ),
    ),
),

//in view script:
<?php echo $this->navigation('navigation')->menu(); ?>

In normal:
- album menu link to: http://localhost/album
- add menu link to: http://localhost/album/add
- edit menu will link to: http://localhost/album/edit
-delete menu will link to: http://localhost/album/delete
But if we edit one album -> url: http://localhost/album/edit/1,
album menu will link to: http://localhost/album/edit/1
add menu will link to: http://localhost/album/add/1
edit menu will link to: http://localhost/album/edit/1
and delete menu will link to: http://localhost/album/delete/1

Sorry for my English!

@Slamdunk

@weierophinney : this PR introduce a big issue in navigation handling.

Navigation URLs must not pull params from active page, otherwise every navigation page with undefined params takes current page params, that is definitively wrong.

However @adamlundrigan is likely also right: current page breadcrumb has to have current page params.

I suggest to:
1) restore default behaviour of Zend\Navigation\Page\Mvc
2) create a specific flag into Zend\Navigation\Page\Mvc to pull current params on-the-fly
3) use the switch in Zend\View\Helper\Navigation\Breadcrumbs and wherever it's needed

This way involves some test deletion and some new code in Zend\View\Helper\Navigation\*: I am not so deep into \Navigation stuff to make such a sensitive PR, so please jump here and speak out your thoughts.

A big note: at the time of writing, release 2.1.5 Zend\View\Helper\Navigation\Menu is unusable

@weierophinney

@Slamdunk Does #4282 fix this issue? (I think it likely does)

@weierophinney weierophinney reopened this
@weierophinney weierophinney merged commit caf8e58 into from
@ghost Unknown referenced this pull request from a commit
@weierophinney weierophinney Merge branch 'hotfix/4095'
Close #4095
eb3b0e5
@ghost Unknown referenced this pull request from a commit
@weierophinney weierophinney Merge branch 'hotfix/4095' into develop
Forward port #4095
06c8db1
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Mar 21, 2013
  1. @adamlundrigan
This page is out of date. Refresh to see the latest.
View
7 library/Zend/Navigation/Page/Mvc.php
@@ -183,7 +183,12 @@ public function getHref()
);
}
- $params = $this->getParams();
+ if ($this->getRouteMatch() !== null) {
+ $params = array_merge($this->getRouteMatch()->getParams(), $this->getParams());
+ } else {
+ $params = $this->getParams();
+ }
+
if (($param = $this->getController()) != null) {
$params['controller'] = $param;
View
26 tests/ZendTest/Navigation/Page/MvcTest.php
@@ -14,6 +14,7 @@
use Zend\Mvc\Router\RouteMatch;
use Zend\Mvc\Router\Http\Regex as RegexRoute;
use Zend\Mvc\Router\Http\Literal as LiteralRoute;
+use Zend\Mvc\Router\Http\Segment as SegmentRoute;
use Zend\Mvc\Router\Http\TreeRouteStack;
use Zend\Mvc\ModuleRouteListener;
use Zend\Mvc\MvcEvent;
@@ -498,4 +499,29 @@ public function testNoExceptionForGetHrefIfDefaultRouterIsSet()
$page->getHref();
$page->setDefaultRouter(null);
}
+
+ public function testMvcPageParamsInheritRouteMatchParams()
+ {
+ $page = new Page\Mvc(array(
+ 'label' => 'lollerblades',
+ 'route' => 'lollerblades'
+ ));
+
+ $route = new SegmentRoute('/lollerblades/view/:serialNumber');
+
+ $router = new TreeRouteStack;
+ $router->addRoute('lollerblades', $route);
+
+ $routeMatch = new RouteMatch(array(
+ 'serialNumber' => 23,
+ ));
+ $routeMatch->setMatchedRouteName('lollerblades');
+
+ $page->setRouter($router);
+ $page->setRouteMatch($routeMatch);
+
+ $this->assertEquals('/lollerblades/view/23', $page->getHref());
+ }
+
+
}
Something went wrong with that request. Please try again.