[NFR] Support default parameters in Router/URL generation.

[temuri416]

The code and comments below:

    $di = new \Phalcon\DI\FactoryDefault();
    $di->set('router', function() {
        $router = (new \Phalcon\Mvc\Router(false)) // Don't want the default routes.
            ->setDefaultModule('admin') // NOTICE! Default name is provided
            ->setDefaultController('index') // NOTICE! Default name is provided
            ->setDefaultAction('index'); // NOTICE! Default name is provided
        $router->add('/:module/:controller/:action/:int', [
            'module' => 1,
            'controller' => 2,
            'action' => 3,
            'oid' => 4 // I expect the integer value to be assigned to "oid" key
        ])->setName('integer');

        return $router;
    }, true);

    $router = $di->get('router');

    $router->handle('/admin/tools/save/10'); // At this point I expect 'oid' parameter to be equal to 10.

    // Now I'm grabbing the URL from DI and I expect URL to be aware of all the routing events happened earlier.
    $url = $di->get('url');
    $url->setBaseUri('/offset/');

    // I expect $test1 to be "/offset/admin/tools/other/10", but it is "/offset//other//"
    $test1 = $url->get([
        'for' => 'integer',
        'controller' => 'other',
    ]);

    // I expect $test2 to be "/offset/admin/tools/save/4", but it is "/offset////"
    $test2 = $url->get([
        'for' => 'integer',
        'oid' => 4
    ]);

When generating URLs, the router should be able to fall back to default parameter values if they are not provided to $url->get() method.

Want to back this issue? Post a bounty on it! We accept bounties via Bountysource.

[skolman]

👍
I am waiting for default parameters too!

[temuri416]

@phalcon Do you have any objection to the behaviour that I described?

We have implemented router defaults and can issue pull request. Let me know whether you'd like to review it first.

Thanks.

[temuri416]

@sjinks

A friend has written a fix for this and I've tested it, seems like working fine.

The problem is that he does not have more time to spend on this.

Can I send you the patch for your review and possible merge?

[temuri416]

@sjinks

It's here: https://github.com/wasp2k/cphalcon/commits/url-defaults

[ghost]

OK looking

[ghost]

The patch looks good to me, I will try to write a test case. Is your friend OK if his patch is included to Phalcon?

[temuri416]

He is totally OK with it.

I've been writing a sort of a test myself. Maybe you will find this useful. It does not cover all cases, however.

<?php

$di = new Phalcon\DI\FactoryDefault;

$defController = 'home';
$defAction = 'index';

$router = $di['router'];
$router->removeExtraSlashes(true)
    ->setDefaultNamespace('My\Controllers')
    ->setDefaultModule('public')
    ->setDefaultController($defController)
    ->setDefaultAction($defAction);

$router->add('/:controller/:action/:params', [
    'controller' => 1,
    'action' => 2,
    'params' => 3
])->setName('secured');

$offset = '/offset/';

$testData = [
    'secured' => [
        [
            'params' => null,
            'expected' => $offset,
            'descr' => '
/**
 * Info: Case when no route parameters are provided, and all default values will be used (NULL behaviour).
 * Expected: Offset value only, without any default values appended
 */'
        ],

        [
            'params' => [],
            'expected' => $offset,
            'descr' => '
/**
 * Info: Case when no route parameters are provided, and all default values will be used (empty array behaviour).
 * Expected: Offset value only, without any default values appended
 */'
        ],

        [
            'params' => [
                'params' => ['a', '1', 'b', 'c']
            ],
            'expected' => $offset . $defController . '/' . $defAction . '/a/1/b/c/',
            'descr' => '
/**
 * Info: Case when only ":params" fragment is provided and is an array.
 * Expected: Offset value appended by default controller, default action and ":params" array values imploded by "/".
 */'
        ],

        [
            'params' => [
                'params' => []
            ],
            'expected' => $offset . $defController . '/' . $defAction . '/a/1/b/c/',
            'descr' => '
/**
 * Info: Case when only ":params" fragment is provided and is an empty array.
 * Expected: Offset value only, without any default values appended
 */'
        ],
    ],
];

foreach ($testData as $routeName => $cases) {
    foreach ($cases as $idx => $case) {
        $url = $di['url'];
        $url->setBaseUri($offset);
        $expected = $case['expected'];
        $params = (array) $case['params'] + ['for' => $routeName];
        $actual = $url->get($params);
        echo "-------- Running case #" . ($idx + 1) . " --------\n";
        if ($actual !== $expected) {
            echo "Test failed. Expected value: '{$expected}', actual: '{$actual}'\n";
            echo "{$case['descr']}\n\n";
        } else {
            echo "Passed: {$actual}\n\n";
        }
    }
}

[ghost]

-------- Running case #1 --------
Passed: /offset/

-------- Running case #2 --------
Passed: /offset/

-------- Running case #3 --------
Test failed. Expected value: '/offset/home/index/a/1/b/c/', actual: '/offset/home/index/a/1/b/c'

/**
 * Info: Case when only ":params" fragment is provided and is an array.
 * Expected: Offset value appended by default controller, default action and ":params" array values imploded by "/".
 */

-------- Running case #4 --------
Test failed. Expected value: '/offset/home/index/a/1/b/c/', actual: '/offset/home/index/'

/**
 * Info: Case when only ":params" fragment is provided and is an empty array.
 * Expected: Offset value only, without any default values appended
 */

[ghost]

Are you sure about the fourth test case? /offset/home/index/ looks good to me

[temuri416]

reviewing it..

[temuri416]

Yes, you are right. For the fourth the expected value should be /offset/home/index/

What about the third case - should there be a trailing slash or not?

[ghost]

I think not. All parameters are joined with a slash, thus [a, 1, b, c] => a/1/b/c

[ghost]

OK the code works for me except this thing:

$router->handle('/admin/tools/save/10'); // At this point I expect 'oid' parameter to be equal to 10.

// Now I'm grabbing the URL from DI and I expect URL to be aware of all the routing events happened earlier.
$url = $di->get('url');

As far as I can tell, URL is unaware of all those routing events.

This gives us

--ACTUAL--
/offset/admin/other
/offset/admin/index/index/4
--EXPECT--
/offset/admin/tools/other/10
/offset/admin/tools/save/4

This actually makes sense to me: URL uses the default values from the router, not from the matched route.

Is this OK for you?

[temuri416]

yes. If you get the fix in, I'll write more tests and if I have further questions, I'll bug you once more. thanks!

[ghost]

git checkout -b url-defaults
git pull https://github.com/sjinks/cphalcon.git url-defaults

if you want to test it.

[temuri416]

Are you waiting for me to test this first, before you do pull request?

[ghost]

No, I am cleaning the patch up ATM.

But if you can run some tests to make sure everything is good, that would be great :-)

[temuri416]

I will test it ASAP, when I have a small break from my work project. I will most likely come up with more improvements :-)

[temuri416]

@sjinks Any movement on this? Waiting impatiently :)

[ghost]

@temuri416 I am sorry to say that the patch had to be reverted because it broke the existing code badly (#1938, #1961).

However, @xboston said he would add more test cases to cover Mvc\Router's code; once this is done and we have better test coverage, I will return to this case.

[temuri416]

oh well.

as a general comment, I'd like to say that URL generation in Phalcon is quite painful, especially when it comes to assembling URL when you're trying to match route's named segments.

please let me know if you'd like me to formulate it further.

thank you.

[ghost]

as a general comment, I'd like to say that URL generation in Phalcon is quite painful

I tend to agree… However, without a good test coverage it is very hard to rewrite it — because my changes will likely break backwards compatibility.

If I rewrite the router and related things in PHP, will you be able to help improving the PHP code?

[temuri416]

I will be more than happy to help you doing that.

[stale]

Thank you for contributing to this issue. As it has been 90 days since the last activity, we are automatically closing the issue. This is often because the request was already solved in some way and it just wasn't updated or it's no longer applicable. If that's not the case, please feel free to either reopen this issue or open a new one. We will be more than happy to look at it again! You can read more here: https://blog.phalconphp.com/post/github-closing-old-issues