Zend Framework

When placing the Route_Module as last in a chain an assertion on seperators in match() might fail

Details

  • Type: Bug Bug
  • Status: Resolved Resolved
  • Priority: Major Major
  • Resolution: Fixed
  • Affects Version/s: 1.10.0
  • Fix Version/s: 1.11.0
  • Component/s: Zend_Controller
  • Labels:
    None

Description

When you define a chain with the default Module route as a final route in the chain the route will fail when the url doesn't explicitly contain a controller or module.

The following example will fail:

public function testChainingDynamicModuleDefaultModuleMatch()
    {
        $foo = new Zend_Controller_Router_Route(':dynamic');
        $dispatcher = new Zend_Controller_Router_ChainTest_Dispatcher();
        $request = new Zend_Controller_Router_ChainTest_Request('http://www.zend.com/foo');
        $bar = new Zend_Controller_Router_Route_Module(array(),$dispatcher,$request);
        $chain = $foo->chain($bar);

        $res = $chain->match($request);
        $this->assertEquals('foo', $res['dynamic']);
        $this->assertEquals('defctrl', $res['controller']);
        $this->assertEquals('defact', $res['action']);
        $this->assertEquals('default', $res['module']);
    }

It doesn't matter if the first route is a static route or (in the above case) a dynamic one.

My patch is an adjustment of the assertion that happens in Chain::match(). In my opinion it should check if the last route is or isn't of the Module type since the Module route normally contains defaults for module, controller and action.

I'll attach a patch I've made.

Issue Links

Activity

Hide
Peter Moolenaar added a comment -

The patch I've included contains (besides the fix for Chain) also an modification of the unit tests.

Of course, if there is a disagreement on the unit tests, the patch for Chain invalidates right away

Show
Peter Moolenaar added a comment - The patch I've included contains (besides the fix for Chain) also an modification of the unit tests. Of course, if there is a disagreement on the unit tests, the patch for Chain invalidates right away
Hide
Matthew Weier O'Phinney added a comment -

Assigning to Ben to review.

Show
Matthew Weier O'Phinney added a comment - Assigning to Ben to review.
Hide
Peter Moolenaar added a comment -

Already had a chance to look at the unit tests?

I'm running with the patched version for a while now (ok, not in production... but still) and haven't experienced any troubles. Still when thinking about the checks that are being performed (which I added myself...) I feel that it might impose unwanted overhead on the Router. For my setup it is totally unnoticeable but for others it might not be... any thoughts?

Show
Peter Moolenaar added a comment - Already had a chance to look at the unit tests? I'm running with the patched version for a while now (ok, not in production... but still) and haven't experienced any troubles. Still when thinking about the checks that are being performed (which I added myself...) I feel that it might impose unwanted overhead on the Router. For my setup it is totally unnoticeable but for others it might not be... any thoughts?
Hide
Peter Moolenaar added a comment -

I think the issue here is basically identical, and thinking about it in that way renders my solution useless since it only deals with the module route, not any other optional routing params

Show
Peter Moolenaar added a comment - I think the issue here is basically identical, and thinking about it in that way renders my solution useless since it only deals with the module route, not any other optional routing params
Hide
Kim Blomqvist added a comment -

I have run the unit test proposed here under the present version of trunk and it pass as expected, because the ZF-8812 was patched yesterday.

Show
Kim Blomqvist added a comment - I have run the unit test proposed here under the present version of trunk and it pass as expected, because the ZF-8812 was patched yesterday.
Hide
Matthew Weier O'Phinney added a comment -

Marking as resolved, based on report by Kim.

Show
Matthew Weier O'Phinney added a comment - Marking as resolved, based on report by Kim.

People

Vote (0)
Watch (1)

Dates

  • Created:
    Updated:
    Resolved: