Remove ability to declare scopes in ActiveRecord, leave it to ActiveQuery

[samdark]

Pros:

• One proper way to define scopes.
• Thinner models.
• Finder is properly separated from model.
• Faster, no magic.
• Proper IDE autocomplete.
• Better API for declaring scopes, solves Scope declaration: remove $query from method signature? #2013

Cons:

• In order to use custom scopes you need ActiveQuery class for a model.
• Not familiar to 1.1 users.

Examples:

class UserQuery extends ActiveQuery
{
  public function active()
  {
    $this->andWhere(['active' => 1]);
    return $this;
  }

  public function age($years)
  {
    $this->andWhere(['age' => $years]);
    return $this;
  }
}

class User extends ActiveRecord
{
  public static function createQuery()
  {
    return new UserQuery(['modelClass' => get_called_class()]);
  }
}

$users = User::find()->active()->age(25)->all();

[creocoder]

@samdark Absollute agree! 👍

[Ragazzo]

Vote for this too, but with some adjustments:

1. In order to use custom scopes you need ActiveQuery class for a model. I think that link to the AR model should be in query-finder.
2. It should be noted in docs that users should not create about a 1000 custom queries per model, so we will not get solutions like with big numbers of scenarios. This should be officially noted.

[tonydspaniard]

@samdark

Makes a lot of sense, since we propose already separation for searching rules. This approach will isolate searching from manipulation, which will lie on Models.

I vote for it!

Edit:

Not familiar to 1.1 users.

Thats not actually a con since Yii2 already force users to adopt new standards

[samdark]

@Ragazzo

1. There's no AR instance yet at the moment Query is created and used.
2. Absolutely agree. Guide will be updated.

[yupe]

just my +1 for this issue =)

[Ragazzo]

@samdark right, sorry for point 1. :)

[slavcodev]

Finally, less than a year you have decided all the same it would be better :)

But what about the opinion that in many cases will not have to make a second class UserQuery, but only one User?

[samdark]

@slavcodev sure. You don't need UserQuery if you aren't adding custom scopes. Built-in ActiveQuery methods are more than enough for day to day use so it is likely that UserQuery will never be created.

[creocoder]

But what about the opinion that in many cases will not have to make a second class UserQuery, but only one User?

You sure? You will have second class in 80% cases in real word app if you use behaviors, etc.

[miksir]

You don't need UserQuery if you aren't adding custom scopes.

And if you don't need IDE autocomplete. So, you need UserQuery.

[qiangxue]

It was added mainly for practical reason because you may not want to take the trouble to write new query classes. I'm fine removing it if people don't like this convenience at all.

[tanakahisateru]

How about to add a method to ActiveQuery:

User::find()->scopeBy('active')->...
or
User::find()->scopeBy(['User', 'active'])->...

which can call any query modifier callback. They are can be in any place -- custom query class, inline closure or static method.

[samdark]

@tanakahisateru that's what we're trying to avoid: scope methods scattered all over the place.

[tanakahisateru]

I totally agree to remove __call from ActiveQuery because it returns mixed and breaks IDE support for method chain.

[tanakahisateru]

@samdark I'm afraid that beginners create fat controller having model knowledges instead of creating custom query.

[samdark]

@tanakahisateru it will be clearly stated in the guide.

[lubosdz]

Do I understand correctly? :
if using scopes, to each model [UserModel] must exist one extra class [UserQuery] with defined scopes?
If so, it doubles the number of classes/files, right?

It would be great, if all such a custom scopes could be maintained at one single place (e.g. shared class MyScopes extends ActiveQuery) and then if any model could read from it... But I am afraid this may not be possible.

E.g.
$users = User::find()->active()->age(25)->all();
$dogs = Dog::find()->active()->color(array("big", "black"))->all();

In some of my projects I have 20-30 models, out of which 30-40% use scopes.

But even if not possible to place into a single class, I prone to give +1 vote due to clearer design. Performance impact is probably not that big issue even if sticking to 1.1 model.

[tonydspaniard]

If so, it doubles the number of classes/files

I don't see a major problem with that. I particularly like the fact that selection and manipulation are completely separated.

[samdark]

@lubosdz

if using scopes, to each model [UserModel] must exist one extra class [UserQuery] with defined scopes?

Yes.

If so, it doubles the number of classes/files, right?

Only classes you have scopes in.

It would be great, if all such a custom scopes could be maintained at one single place (e.g. shared class MyScopes extends ActiveQuery) and then if any model could read from it... But I am afraid this may not be possible.

It is possible technically by returning the same query class from createQuery.

[lucianobaraglia]

This would be really great!!!
Just like Symfony 1 Doctrine table classes and Propel peer classes...
GII could create them in other directory, like models/query/...

👍

[tanakahisateru]

It should not be double. I will do if scope removed:

class MyExtraActiveQuery extends ActiveQuery
{
  public function modify($callback)
  {
    $callback($this);
    return $this;
  }
}

class MyExtraActiveRecord extends ActiveRecord
{
  public static function createQuery()
  {
    return new MyExtraActiveQuery(['modelClass' => get_called_class()]);
  }
}

then

class User extends MyExtraActiveRecord
{
    public static active($query)
    {
        // do something
        return $q;
    }
}

User::find()->modify(function($q){ User::active($q); })->all();

or

User::active(User::find())->all();

To add this modify() to standard class is my proposal.

[samdark]

@tanakahisateru User::active(User::find())->all(); will works w/o any additions. Nice try. Why you're so strong about keeping these methods in a model?

[qiangxue]

Seems everyone is excited about this. I want to point out the following facts:

• The current implementation already supports the separation of AR and ActiveQuery and defining scope methods in ActiveQuery classes. All the benefits discussed here are already in the current implementation.
• We are here discussing about whether to drop the support of defining scopes in AR classes. By making the proposed change, you lose the possibility of defining scopes in AR classes. I'm not saying this is good or bad, I just wanted to remind everyone the consequence.

[tanakahisateru]

Oh if it would be supported by Gii, there are no problem!

[lubosdz]

Only one proper way should be implemented - I dont see point to keep old (deprecated) approach from 1.1.

[lucianobaraglia]

@qiangxue

We are here discussing about whether to drop the support of defining scopes in AR classes. By making the proposed change, you lose the possibility of defining scopes in AR classes. I'm not saying this is good or bad, I just wanted to remind everyone the consequence.

What is the real consequence?
How the scopes should be called from now? Something like this: ?

User::query()->active()->olderThan(30);

?

[qiangxue]

The consequence is that you lose the capability of defining scope methods in AR class, and you are forced to subclass ActiveQuery and put scope methods there (note that this is already supported currently).

[alanwillms]

I like the idea of having a separate class to deal with scopes, but I think there should be a way to shorten the calling to eliminate the need to call find() method:

User::find()->active()->all(); // currently
User::active()->all();

User::find()->olderThan(21)->all(); // currently
User::olderThan(21)->all();

User::find()->where(['email' => 'x@y.com'])->all(); // currently
User::where(['email' => 'x@y.com'])->all();

[lucianobaraglia]

Well...putting scopes in the Query class will make code cleaner, so I see as a real benefit.
That from the point of view of a simple web developer (the thing I am)...

[qiangxue]

@lucianobaraglia The current implementation already lets you do this. The question is whether we should always force this.

[SonicGD]

Scopes without proper IDE support are not very usable, so i vote for this changes. There is no problem to have second class for model.

[nkostadinov]

Well, I guess this is the right thing 👍 , after all the name ActiveRecord is a representation of a record in a DB. So it makes no sense that this record should hold scopes used for searching other records of its kind :) it breaks the separation of concerns principle.

[tonydspaniard]

@qiangxue

The consequence is that you lose the capability of defining scope methods in AR class, and you are forced to subclass ActiveQuery and put scope methods there (note that this is already supported currently).

Wouldn't this rule force coders to follow better coding approach by separating logic? When I first met Yii was impressed by its flexibility and freedom to do the same thing in many ways, but afterwards when we grow as a company, the same freedom lead us to see horrible coding pitfalls from employees. Restricting this freedom a little won't hurt anybody.

[lucianobaraglia]

@samdark then the Query class won't be used only for custom scopes but also for all finding methods...so chances are you will need the class at one time or another...
I repeat, I would prefer some empty classes than not classes at all...

[creocoder]

@lucianobaraglia

then the Query class won't be used only for custom scopes but also for all finding methods..

Why you assume that?

[lucianobaraglia]

@creocoder because is the convention in this way of separating classes...
Just as @phreakbg said #2016 (comment)...
ActiveRecord class is mainly for operating in a single object/row, while ActiveQuery class is for table related methods (let's say finding, scoping, etc)...

Well, this is the way I worked with Symfony/Doctrine...

[cebe]

Why drop a feature that may be used? When I only need one scope, why should I create separate class. Scopes in Query class are already supported so we should state arguments in another way.

See #2016 (comment)

The question is, why we should drop a feature as the requested feature is already supported.
What are the cons of allowing scopes in AR class? I currently do not see any as we can document both ways and may also stress the "better" approach a bit more. But do we really need to drop support for having scopes in AR? I am for keeping it.

[cebe]

Btw: one of yii's strength is to keep things simple and easy. If you like the more ideological way of separating responsibilities that's fine and you are free to choose in this case.

[samdark]

@cebe some cons of keeping both:

1. There's no single place where you can look for scopes available.
2. Method signature and structure is different in ActiveRecord and ActiveQuery:

// ActiveRecord
public static function ageOf($query, $age)
{
    $query->addWhere('age', 42);
}

// ActiveQuery
public function ageOf($age)
{
    $this->addWhere('age', 42);
    return $this;
}

Confusing, isn't it?

[lucianobaraglia]

@cebe I really love what you mean about YII, that's why I ran away from other bloated frameworks...

But in this point, I think it enforces a very good practice what will be very valuable when several developers with very different skill level will work in the same project in a long time span...
And exactly as @samdark says...having a single place to look for these methods is better...all developers should define them in that class...

[samdark]

I won't merge it for a couple of days at least. Need more opinions.

[cebe]

okay, different signatures are a big argument. I'm quite convinced now.

[andersonamuller]

👍 for dropping the scopes in the ActiveRecord.

Totally agree with @tonydspaniard in #2016 (comment)

[o-rey]

Here's possible trouble.

Sometimes it's convenient to define 'global' scopes (that is, defined in parent class), like active(), visible() and so on.
If join() is also involved, this can cause 'column ambigous' error, so we need to use table name also.
Now I'm doing it like this:

$query->andWhere(static::tableName() . '.is_disabled = 1');

If scopes are moving to AQ, there will be no access to tableName(), since it's an AR method.
How this issue can be solved?

[slavcodev]

@o-rey maybe

$this->andWhere($this->modelClass::tableName() . '.is_disabled = 1');

[o-rey]

@slavcodev Yes, totally missed this one. Thx.

[lubosdz]

Will following inheritance work?

ActiveQuery -> SharedActiveQuery -> DogModelActiveQuery
ActiveQuery -> SharedActiveQuery -> CatModelActiveQuery

If so, then it should be no problem to keep any scopes in a shared file.

I see 3 reasons why users should be left with an option to keep scopes in a single file, if they want to:

1. lower maintenance costs
2. reusing shared scopes (avoiding duplicates - e.g. tables "comments", "payments", "exports" may use the same scope for finding items older than 3 days).
3. parsing overhead (rather than parsing 20 files with 5 lines of code parse 1 file with 100 lines of code)

Summing up - dropping Yii1 way about scopes within a model is OK but an option to keep scopes at one place in Yii2 would be very welcome.

[samdark]

@lubosdz yes. It will work. Noone denies scopes in a single file.

[nizsheanez]

doesn't work for me with hasOne method.
i try to do
$this->hasOne(Report::className(), ['fk_goal' => 'id'])->myScope()->one();
have exception
Calling unknown method: yii\db\ActiveRelation::myScope()
I must to redeclare createActiveRelation ?

[samdark]

@nizsheanez that's a valid issue. Can you create a separate ticket? Thanks.

[nizsheanez]

sure