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

Merged
merged 1 commit into from Apr 23, 2013

Conversation

Projects
None yet
4 participants
Member

adamlundrigan commented Mar 22, 2013

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)

@weierophinney weierophinney added a commit that referenced this pull request Mar 28, 2013

@weierophinney weierophinney Merge branch 'hotfix/4095' into develop
Forward port #4095
7a89d73

tranba commented Apr 20, 2013

@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!

Contributor

Slamdunk commented Apr 23, 2013

@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_

Owner

weierophinney commented Apr 23, 2013

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

weierophinney reopened this Apr 23, 2013

weierophinney merged commit caf8e58 into zendframework:master Apr 23, 2013

1 check passed

default The Travis build passed
Details

@weierophinney weierophinney added a commit to zendframework/zend-navigation that referenced this pull request May 15, 2015

@weierophinney weierophinney Merge pull request zendframework/zendframework#4095 from adamlundriga…
…n/feature/navigation-mvc-routematch-parameters

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

@weierophinney weierophinney added a commit to zendframework/zend-navigation that referenced this pull request May 15, 2015

@weierophinney weierophinney Merge branch 'hotfix/4095' 84330e4

@weierophinney weierophinney added a commit to zendframework/zend-navigation that referenced this pull request May 15, 2015

@weierophinney weierophinney Merge branch 'hotfix/4095' into develop cdf2159
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment