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

I'll look at this and write unit tests.

Patch and unit test added.

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:

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

I will create a new patch.

Complete new patch for unit tests.

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

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 ?

Hi Bruno, I have written the patch. :)

The config for your pages are wrong: {quote}

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.

@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 {{}}.

Does this count as a BC-break?

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?

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

         $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:

-        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.

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.

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

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