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

Add rest\GroupUrlRule; add rest\UrlRule param controllerPrefix #14996

Closed

Conversation

bscheshirwork
Copy link
Contributor

@bscheshirwork bscheshirwork commented Oct 20, 2017

For more "versioned" of config of RESTful api application

All of rules of major version is one rest\GroupUrlRule;
In nested rules module prefix is omitted;

Current

return [
    'modules' => [
        'v1' => [
            'class' => 'app\modules\v1\Module',
        ],
        'v2' => [
            'class' => 'app\modules\v2\Module',
        ],
    ],
    'components' => [
        'urlManager' => [
            'enablePrettyUrl' => true,
            'enableStrictParsing' => true,
            'showScriptName' => false,
            'rules' => [
                ['class' => 'yii\rest\UrlRule', 'controller' => ['v1/user', 'v1/post']],
                ['class' => 'yii\rest\UrlRule', 'controller' => ['v2/user', 'v2/post']],
            ],
        ],
    ],
];

propose

return [
    'modules' => [
        'v1' => [
            'class' => 'app\modules\v1\Module',
        ],
        'v2' => [
            'class' => 'app\modules\v2\Module',
        ],
    ],
    'components' => [
        'urlManager' => [
            'enablePrettyUrl' => true,
            'enableStrictParsing' => true,
            'showScriptName' => false,
            'rules' => [
                // v1
                [
                    'class' => 'yii\rest\GroupUrlRule',
                    'prefix' => 'v1',
                    'rules' => [
                        [
                            'controller' => 'post',
                            'pluralize' => false,
                            'only' => ['index', 'view', 'options'],
                        ],
                       'user',
                        //another yii\rest\UrlRule of v1 module with differences in params
                    ],
                ],
                // v2
                [
                    'class' => 'yii\rest\GroupUrlRule',
                    'prefix' => 'v2',
                    'rules' => [
                        [
                            'controller' => 'post',
                            'pluralize' => false,
                            'only' => ['index', 'view', 'options'],
                        ],
                        'u' => 'user',
                        //another yii\rest\UrlRule of v2 module with differences in params
                    ],
                ],
                //...
            ],
        ],
    ],
];
Q A
Is bugfix? no
New feature? yes
Breaks BC? no
Tests pass? yes
Fixed issues

@bscheshirwork
Copy link
Contributor Author

bscheshirwork commented Oct 20, 2017

huh, I get some error in tests with dataProvider config %)

        $rule = [ // Rule properties
            'prefix' => 'v1',
            'rules' => [
                [
                    'controller' => 'channel',
                    'pluralize' => true,
                ],
            ],
        ];
        $manager = new \yii\web\UrlManager([
            'cache' => null,
        ]);
        $rule = new \yii\rest\GroupUrlRule($rule);

is work correctly...
so what I do wrong?

upd: copy-paste 😸 I do catch a vulnerables...

@samdark samdark added this to the 2.0.16 milestone Oct 20, 2017
@rob006
Copy link
Contributor

rob006 commented Oct 21, 2017

Is it really necessary to create another group rule class? We could adjust yii\web\GroupUrlRule to work properly with rest rules. #9122

@bscheshirwork
Copy link
Contributor Author

bscheshirwork commented Oct 21, 2017

@rob006 as you can see in code this class is a sugar for module name.

For more "versioned" of config of RESTful api application

your example does not relate to the idea of PR

In current realisation we must set prefix to all controller manually

        'urlManager' => [
            'enablePrettyUrl' => true,
            'enableStrictParsing' => true,
            'showScriptName' => false,
            'rules' => [
                // v1 use 'yii\web\GroupUrlRule'
                [
                    'class' => 'yii\web\GroupUrlRule',
                    'prefix' => 'v1',
                    'ruleConfig' =>  ['class' => 'yii\rest\UrlRule'],
                    'rules' => [
                        [
                            'controller' => 'v1/post',
                            'pluralize' => false,
                            'only' => ['index', 'view', 'options'],
                        ],
                       ['controller' => 'v1/user'],
                        //another yii\rest\UrlRule of v1 module with differences in params
                    ],
                ],
                // v2 use 'yii\rest\GroupUrlRule'
                [
                    'class' => 'yii\rest\GroupUrlRule',
                    'prefix' => 'v2',                    
                    'rules' => [
                        [
                            'controller' => 'post',
                            'pluralize' => false,
                            'only' => ['index', 'view', 'options'],
                        ],
                        'u' => 'user',
                        //another yii\rest\UrlRule of v2 module with differences in params
                    ],
                ],
                //...
            ],

yii\web\GroupUrlRule not implement module prefix for controller property of nested yii\rest\UrlRule;
Set prefix to controller property from yii\web\GroupUrlRule is imposible (also callsble is not implemented)

I propose "isolate" version of api from config of nested rules

@rob006
Copy link
Contributor

rob006 commented Oct 22, 2017

yii\web\GroupUrlRule not implement module prefix for controller property of nested yii\rest\UrlRule;

It has $prefix and $routePrefix which is basically the same. You just changed name of these properties. Why not use $routePrefix and $prefix directly in $ruleConfig?

@bscheshirwork
Copy link
Contributor Author

@rob006 yes of course.

2 differences:

  • first
    $routePrefix property is not implemented in yii\rbac\UrlRule and in yii\web\CompositeUrlRule

  • second
    additional "short form" can't be use.

For first:
I propose add the $controllerPrefix property to yii\rbac\UrlRule. This property is same at routePrefix in another rule, but is named according to meaning.

This property unbind of $prefix by BC reason (unlike $routePrefix = $routePrefix ?: $route).

For this reason the yii\rbac\GroupUrlRule process prefix and controllerPrefix and pass it both.

@bscheshirwork
Copy link
Contributor Author

bscheshirwork commented Oct 23, 2017

And, before you can ask...

If we will add only contrllerPrefix property in yii\rest\UrlRule

        'urlManager' => [
            'enablePrettyUrl' => true,
            'enableStrictParsing' => true,
            'showScriptName' => false,
            'rules' => [
                // v1 use 'yii\web\GroupUrlRule' and property `yii\rest\UrlRule` `controllerPrefix`
                [
                    'class' => 'yii\web\GroupUrlRule',
                    'prefix' => 'v1',
                    'routePrefix' => 'v1',
                    'ruleConfig' =>  [
                        'class' => 'yii\rest\UrlRule',
                        'prefix' => 'v1',
                        'controllerPrefix' => 'v1',
                    ],
                    'rules' => [
                        [
                            'controller' => 'post',
                            'pluralize' => false,
                            'only' => ['index', 'view', 'options'],
                        ],
                       ['controller' => 'user'],
                        //another yii\rest\UrlRule of v1 module with differences in params
                    ],
                ],
                // v2 use 'yii\rest\GroupUrlRule' use both property `controllerPrefix` and class `yii\rest\GroupUrlRule`
                [
                    'class' => 'yii\rest\GroupUrlRule',
                    'prefix' => 'v2',                    
                    'rules' => [
                        [
                            'controller' => 'post',
                            'pluralize' => false,
                            'only' => ['index', 'view', 'options'],
                        ],
                        'u' => 'user', // example of short syntax of nested rule controller property
                        //another yii\rest\UrlRule of v2 module with differences in params
                    ],
                ],
                //...
            ],

@bscheshirwork
Copy link
Contributor Author

related #13968

@rob006
Copy link
Contributor

rob006 commented Oct 23, 2017

$routePrefix property is not implemented in yii\rbac\UrlRule and in yii\web\CompositeUrlRule

You can implement it in the same way as $controllerPrefix. You will get unified API and less duplicated code and redundant classes.

@bscheshirwork
Copy link
Contributor Author

@rob006 $controllerPrefix uses instead of $routePrefix
because logic of applied $prefix to empty $controllerPrefix in yii\rest\UrlRule and to empty $routePrefix in yii\web\GroupUrlRule is difference.

        $this->prefix = trim($this->prefix, '/');
        $this->routePrefix = $this->routePrefix === null ? $this->prefix : trim($this->routePrefix, '/');
        $this->prefix = trim($this->prefix, '/');
        $this->controllerPrefix = trim($this->controllerPrefix, '/');

You propose use $routePrefix in yii\rest\UrlRule instead of $controllerPrefix anyway?

@rob006
Copy link
Contributor

rob006 commented Oct 23, 2017

because logic of applied $prefix to empty $controllerPrefix in yii\rest\UrlRule and to empty $routePrefix in yii\web\GroupUrlRule is difference.

But why? Sorry, I really don't get what is the problem here and why $controllerPrefix must be different from $routePrefix.

@bscheshirwork
Copy link
Contributor Author

bscheshirwork commented Oct 23, 2017

@rob006 just BC compatibility

I am a developer.
I use yii2 for RESTful api
I have a worked api
I have config

return [
    'modules' => [
        'v1' => [
            'class' => 'app\modules\v1\Module',
        ],
        'v2' => [
            'class' => 'app\modules\v2\Module',
        ],
    ],
    'components' => [
        'urlManager' => [
            'enablePrettyUrl' => true,
            'enableStrictParsing' => true,
            'showScriptName' => false,
            'rules' => [
                ['class' => 'yii\rest\UrlRule', 'prefix' => 'api/v1', 'controller' => ['v1/user', 'v1/post']],
                ['class' => 'yii\rest\UrlRule', 'prefix' => 'api/v2', 'controller' => ['v2/user', 'v2/post']],
                // ... or any else - see tests case below
                ['class' => 'yii\rest\UrlRule', 'prefix' => 'admin', 'controller' => 'post'], 
            ],
        ],
    ],
];

[
'prefixed route',
['controller' => 'post', 'prefix' => 'admin'],
[
['admin/posts', 'post/index'],
],
],

'api/v1/posts', 'v1/post/index'
...
'admin/posts', 'post/index'

I (we, contributors of framework) add property to yii\rest\UrlRule, property is prefix of controller;

  1. With relation of prefix (like yii\web\GroupUrlRule)
    I have init()
        $this->prefix = trim($this->prefix, '/');
        $this->routePrefix = $this->routePrefix === null ? $this->prefix : trim($this->routePrefix, '/');

I have wrong resolve

'api/v1/posts', 'api/v1/v1/post/index'
...
'admin/posts', 'admin/post/index'
  1. Unbind / independent properties

I have init()

        $this->prefix = trim($this->prefix, '/');
        $this->controllerPrefix = trim($this->controllerPrefix, '/');

I have my code, who still work

@rob006
Copy link
Contributor

rob006 commented Oct 28, 2017

In your example you're not using GroupUrlRule, so this will not change anything. AFAIK GroupUrlRule doesn't work well with yii\rest\UrlRule so you can't really break BC for thing that doesn't work. :D

But even if I'm wrong and this will break BC, I would rather move this to 2.1 than introduce yii\rest\GroupUrlRule which do exactly the same thing as yii\web\GroupUrlRule, but in different way (for no reason) and with different names of properties. We should keep API simple and introducing two different interfaces for the same job is confusing and harder to maintain.

@bscheshirwork
Copy link
Contributor Author

which do exactly the same thing as

Not same.
yii\web\GroupUrlRule set prefix and routePrefix for self

yii\rest\GroupUrlRule set prefix and controllerPrefix (aka routePrefix) for self
AND propose sugar for set controller property
AND set another default class for nested rules.
AND set controllerPrefix (aka routePrefix) for nested rules

I will change controllerPrefix to routePrefix without null-fill logic and add todo for BC broken changes in 2.1

Also I will split changes into 2 PR.

But it is not same.

@samdark samdark modified the milestones: 2.0.16, 2.0.14 Feb 3, 2018
@samdark samdark modified the milestones: 2.0.14, 2.1.0 Feb 11, 2018
@samdark
Copy link
Member

samdark commented Apr 23, 2019

Won't be merged into 2.0.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants