Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

yii\rest\UrlRule doesn't support route params. #13968

Closed
Faryshta opened this issue Apr 12, 2017 · 19 comments
Closed

yii\rest\UrlRule doesn't support route params. #13968

Faryshta opened this issue Apr 12, 2017 · 19 comments

Comments

@Faryshta
Copy link
Contributor

Faryshta commented Apr 12, 2017

What steps will reproduce the problem?

on config/main.php

'urlManager' => [
    'rules' => [
        [
            'class' => \yii\rest\UrlRule::class,
            'controller' => ['store/<store_id:\d+>/employee' => 'store-employee'],
        ],
    ],
],

What is the expected result?

use the StoreEmployeeController controller when accessing the route store/1/employee

What do you get instead?

http status 404 the rest rule is not even parsed.

Additional info

caused by line https://github.com/yiisoft/yii2/blob/master/framework/rest/UrlRule.php#L221 since it executes

strpos('store/1/employee', 'store/<store_id:\d+>/employee')

which obviously will return false. there is no work-around this. when removing this line (and the ending bracket) the url is parsed properly.

@cebe
Copy link
Member

cebe commented Apr 12, 2017

Your config looks really weird, why are you configuring the class of UrlManager to be yii\rest\UrlRule?
Also, a controller name can not contain variables. Please provide more detailed/correct information.

@Faryshta
Copy link
Contributor Author

@cebe my bad i fixed the original example

@cebe
Copy link
Member

cebe commented Apr 13, 2017

From the PHPdoc:

the controller ID (e.g. user, post-comment) that the rules in this composite rule are dealing with.

what makes you think it could contain parameters?

@Faryshta
Copy link
Contributor Author

Faryshta commented Apr 13, 2017

if it doesn't support route params then lets add support to route params.

http://www.yiiframework.com/doc-2.0/yii-rest-urlrule.html#$controller-detail

you want to explicitly specify how the controller ID should appear in the patterns, you may use an array with the array key being as the controller ID in the pattern, and the array value the actual controller ID. For example, ['u' => 'user'].

it make sense to be able to have a pattern like the one i mention on my example.

@Faryshta
Copy link
Contributor Author

@cebe so can i make a pr with the support for this?

@cebe
Copy link
Member

cebe commented Apr 13, 2017

I do not think this is a good idea to support this inside $controller property. Using $prefix looks like a better solution to me.

@tunecino
Copy link
Contributor

@Faryshta see #9474

@Faryshta
Copy link
Contributor Author

@cebe thats suboptimal specially when handling all the controllers in an api

    'rules' => [
        [
            'prefix' => 'api/v1/',
            'class' => \yii\rest\UrlRule::class,
            'controller' => [
                'store',
                'store/<store_id:\d+>/employee' => 'store-employee'
            ],
        ],
    ],

make more sense than your alternative.

adding the support can be done with removing just one line of code and as @tunecino pointed this feature has been requested since 2015.

@Faryshta
Copy link
Contributor Author

@samdark @SilverFire any input on this?

@samdark
Copy link
Member

samdark commented Apr 19, 2017

Controller ID should not contain anything but controller ID.

@samdark
Copy link
Member

samdark commented Apr 19, 2017

So that leaves us with either extra patterns and prefixes.

@Faryshta
Copy link
Contributor Author

@samdark the docs mentions that the key can support patterns and the value is the controller id it points to

@samdark
Copy link
Member

samdark commented Apr 19, 2017

Nope:

 /**
     * @var string|array the controller ID (e.g. `user`, `post-comment`) that the rules in this composite rule
     * are dealing with. It should be prefixed with the module ID if the controller is within a module (e.g. `admin/user`).
     *
     * By default, the controller ID will be pluralized automatically when it is put in the patterns of the
     * generated rules. If you want to explicitly specify how the controller ID should appear in the patterns,
     * you may use an array with the array key being as the controller ID in the pattern, and the array value
     * the actual controller ID. For example, `['u' => 'user']`.
     *
     * You may also pass multiple controller IDs as an array. If this is the case, this composite rule will
     * generate applicable URL rules for EVERY specified controller. For example, `['user', 'post']`.
     */

@tunecino
Copy link
Contributor

It could be done using prefixes and 2 different rules:

'rules' => [
    [
        'prefix' => 'api/v1/',
        'class' => \yii\rest\UrlRule::class,
        'controller' => ['store'],
    ],
    [
        'prefix' => 'api/v1/store/<store_id:\d+>/',
        'class' => \yii\rest\UrlRule::class,
        'controller' => [
            'employee' => 'store-employee'
        ],
    ]
],

This is also related to https://github.com/yiisoft/yii2/issues/7878 as we can also introduce a new advanced UrlRule class for it. My attempt for it was this one which was based on this idea: https://github.com/yiisoft/yii2/issues/7878#issuecomment-86485350

@Faryshta
Copy link
Contributor Author

I don't see how that is easier to maintain, understand or add new resources than

'rules' => [
    [
        'prefix' => 'api/v1/',
        'class' => \yii\rest\UrlRule::class,
        'controller' => [
            'store',
            'store/<store_id:\d+>/employee' => 'store-employee'
        ],
    ],
],

Specially when dealing with several resources in the same api.

The fix will be just deleting one line of code and it would be 100% backward compatible and would fix several issues such as #8153 and #9474

@samdark samdark added this to the 2.0.13 milestone Apr 19, 2017
@tunecino
Copy link
Contributor

well. I agree with you in that. I think if $controller accepts a custom urlName then why not parsing it too for dynamique tokens same as it is done with $prefix. but I don't know how it could be easily achieved by creating the correct yii\web\UrlRule instance for it without too much change.

@Faryshta
Copy link
Contributor Author

@tunecino https://github.com/tecnocen-com/yii2-roa/blob/master/src/UrlRule.php i only removed an if statement from https://github.com/yiisoft/yii2/blob/master/framework/rest/UrlRule.php#L221 and it works exactly as i mention in my example.

@samdark which tests should i write on the pr?

@samdark
Copy link
Member

samdark commented Apr 20, 2017

@Faryshta first of all, tests which are missing for validating current behavior. These should go in a separate pull request which doesn't change framework code. Then changes + tests for new behavior.

Overall I see why it could be handy. I still find it a bit confusing about using patterns in controller but since there's key=>value syntax anyway I think it's acceptable.

@yii-bot
Copy link

yii-bot commented Apr 5, 2018

Issue moved to yiisoft/yii-api#7

@yii-bot yii-bot closed this as completed Apr 5, 2018
@cebe cebe removed this from the 2.1.1 milestone Apr 6, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants