Attach query behavior in NgRestModel::find

[boehsermoe]

What are you changing/introducing

In luyadev/luya-module-cms/pull/274 I need a easy and generic way to filter models by a attribute e.g. with a trait, interface, behavior.
In this case I want to filter a model by website_id of the current website.

In NgRestModel::find will now attach behaviors to NgRestActiveQuery. Query behaviors can be declare by override the NgRestModel::findBehaviors.

What is the reason for changing/introducing

With a trait there can be conflicts with the \luya\admin\traits\SoftDeleteTrait which also override the ngRestFind and find function and this conflict have to solved by the developer it self.

QA

Q	A
Is bugfix?	no
New feature?	yes
Breaks BC?	no
Tests pass?	yes

[nadar]

In general looks good to me. We are on patch track but this won't hurt. Please add more phpdocs as mentioned, afterwards i will merge.

thanks for the work 👍

[nadar]

I reviewed in terms of code, which is possible and makes sense. But then i checked your description text again, and it just came into my mind:

When you say In this case I want to filter a model by website_id of the current website. why don't you just do andWhere(['website_id' => xyz'])? And if you want to have a more convenient way with an extend active query class like

public function filterByWebsite($id)
{
    return $this->andWhere(['website_id' => $id]);
}

Then we should create a new active query class for this model.

Whats the benefit if your implementation compared to my solutions above?

[boehsermoe]

My intention was to give developers an easy way to automatically filter a model by the website without calling explicit a function.
With a new query class the filter function should called on each find() call.

What's why I prefer to use behavior and declare behaviours already known from yii2.

[nadar]

My intention was to give developers an easy way to automatically filter a model by the website without calling explicit a function.

@boehsermoe Could you then please show me the behavior you would create for this situation? When you need to create a behavior why not either create a custom active query class?

Why should i do a behavior with a function and attach to the query when i could create the same function directly as the active query? Or maybe i don't get your use case then :/

[nadar]

Just to be clear, i am not against but currently i don't see the use case: why should i do:

class MySuperBehavior extends yii\base\Behavior
{
    public function actionFooBar()
    {
        return $this->andWhere(['foo' => 'bar']);
    }
}

when i could make:

class MySuperActiveQuery extends yii\db\ActiveQuery
{
    public function actionFooBar()
    {
        return $this->andWhere(['foo' => 'bar']);
    }
}

The extending of the ActiveQuery event provides full PHP SDK support, while behaviors don't.

[boehsermoe]

In the case of a behavior I use the event and can use multiple behaviours without override them. And I can provide some standard behaviors which only need to add to the model.

class MySuperBehavior extends yii\base\Behavior
{
    public function actionFooBar()
    {
        return $this->andWhere(['foo' => 'bar']);
    }
}

class MyCutomModel extends NgrestModel {
    public function findBehaviors() {
        return ['MySuperBehavior' => MySuperBehavior::class];
    }
}

In the case of a new Query class I also have to initialise the new query class in my custom model and call the function explizit.

class MySuperActiveQuery extends yii\db\ActiveQuery
{
    public function actionFooBar()
    {
        return $this->andWhere(['foo' => 'bar']);
    }
}

class MyCutomModel extends NgrestModel {
    public function find() {
        return (new MySuperActiveQuery())->actionFooBar();
    }
}

[nadar]

In terms of standard behaviors, i agree. So, since we have now shown the pros/contra (no IDE support) we should maybe provide this information in the phpdoc block as it could be confusing WHEN to use which method. :-)

[boehsermoe]

Other variation could be

class MyCutomModel extends NgrestModel {
    public static function filterWhere()
    {
        return [
            'foo' => 'bar'
        ];
    }
}

class NgrestModel {
    public static function find()
    {
           $query = new NgRestActiveQuery(get_called_class();
           $query->andWhere(static::filterWhere());
           return $query
    }
}

[nadar]

I don't see any benefits from this, then better add this informations directly to the find() or ngRestFind() method. I still believe the best solution is to create a custom active query:

• best practice regarding yii docs
• full IDE support (! which is, in my opinion, very important)
• no extra sugar/magic

The behavior you have proposed with findBehaviors() makes sense if there are standardized behaviors we can attach, like SoftDelete f.e. but not if you like to have additional where conditions (or any other sql statements) which are only works in this model.

The use case with website_id is custom and works only for this specific model, therefore i would recommend to create an WebsiteActiveQuery class along with the model: https://www.yiiframework.com/doc/guide/2.0/en/db-active-record#customizing-query-classes

[boehsermoe]

However, I want to create a standardized behavior for website filtering that can be used in several custom models. That's why I thought of behaviors. Maybe there came other behaviors for filtering like language eg.

[nadar]

I have updated the phpdoc. Maybe we should use findActiveQueryBehaviors() (or activeQueryBehaviors()) instead of findBehaviors(). What do you think? It would be more clear that the behavior is attached to the active query class.

[boehsermoe]

findActiveQueryBehaviors sound good for me. Because its only attache to the find() method. Maybe there could came ngRestFindActiveQueryBehaviors method also for ngRestFind()

[codeclimate]

Code Climate has analyzed commit 0e07e2f and detected 0 issues on this pull request.

The test coverage on the diff in this pull request is 100.0% (50% is the threshold).

This pull request will bring the total coverage in the repository to 46.9% (0.0% change).

View more on Code Climate.