Issues

ZF-11372: Zend_Navigation Should Be Capable of Determining MVC Page Class Based Solely on "params" Option

Issue Type: Bug Created: 2011-05-13T16:16:38.000+0000 Last Updated: 2012-11-16T15:12:27.000+0000 Status: Resolved Fix version(s): - 1.12.1 (18/Dec/12)

Reporter: Bradley Holt (bradley.holt) Assignee: Frank Brückner (frosch) Tags: - Zend_Navigation

  • After1.12.0
  • FixForZF1.12.1
  • state:patch-ready-for-review
  • zf-crteam-padraic
  • zf-crteam-priority
  • zf-crteam-review

Related issues: - ZF-11805

Attachments: - Page.php.patch

Description

If the params option has been specified, then there should be no need to specify the module, controller, or action as page options (assuming a default module, controller, and action have all been configured) in order for Zend_Navigation_Page::factory() to determine whether to create a Zend_Navigation_Page_Mvc or a Zend_Navigation_Page_Uri. The params option is only applicable to MVC pages, so should be sufficient to determine the type of page to create.

Comments

Posted by Bradley Holt (bradley.holt) on 2011-05-13T16:19:40.000+0000

Here is a trivial fix for this issue. I can commit this change once someone else reviews it.

Posted by Kai Uwe (kaiuwe) on 2011-06-08T15:01:50.000+0000

I'll look at this and write unit tests.

Posted by Frank Brückner (frosch) on 2011-09-22T09:25:40.000+0000

Patch and unit test added.

Posted by Adam Lundrigan (adamlundrigan) on 2012-06-02T01:46:22.000+0000

Thanks for your patch. I've reviewed it, and have found two issues: * The test uses assertInstanceOf, which is not available in PHPUnit 3.4. Please use assertType. * The test fails when I run it against trunk:

<pre class="highlight">
1) Zend_Navigation_PageFactoryTest::testMvcShouldHaveDetectionPrecedence
Zend_Navigation_Exception: Invalid argument: Unable to determine class to instantiate (Page label: MVC Page)

/var/www/ZFv1/trunk/library/Zend/Navigation/Page.php:268
/var/www/ZFv1/trunk/tests/Zend/Navigation/PageFactoryTest.php:97

Posted by Frank Brückner (frosch) on 2012-06-04T06:07:46.000+0000

I will create a new patch.

Posted by Frank Brückner (frosch) on 2012-06-06T16:49:03.000+0000

Complete new patch for unit tests.

Posted by Rob Allen (rob) on 2012-11-07T20:50:40.000+0000

Patch applied to trunk (25110) and release-1.12 (25111)

Posted by Bruno Friedmann (brunofriedmann) on 2012-11-13T12:36:18.000+0000

Rob the last patch now create a trouble here with application that use a navigation xml description

Zend Application error Invalid argument: Unable to determine class to instantiate (Page label: label_menu_home)

extract of the xml is label_menu_home indexindex/index/indexlabel_menu_activites activitesindex/activites
label_menu_clients clientsindex/clients

The label has translated in tmx file & method This configuration has work for years now ...

any idea why this would become not more viable ?

Posted by Frank Brückner (frosch) on 2012-11-13T13:14:20.000+0000

Hi Bruno, I have written the patch. :)

The config for your pages are wrong: {quote}

<pre class="highlight">
label_menu_home
    indexindex/index/index

What do you want? A URI-page or a MVC-page?

  • If you want a URI-page remove the "controller" and "action" elements in your XML.
  • If you want a MVC-page remove the "uri" element in your XML.

Posted by Frank Brückner (frosch) on 2012-11-13T13:22:17.000+0000

@Bruno

In the old version you got a MVC-page, because MVC was the preferred page type and that was a problem: It was not documented! Many people wrote the config like you, but some thought they also get the URL from .

Posted by Rob Allen (rob) on 2012-11-14T19:38:02.000+0000

Does this count as a BC-break?

Posted by Rob Allen (rob) on 2012-11-14T19:42:19.000+0000

Thinking a bit more, the fix for this bug shouldn't have changed the behaviour Bruno is reporting as it should have just chosen MVC if params exists. Thoughts?

Posted by Frank Brückner (frosch) on 2012-11-15T17:02:22.000+0000

Hi Rob, this first part of the patch does so:

<pre class="highlight">
         $hasUri = isset($options['uri']);
         $hasMvc = isset($options['action']) || isset($options['controller']) ||
-                  isset($options['module']) || isset($options['route']);
+                  isset($options['module']) || isset($options['route']) ||
+                  isset($options['params']);

The second part of the patch fixes a problem with a none documented behaviour: if $options contains a key like "controller", "action" or "route" and also the key "uri" – always a MVC page was created. The MVC page was the preferred page type in the code but this fact was nowhere documented!

The second part:

<pre class="highlight">
-        if ($hasMvc) {
+        if ($hasMvc && !$hasUri) {
             require_once 'Zend/Navigation/Page/Mvc.php';
             return new Zend_Navigation_Page_Mvc($options);
-        } elseif ($hasUri) {
+        } elseif ($hasUri && !$hasMvc) {
             require_once 'Zend/Navigation/Page/Uri.php';
             return new Zend_Navigation_Page_Uri($options);
         } else {

All navigations, which breaks with this patch, have a wrong configuration.

Posted by Rob Allen (rob) on 2012-11-16T13:39:32.000+0000

I don't disagree that it wasn't documented. I just don't think that we can change that behaviour now after so many years.

Posted by Frank Brückner (frosch) on 2012-11-16T13:56:33.000+0000

I think you're right. I will add a patch.

Posted by Rob Allen (rob) on 2012-11-16T15:12:27.000+0000

Thanks Frank! I've applied to trunk as r25124 and release-1.12 as 25125.

Have you found an issue?

See the Overview section for more details.

Copyright

© 2006-2016 by Zend, a Rogue Wave Company. Made with by awesome contributors.

This website is built using zend-expressive and it runs on PHP 7.

Contacts