allow route options/defaults in uri generation

[pine3ree]

this replaces #5 and adds test cases

[weierophinney]

Create separate test cases. That way if the first fails, we're still testing the second. If the second depends on setup from the first, have a method for doing the setup and/or use the @depends annotation, and return $router from the first (and accept it as an argument in the second).

[pine3ree]

@weierophinney
sure, thanks. I'll take a look tomorrow as it's getting late in my timezone. I also had some doubts about providing recursive merging.

[pine3ree]

@weierophinney

maybe it's better to make the nested 2 tests separate AND independent so we can reuse once more the data provider used for the test without optional route parameters

these are passing 55, 56 and 70 in my local setup, if ok i will commit tomorrow.

    public function uriGeneratorDataProvider()
    {
        return [
            // both param1 and params2 are missing => use route defaults
            ['/foo/abc/def', []],

            // param1 is passed to the uri generator => use it
            // param2 is missing => use route default
            ['/foo/123/def', ['param1' => '123']],

            // param1 is missing => use route default
            // param2 is passed to the uri generator => use it
            ['/foo/abc/456', ['param2' => '456']],

            // both param1 and param2 are passed to the uri generator
            ['/foo/123/456', ['param1' => '123', 'param2' => '456']],
        ];
    }

    /**
     * @dataProvider uriGeneratorDataProvider
     */
    public function testUriGenerationSubstitutionsWithDefaultOptions($expectedUri, $params)
    {
        $router = new FastRouteRouter();

        $route = new Route('/foo/{param1}/{param2}', 'foo', ['GET'], 'foo');
        $route->setOptions([
            'defaults' => [
                'param1' => 'abc',
                'param2' => 'def',
            ]
        ]);

        $router->addRoute($route);

        $this->assertEquals($expectedUri, $router->generateUri('foo', $params));
    }

    public function uriGeneratorWithPartialDefaultsDataProvider()
    {
        return [
            // required param1 is missing => use route default
            // optional param2 is missing and no route default => skip it
            ['/foo/abc', []],

            // required param1 is passed to the uri generator => use it
            // optional param2 is missing and no route default => skip it
            ['/foo/123', ['param1' => '123']],

            // required param1 is missing => use default
            // optional param2 is passed to the uri generator => use it
            ['/foo/abc/456', ['param2' => '456']],

            // both param1 and param2 are passed to the uri generator
            ['/foo/123/456', ['param1' => '123', 'param2' => '456']],
        ];
    }

    /**
     * @dataProvider uriGeneratorDataProvider
     */
    public function testUriGenerationSubstitutionsWithDefaultsAndOptionalParameters($expectedUri, $params)
    {
        $router = new FastRouteRouter();

        $route = new Route('/foo/{param1}/{param2}', 'foo', ['GET'], 'foo');
        $route->setOptions([
            'defaults' => [
                'param1' => 'abc',
                'param2' => 'def',
            ]
        ]);

        $router->addRoute($route);

        $this->assertEquals($expectedUri, $router->generateUri('foo', $params));
    }

    /**
     * @dataProvider uriGeneratorWithPartialDefaultsDataProvider
     */
    public function testUriGenerationSubstitutionsWithPartialDefaultsAndOptionalParameters($expectedUri, $params)
    {
        $router = new FastRouteRouter();

        $route = new Route('/foo/{param1}[/{param2}]', 'foo', ['GET'], 'foo');
        $route->setOptions([
            'defaults' => [
                'param1' => 'abc',
            ]
        ]);

        $router->addRoute($route);

        $this->assertEquals($expectedUri, $router->generateUri('foo', $params));
    }

kind regards

PS
the default options array is costant in the data-provider aware tests so there is no need to pass it via the data-provider. Nevertheless i was tempted to add it as a 3rd argument just to have its definition close (above) to the provided-data just for clarity, smt like:

//...
    public function uriGeneratorDataProvider()
    {
        $defaults = [
            'param1' => '123',
            'param2' => '456',
        ];

        return [
           // both param1 and params2 are missing => use route defaults
            ['/foo/abc', [], $defaults],
            //...
//...

• of course the needed changes in the related test methods.

[pine3ree]

@weierophinney
About adding a DefaultParamsTrait:
since in this case we are only using the 'defaults' key of route options and since route parameters should be scalar values, maybe we should skip adding the trait and perhaps just raise an exception for non-scalar options-defaults element values.
A simple array_merge is used also in zend-mvc Segment route inside the assemble() and match() methods (array_merge($this->defaults, $params))

[weierophinney]

@pine3ree The approaches you suggest for both the tests, and omission of the DefaultParamsTrait, both sound sane; go ahead!

[pine3ree]

@weierophinney
just pushed the commit with the refactored test cases.

p.s.
As I suggested, maybe we should check for scalar type in the defaults array, but it seems to me that the right place to do that is the zend-expressive-router repo:

• with this PR both zend-router bridge and fastrouter bridge will be using the 'defaults' key in the route options property int the same way representing default route parameters.
• aura/router vs 2 is using the 'values' key (and that is what is checked and passed in the aura-router-bridge) but v.3 changed it to 'defaults' too...moving to aura/router v.3 will have the benefit of a more common configuration.

This lead me to another - more general - question: should we use a "zend-expressive" configuration and have the bridge packages translate it to the used implementation or should we stick to the implementation configuration or maybe support both (this last option would mean for example that if i use 'defaults' with aura/router v.2 bridge it will be used as 'values', this is just a case 'defaults': block away)?

[weierophinney]

should we use a "zend-expressive" configuration and have the bridge packages translate it to the used implementation or should we stick to the implementation configuration

The idea is that factories will do translation. While we may be able to use the general defaults configuration, we have to be cognizant also of other routing implementations, which may have their own configuration styles. As such, we can defer this decision for now.

[weierophinney]

I think we need more test cases.

Given the following route definition: /{param1}[/{param2}]

• What should the behavior be if param2 is provided a default, but not param1, and no additional parameters are provided to generateUri()?
• What should the behavior be if no defaults are provided, and either no parameters or only param2 are provided to generateUri()?

The other routers, IIRC, raise an exception in such cases, as they are unable to fully assemble the URI.

[pine3ree]

@weierophinney
I believe that testGenerateUriRaisesExceptionForIncompleteUriSubstitutions already covers those cases: when any non-optional segment contains unsubstituted parameters an exception is thrown (behaviour added in #4). defaults (if any) are merged with subtitutions (and used as such) before the code that raises the exception. Do you reckon that we need to add explicit tests when using defaults?