Yii2: URL Rule skipped if default parameter in omitted

Created on 25 Feb 2016 · 21Comments · Source: yiisoft/yii2

I'm probably missing something obvious, but default parameter in a rule just doesn't seem to behave as stated in the routing guide. It claims that if I specify a default for some parameter, I would be able to create URLs using this rule and omitting the parameter. Well, that doesn't work.

Rule configuration:

[
     'pattern' => 'post/<id:\d+>/<slug>',
     'route' => 'post/view',
     'defaults' => ['slug' => ''],
]

I want to be able to create URLs without the slug. So I expect the following call

Url::to(['post/view', 'id' => 100]);

to generate URL /post/view/100. Instead it skips the rule and creates the regular URL /post/view?id=100.

Looking at the framework code, there's a part that puzzles me in UrlRule class:

// match default params
// if a default param is not in the route pattern, its value must also be matched
foreach ($this->defaults as $name => $value) {
// ...
    if (!isset($params[$name])) {
        return false;
    } // ...
}

So it clearly reads that if a parameter is mentioned in defaults section (such as slug in my case), and the value for this parameter is not provided when creating URL, then skip the rule. That contradicts what I read in the documentation.

bug

Source

nirvana-msu

Most helpful comment

IMO, what doc says is the desirable behavior.

As @nirvana-msu said this rule:

[
     'pattern' => 'post/<id:\d+>/<slug>',
     'route' => 'post/view',
     'defaults' => ['slug' => ''],
]

Should match URLs "/post/12" and "/post/12/something". That's an "optional" parameter.

The way to declare it as "optional" is a different issue but according to the doc it should work. Just remove the "if" @nirvana-msu mentioned and it will work.

marcis on 2 Dec 2016

👍3

All 21 comments

The behavior is covered with the tests in UrlRuleTest and seems to be expected.

I'm closing the issue for now, but if it is unexpected - post your comments, please

SilverFire on 26 Feb 2016

@SilverFire this is inconsistent with existing docs and also does not really make sense to me.

http://www.yiiframework.com/doc-2.0/guide-runtime-routing.html#parameterizing-routes

The above rule can be used to parse or create any of the following URLs:
    /index.php/posts: page is 1, tag is ''.
    /index.php/posts/2: page is 2, tag is ''.
    /index.php/posts/2/news: page is 2, tag is 'news'.
    /index.php/posts/news: page is 1, tag is 'news'.
Without using optional parameters, you would have to create 4 rules to achieve the same result.

cebe on 26 Feb 2016

👍1

IMO, the docs that wrong.
There are no optional parameter. You need 4 rules instead of 1.

mdmunir on 3 Aug 2016

this is inconsistent with existing docs and also does not really make sense to me.

Example from guide works fine without any changes:
https://github.com/rob006/yii2-dev/commit/fe63ea246ac4dcbc5d80bf66aa7ecc9ef578a212 => https://travis-ci.org/rob006/yii2-dev/builds/162631606

rob006 on 25 Sep 2016

What happens if you add id to the defaults as well?

'defaults' => ['id' => 1, 'slug' => ''],

YiiRocks on 26 Sep 2016

I have same problem.

@rob006 consider adding these tests:

                'optional params - example from guide',
                [
                    'pattern' => 'posts/<page:\d+>/<tag>',
                    'route' => 'post/index',
                    'defaults' => ['page' => 1, 'tag' => ''],
                ],
                [
                    ['post/index', ['page' => 1, 'tag' => ''], 'posts'],
                    ['post/index', ['page' => 2, 'tag' => ''], 'posts/2'],
                    ['post/index', ['page' => 2, 'tag' => 'news'], 'posts/2/news'],
                    ['post/index', ['page' => 1, 'tag' => 'news'], 'posts/news'],

                   // *** THESE FOUR TESTS:
                    ['post/index', [], 'posts'],
                    ['post/index', ['page' => 1], 'posts'],
                    ['post/index', ['page' => 2], 'posts/2'],
                    ['post/index', ['tag' => 'news'], 'posts/news'],
                ],

This is what personally I would expect from default parameters on url creation. Currently these tests fail.

cronfy on 26 Sep 2016

I created PR for this (#12610), but I'm not really convinced that this should work in this way. It looks too magically for me that 4 different combinations create the same URL.

This is also probably BC breaker.

rob006 on 26 Sep 2016

I tried your PR, looks like some behavior that I would expect is still missing. Please consider adding these tests too:

[
    'optional + mandatory params',
    [
        'pattern' => 'catalog/<category>',
        'route' => 'catalog/index',
        'defaults' => ['page' => 1],
    ],
    [
        ['catalog/index', [], false],
        ['catalog/index', ['page' => 1], false],
        ['catalog/index', ['page' => 1, 'category' => 'a'], 'catalog/a'],
        ['catalog/index', ['page' => 2, 'category' => 'a'], 'catalog/a?page=2'],
    ],
],

The current problem is that it fails on fourth test ['page' => 2, 'category' => 'a']. If we remove 'defaults' => ['page' => 1], correct url will be created: 'catalog/a?page=2', however in this case default is lost.

cronfy on 27 Sep 2016

I tried your PR, looks like some behavior that I would expect is still missing. Please consider adding these tests too:

This behavior is incorrect - if param is not present in pattern then it is not optional. Changing this will make rules less flexible - you will not be able to create rules like this:

[
    'pattern' => 'about-me',
    'route' => 'site/page',
    'defaults' => ['id' => 1],
],
[
    'pattern' => 'contact',
    'route' => 'site/page',
    'defaults' => ['id' => 2],
],

rob006 on 27 Sep 2016

By my opinion "default" means "if parameter is omitted, substitute default value" without any exceptions.

In case we talk about url parsing, the example above looks ok at first glance. Rule matches a pattern -> use this rule, but url parameter is omitted -> substitute default value. Ok, that's how default parameters usually work.

But in case of url creation current yii logic is different: "if parameter matches the value, use this rule, otherwise skip it". It is not about default parameters, it is about parameter matching.

More, suppose request url is example.com/about-me?id=2. What should be the result?

if 'defaults' => ['id' => 1] is just a default, then SiteController->actionPage() should be invoked with id = 2 in GET parameters.
By other hand, if 404 is expected in this case, then 'defaults' => ['id' => 1] was not a default, it was a constraint.

The name 'defaults' for the parameter in discussion is misleading therefore.

cronfy on 27 Sep 2016

What would be more clear is:

[
    'pattern' => 'about-me',
    'route' => 'site/page',
    'args' => ['id' => 1],
],
[
    'pattern' => 'contact',
    'route' => 'site/page',
    'args' => ['id' => 2],
],
[
    'pattern' => 'page/<id:\d+>',
    'route' => 'site/page',
],

This would mean that about-me request should lead to SiteController()->actionPage() with id parameter set to 1, and, vise-versa, route site/page with id=2 parameter should produce about-me url. Same for contact. All site/page routes with other ids would map to page/... url, and site/page route without id would not match any rule.

And:

[
    'pattern' => 'catalog/<category:\w+>',
    'route' => 'catalog/category',
    'defaults' => ['page' => 1],
],

When catalog/books is requested, this would lead to CatalogController()->actionCategory() with page=1, and vise versa. But for route catalog/category with category = books and page = 2 it should produce catalog/books?page=2.

Even possible to mix:

[
    'pattern' => 'about-me',
    'route' => 'site/page',
    'args' => ['id' => 1],
    'defaults' => [ 'language' => 'ru' ]
],

In other words, args is for mandatory parameter values, and defaults is for defaults.

But as 'defaults' option is already in use, the hardest thing is to invent a new name for that 'real defaults' option.

cronfy on 27 Sep 2016

The important thing I forgot to mention:

When catalog/books is requested, this would lead to CatalogController()->actionCategory() with page=1, and vise versa. But for route catalog/category with category = books and page = 2 it should produce catalog/books?page=2.

AND for route catalog/category with category = books and WITHOUT page parameter it should give catalog/books url too.

cronfy on 27 Sep 2016

Possible solution could be following:

Deprecate defaults options.
Introduce mandatoryParams and defaultParams options.

Example:

[
    'pattern' => 'about-me',
    'route' => 'site/page',
    // deprecated: 'defaults' => ['id' => 1],
    'mandatoryParams' => ['id' => 1],
    'defaultParams' => [ 'language' => 'ru' ]
],

cronfy on 28 Sep 2016

@cronfy I think you overcomplicating this. You can just add another rule for this - in most cases it is easier, more readable and more efficient solution.

rob006 on 28 Sep 2016

@rob006 well, if we have 1 default parameter, 2 rules are required (that's how I currently solve this issue):

            [
                'pattern' => "catalog/<category>",
                'route' => "catalog/category",
                "defaults" => ['page' => 1]
            ],
            [
                'pattern' => "catalog/<category>",
                'route' => "catalog/category",
            ],

It allows me to use createUrl() without explicitly specifying page.

But how many rules would be required for 2 default parameters (e.g. example from guide)?

                [
                    'pattern' => 'posts/<page:\d+>/<tag>',
                    'route' => 'post/index',
                    'defaults' => ['page' => 1, 'tag' => ''],
                ],

cronfy on 28 Sep 2016

@cronfy You don't need add any additional rules - you should just explicitly specify parameters on URL creation. This is more safe and less magic solution.

Anyway, #12610 address this without introducing mandatory and default params.

rob006 on 28 Sep 2016

Anyway, #12610 address this without introducing mandatory and default params.

@rob006 hmm, yes, I gave wrong example. #12610 really should fix what's mentioned at the first post of this issue.

But it does not fix case when parameters specified in defaults are not it pattern.

If this is not going to be fixed, then documetation should state that defaults can be used only for parameters specified in pattern and emphathize that it will break url creation if parameters are not in pattern.

cronfy on 28 Sep 2016

Probably defaults should even throw an exception if they are not mentioned in pattern.

See https://github.com/yiisoft/yii2/issues/10970#issuecomment-249818020

You expecting that URL rules should configure default GET params for app components. This is dirty solution, we should avoid it. I think that #12610 should be narrowed only for empty default params.

rob006 on 28 Sep 2016

Probably defaults should even throw an exception if they are not mentioned in pattern.
See #10970 (comment)

Sorry, my suggestion about exception was wrong, I deleted it.

You expecting that URL rules should configure default GET params for app components. This is dirty solution, we should avoid it. I think that #12610 should be narrowed only for empty default params.

Well, I still think it should be mentioned in the documentation somehow.

I see difference between

[
    'pattern' => 'about-me',
    'route' => 'site/page',
    'defaults' => ['id' => 1],
],

and

[
    'pattern' => 'catalog/index',
    'route' => 'catalog/index',
    'defaults' => ['page' => 1],
],

But without this discussion I would likely not understand why I can use the former, but can't use the latter, because they look the same.

cronfy on 28 Sep 2016

IMO, what doc says is the desirable behavior.

As @nirvana-msu said this rule:

[
     'pattern' => 'post/<id:\d+>/<slug>',
     'route' => 'post/view',
     'defaults' => ['slug' => ''],
]

Should match URLs "/post/12" and "/post/12/something". That's an "optional" parameter.

The way to declare it as "optional" is a different issue but according to the doc it should work. Just remove the "if" @nirvana-msu mentioned and it will work.

marcis on 2 Dec 2016

👍3

Agree with @marcis. This behavior is logical.