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

Suggestion for yii\rest\CreateAction and yii\rest\UpdateAction #14010

Closed
renom opened this issue Apr 19, 2017 · 17 comments
Closed

Suggestion for yii\rest\CreateAction and yii\rest\UpdateAction #14010

renom opened this issue Apr 19, 2017 · 17 comments

Comments

@renom
Copy link

renom commented Apr 19, 2017

Callable property for yii\rest\CreateAction and yii\rest\UpdateAction which should allows modifying new model before saving was produced. I think it would be useful and more conveniently than make these things in beforeAction of model or make redefinition of actionCreate / actionUpdate entirely.

@samdark
Copy link
Member

samdark commented Apr 19, 2017

Could you provide an example? It's not clear now...

@renom
Copy link
Author

renom commented Apr 19, 2017

For example I wanna to prepare my model by such scenario - set up author_id (author which creates the model) field manually (instead of loading it from the POST data). In the scenario author_id would be set from user.id of the authorised user.

@samdark
Copy link
Member

samdark commented Apr 19, 2017

Still can't get it :(

@cebe
Copy link
Member

cebe commented Apr 19, 2017

so you want a beforeSave() event or function in the action class instead of the model?

@renom
Copy link
Author

renom commented Apr 19, 2017

Ok. For example, can be used something similar with another yii\rest\ActiveController actions. It's findModel for yii\rest\ViewAction, yii\rest\UpdateAction, yii\rest\DeleteAction and prepareDataProvider for yii\rest\IndexAction. I think the suggestion should be done by adding another callable property, which yii\rest\CreateAction and yii\rest\UpdateAction should supports. For example (controller code):

public function actions()
{
    $actions = parent::actions();
    $actions['create']['prepareModel'] = [$this, 'prepareModel'];
    $actions['update']['prepareModel'] = [$this, 'prepareModel'];
    return $actions;
}

public function prepareModel($model)
{
    $model->author_id = Yii::$app->user->id;
    return $model;
}

@Deele
Copy link
Contributor

Deele commented Apr 20, 2017

If I understood op issue correctly, then I had a similar issue where I wanted to load specific language for multi-lingual model, depending on request.

I did it like this:

For $modelClass I create new model class ApiArticle that extends Article.

Inside ApiArticle I added event handlers:

public function init()
{
    parent::init();
    $this->on(
        self::EVENT_AFTER_FIND,
        [$this, 'loadSelectedLanguage']
    );
    $this->on(
        self::EVENT_BEFORE_VALIDATE,
        [$this, 'loadSelectedLanguage']
    );
}

and a method

public function loadSelectedLanguage()
{

    /**
     * @var yii\rest\ActiveController $controller
     */
    $controller = Yii::$app->controller;
    $this->language = $controller->language;
}

With similar method you can make changes, predefine values and do anything else with model that is managed via API.

@renom
Copy link
Author

renom commented Apr 20, 2017

Yes, it should works. But as I understand, you quoted a part of ActiveRecord model code. My offer is add support for similar feature to action classes yii\rest\CreateAction and yii\rest\UpdateAction.

@samdark samdark added this to the 2.0.14 milestone Apr 20, 2017
@Deele
Copy link
Contributor

Deele commented Apr 20, 2017

@renom You are talking about very specific situation that should be dealt with in very specific way. Using ActiveRecord events and extended class is much more flexible and it really works. I don't think that there should be special prepareModel thing for create and update, because it is possible to require preparation for read, read collection and delete actions too, not to mention custom actions that would require some preparation too, and to implement that, it would require too much work to justify not using custom ActiveRecord class.

@renom
Copy link
Author

renom commented Apr 21, 2017

Can you prove justification of necessity for your assertion "because it is possible to require preparation for read, read collection and delete actions too, not to mention custom actions that would require some preparation too"? I don't think so, because there we can use the findModel property for these purposes. As for custom actions... I don't think that they need a similar feature, because it's custom actions, and programmer in any case must develops full code of these actions kind. Some words from Yii developers?

@tunecino
Copy link
Contributor

I'm not sure if adding this feature is a good idea. The ActiveController class is providing simple actions and developers are supposed to override them within their controllers whenever they require a different implementation. Adding more code to default implementation may make it more complex and harder to follow to newcomers and less people will do the override thing as it should be.

@renom
Copy link
Author

renom commented Apr 21, 2017

@tunecino but why the developers added support for similar features inside yii\rest\IndexAction, yii\rest\ViewAction, yii\rest\UpdateAction, yii\rest\DeleteAction? I mean prepareDataProvider and findModel properties, which I mentioned above. I think yii\rest\ActiveController class was added as simplified and typical cases usage version of yii\rest\Controller. It means that a purpose of yii\rest\ActiveController creation is allow to develop restful web api with minimal code repetition between different controllers from the box. In other cases you can use yii\rest\Controller or override methods of yii\rest\ActiveController as you mentioned above.

@tunecino
Copy link
Contributor

Well that makes sense too @renom . I personally understood how REST works after checking the code inside yii\rest\Controller. Before that and by just reading docs I used to extend yii\rest\ActiveController and unsettling its actions even if I didn't need any of them. Understanding how yii\rest\Controller works was the key point to link what I knew from yii\web\Controller to how REST works. So maybe overall improvements to REST should be the following:

  1. improve docs with examples to use yii\rest\Controller before jumping to yii\rest\ActiveController. yii\rest\ActiveController may be better presented as a helper class instead of actual REST.
  2. checkAccess() method may be better moved to yii\rest\Controller.
  3. yii\rest\OptionsAction may be better moved to yii\rest\Controller or at least explained in docs to be added if required. (I remember having seen @cebe suggesting an alternative implementation for it but I can't find that conversation)

I know those are different issues but I'm just trying to say that when the use of yii\rest\Controller and its relation to yii\rest\ActiveController are clearer then it will make sense to improve yii\rest\ActiveController and add features like the one you are suggesting which I think it will become useful in that case.

@Faryshta
Copy link
Contributor

i dont see why its not ok to have a checkAccess() in the controller and the action, we just need a way to attach them without overwrite previous parameters.

something like

class Action
{
     public function checkAccess()
     {
          $this->controller->checkAccess();
          if (null !== $this->checkAccess) {
              call_user_func($this->checkAccess, $this->id);
          }
     }
}

and for the prepareModel(). if you want to modify the model before running the save() logic you need to over write the entire CreateAction::run() method. you can't call parent::run() anywhere so for me it makes sense to have such method.

@renom
Copy link
Author

renom commented Apr 25, 2017

My implementation of this feature for yii\rest\CreateAction.

Contents of vendor/yiisoft/yii2/rest/CreateAction.php file, or code of custom class inherited from yii\rest\CreateAction class:

+   public $prepareModel;

    public function run()
    {
        if ($this->checkAccess) {
            call_user_func($this->checkAccess, $this->id);
        }

        /* @var $model \yii\db\ActiveRecord */
        $model = new $this->modelClass([
            'scenario' => $this->scenario,
        ]);

        $model->load(Yii::$app->getRequest()->getBodyParams(), '');
+       if($this->prepareModel !== null) {
+           $model = call_user_func($this->prepareModel, $model);
+       }
        if ($model->save()) {
            $response = Yii::$app->getResponse();
            $response->setStatusCode(201);
            $id = implode(',', array_values($model->getPrimaryKey(true)));
            $response->getHeaders()->set('Location', Url::toRoute([$this->viewAction, 'id' => $id], true));
        } elseif (!$model->hasErrors()) {
            throw new ServerErrorHttpException('Failed to create the object for unknown reason.');
        }

        return $model;
    }

Next you should override an actions() method of your controller, and describe code of custom prepareModel realization:

// some code of some controller
// ...
// ...
// code ends
+   public function actions()
+   {
+       $actions = parent::actions();
+       $actions['create']['prepareModel'] = [$this, 'prepareModel'];
+       return $actions;
+   }
+
+   public function prepareModel($model)
+   {
+       // fills up field manually
+       $model->some_field = 'some value';
+       return $model;
+   }
// code of your controller follows further

Currently I use a custom class which is inherited from yii\rest\CreateAction class. In the custom class I have overridden run() method entirely.

@mikk150
Copy link
Contributor

mikk150 commented May 2, 2017

I think you should do different models from common for REST api afterall

gives you more control, and there you can do pretty much whatever you want without hacking Controller

@Vovan-VE
Copy link
Contributor

Why not to implement the feature as some extension points (callbacks and/or overrideable methods) to let framework components (REST actions) be more flexible? Let end developer to decied how to use it.

Are PRs allowed?

@yii-bot
Copy link

yii-bot commented Apr 5, 2018

Issue moved to yiisoft/yii-api#6

@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

9 participants