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

ActiveRelation: support custom Scopes #2146

Closed
nizsheanez opened this issue Jan 24, 2014 · 57 comments
Closed

ActiveRelation: support custom Scopes #2146

nizsheanez opened this issue Jan 24, 2014 · 57 comments

Comments

@nizsheanez
Copy link

@nizsheanez nizsheanez commented Jan 24, 2014

$this->hasOne(Report::className(), ['fk_goal' => 'id'])->myScope()->one();
have exception
Calling unknown method: yii\db\ActiveRelation::myScope()
I must to redeclare createActiveRelation ?

Now i solve i like:

class Report extends ActiveRecord {

    public static function createQuery()
    {
        return new ReportQuery(['modelClass' => get_called_class()]);
    }

    public static function createActiveRelation($config = [])
    {
        return new ReportRelation($config);
    }

}

//scopes
trait ReportScopes
{
    public function myScope() {
        return $this;
    }
}

//apply scopes to query
class ReportQuery extends ActiveQuery
{
    use ReportScopes;
}

//apply scopes to relations
class ReportRelation extends ActiveRelation
{
    use ReportScopes;
}
@samdark
Copy link
Member

@samdark samdark commented Jan 24, 2014

Looks like showstopper for beta release. @yiisoft/core-developers any idea how to solve it?

@qiangxue
Copy link
Member

@qiangxue qiangxue commented Jan 24, 2014

I would say bring back the previous support for declaring scopes in AR classes.

@samdark
Copy link
Member

@samdark samdark commented Jan 24, 2014

And make it the only way to declare scopes?

@qiangxue
Copy link
Member

@qiangxue qiangxue commented Jan 24, 2014

The methods declared in ActiveQuery are just normal methods, nothing special.
The methods declared in ActiveRecord can be called scope methods, which are specially handled.

@nizsheanez
Copy link
Author

@nizsheanez nizsheanez commented Jan 24, 2014

What the case of creating method in ActiveQuery and don't create in ActiveRelation?
Or extends ActiveQuery and don't extends ActiveRelation?

@samdark
Copy link
Member

@samdark samdark commented Jan 24, 2014

@nizsheanez forcing users to create method in ActiveRelation as well makes it 3 extra classes per model that doesn't sound good.

@nizsheanez
Copy link
Author

@nizsheanez nizsheanez commented Jan 24, 2014

@samdark, if don't talk about scopes, if user want to create/redeclare method in ActiveQuery then we will have same situation: method will work after find() and don't after hasOne()?

A bit abstract question, but hard to say clearly.

@samdark
Copy link
Member

@samdark samdark commented Jan 24, 2014

@samdark
Copy link
Member

@samdark samdark commented Jan 24, 2014

@qiangxue I don't like the idea of reverting the change. Scope methods are definitely query-related and not instance ones.

@nizsheanez
Copy link
Author

@nizsheanez nizsheanez commented Jan 24, 2014

@samdark, ok

@Ragazzo
Copy link
Contributor

@Ragazzo Ragazzo commented Jan 24, 2014

Hm, @samdark but i would like to have this feature, extremely flexible, especially when user dont want to specify each time scopes in with in callback. Need to make some workaround here to have this feature. I also was told by other developers that they dont like total removing scopes from AR for this reason:

  1. Scopes are business logic of urrent ar;
  2. If extracting scopes to queries why then not extracting relations there, because they are also about simple extra-query.

Not starting long discussion about should/should not we bring this feature back, just notice.

@qiangxue
Copy link
Member

@qiangxue qiangxue commented Jan 24, 2014

Scope methods are definitely query-related and not instance ones.

True theoretically. The old implementation was all for practical reason like this one. Another example is that for convenience we often put query methods directly in AR, for example, User::findByUsername().

@creocoder
Copy link
Contributor

@creocoder creocoder commented Jan 24, 2014

Another example is that for convenience we often put query methods directly in AR, for example, User::findByUsername().

Agreed. Another examples Category::findAll(), Tag::findAllByName().

@creocoder
Copy link
Contributor

@creocoder creocoder commented Jan 24, 2014

Seems there is only two ways:

  • remove this feature $this->hasOne(Report::className(), ['fk_goal' => 'id'])->myScope()->one();
  • rollback scopes to AR

I'm for first way. [active_relation]->[scope] seems excess. Same things can be achived other ways.

@qiangxue
Copy link
Member

@qiangxue qiangxue commented Jan 24, 2014

Well, it's about code reuse: you use scope because you want to reuse the same condition in multiple places.

@creocoder
Copy link
Contributor

@creocoder creocoder commented Jan 24, 2014

@qiangxue Yes, scopes goal is clean. But can you show example when you'll have any benefits from using scope inside relation definition? I see only drawbacks like "want unclean model? use scopes inside relation definition!" :)

@qiangxue
Copy link
Member

@qiangxue qiangxue commented Jan 24, 2014

I would say if used judiciously, scope methods are very convenient and useful.

A simple example is most content models (e.g. Article, Comment, Post) have a status column. Introducing scope methods such as published() in the AR classes would make it very convenient and consistent when performing relational and non-relational queries (including defining relations).

Yes, you can certainly directly write the condition when you define the relation, but why do you want to repeat? You want to write the condition explicitly in a relation method, but don't want to do so in a scope method? What's the difference?

@creocoder
Copy link
Contributor

@creocoder creocoder commented Jan 24, 2014

@qiangxue

Yes, you can certainly directly write the condition when you define the relation, but why do you want to repeat? You want to write the condition explicitly in a relation method, but don't want to do so in a scope method? What's the difference?

You understand me wrong, i would not write condition/scope/with when defining relation. Never. This way lead to unclean models/excess queries/non-error free models. There is one reason. When you'll need relation without condition/scope/with you'll need to find all places where such "smart" optimization was used and write condition/scope/with explicitly in that places. As result time will be wasted. But i agree scopes is usefull to reuse conditions. I do not say scopes is excess feature. I just say that possibility to use scopes (and many other things) inside relation definition usually bad practice or "too smart" optimization. In reality situations when you can use scope inside relation definition is sooo rare and also you need to be 1000% ensure that this relation will not be needed in other places without that scope in future.

@qiangxue
Copy link
Member

@qiangxue qiangxue commented Jan 24, 2014

So are you opposing to supporting scope method definition in AR classes?

@creocoder
Copy link
Contributor

@creocoder creocoder commented Jan 24, 2014

@qiangxue After @samdark PR we not support scopes in AR classes, only in AQ classes. And i think currently all fine and current issue with ActiveRelation is invalid since approach was changed.

@creocoder
Copy link
Contributor

@creocoder creocoder commented Jan 24, 2014

@qiangxue On other hand class ActiveRelation extends ActiveQuery...

@creocoder
Copy link
Contributor

@creocoder creocoder commented Jan 24, 2014

@qiangxue Also issue starter solution seems not optimal. Optimal solution is:

class Report extends ActiveRecord {

    public static function createQuery()
    {
        return new ReportQuery(['modelClass' => get_called_class()]);
    }

    public static function createActiveRelation($config = [])
    {
        return new ReportRelation($config);
    }

}

class ReportQuery extends ActiveQuery
{
    public function myScope() {
        ...
    }
}

class ReportRelation extends ReportQuery
{
}

So maybe there is no problems at all. Want scopes? Define *Query model. Want use scopes inside relation definition? Define *Relation model extends yours *Query model. Seems clean if take into account that using scopes inside relation definitions really rare.

P.S. Seems createActiveRelation() need to be renamed to createRelation() to be consistent with createQuery() method.

@qiangxue
Copy link
Member

@qiangxue qiangxue commented Jan 24, 2014

Nope, this won't work: ReportRelation extends ReportQuery

@qiangxue
Copy link
Member

@qiangxue qiangxue commented Jan 24, 2014

The issue reported is the complaint about excessive number of query classes needed, which I think is valid.

Scope is not just used in relation definition. You can use it also DURING performing relational queries, e.g., $post->getComments()->active()->all().

@samdark
Copy link
Member

@samdark samdark commented Jan 24, 2014

Losing ability to use scope for relations doesn't look like acceptable loss but just reverting recent change means that there's one fully working and valid approach for scopes and it's magical definition in AR class.

@qiangxue
Copy link
Member

@qiangxue qiangxue commented Jan 24, 2014

Yes, it is magical, and that's why it is called a feature.

Scope methods defined in query classes aren't any special at all. We don't even need to talk about them because they are just normal methods without any special support.

@samdark
Copy link
Member

@samdark samdark commented Jan 24, 2014

Well, it's not expected that these methods are available in one case and not available in another. Currently I see two ways solving it:

  1. Revert removal of scopes from AR. Remove any mentioning of query classes from docs, make AR-level scopes the only official way to do it.
  2. Redesign relations somehow to use query scopes.
@slavcodev
Copy link
Contributor

@slavcodev slavcodev commented Jan 24, 2014

@samdark

Redesign relations somehow to use related model query :)
@qiangxue
Copy link
Member

@qiangxue qiangxue commented Jan 24, 2014

Redesign relations somehow to use query scopes.

This means combining ActiveQuery and ActiveRelation into a single class.

@qiangxue
Copy link
Member

@qiangxue qiangxue commented Jan 24, 2014

Combining ActiveQuery and ActiveRelation is not too bad. The only drawback is ActiveQuery will contain quite some methods/properties that are not used in non-relational queries.

@cebe cebe mentioned this issue Feb 20, 2014
3 of 3 tasks complete
@cebe cebe closed this in #2497 Feb 21, 2014
@cebe
Copy link
Member

@cebe cebe commented Feb 21, 2014

Now we have the current implementation working with relations but @rawtaz has some valid points.
We decided to remove scopes from AR because of the fact that scope methods in AR differ from that in query significantly so this will cause confusion. #2016 (comment)
If there is a better way to have scopes in AR I'd vote to add it back.

@qiangxue
Copy link
Member

@qiangxue qiangxue commented Feb 21, 2014

Let's see if more people are requesting to add it back. If so, we may consider prefixing all scope methods in AR with scope.

@rawtaz
Copy link
Contributor

@rawtaz rawtaz commented Feb 21, 2014

@qiangxue How would one apply those scopes, are you thinking ->scopeForCountry($country) or ->forCountry($country)? The former doesn't make much sense IMO, but I guess you're thinking the latter :-)

@samdark
Copy link
Member

@samdark samdark commented Feb 21, 2014

Magic again?

@qiangxue
Copy link
Member

@qiangxue qiangxue commented Feb 21, 2014

Yes, it's the latter:

class Post extends ActiveRecord
{
    public static function scopeVisible($query)
    {
        $query->andWhere(['visible' => 1]);
    }
}

Post::find()->visible()->all();

Yes, the signature is different, but so do the method name. This is a magical feature, anyway.

@rawtaz
Copy link
Contributor

@rawtaz rawtaz commented Feb 21, 2014

Similar to what I said in my earlier post; Even if one can make scopes like this (in the model, with a scopeFoo() method), people who want to write "cleaner" code are still free to do so (by extending ActiveQuery and overriding createQuery()). Maybe this is a good combination of pragmatism, allowing a practical approach to code, as well as clean structure? I don't think I have anything against it, seems like a good compromise.

I mean, if all one wants to do is add some scopes, then having to extend a class and override a method does feel a bit overkill. Again, usually one just adds a little piece of condition, that's all. Would be different if the scope did much more than that.

@samdark
Copy link
Member

@samdark samdark commented Feb 21, 2014

I think for now we should keep it as is and wait for feedback.

@Ragazzo
Copy link
Contributor

@Ragazzo Ragazzo commented Feb 22, 2014

I am against scope prefix, it is not good, better as it was before - simple method, i also against $query in method signature, i was proposing how to solve it. It is more and more like L4 and Yii2.

@jfragoulis
Copy link
Contributor

@jfragoulis jfragoulis commented Feb 22, 2014

If you go for supporting scopes in AR model, I am 200% positive for going down the scope prefix road.
For me at least, it is most definitely useful to be able to distinguish scope methods from other methods just by looking at the method's name.

@rawtaz
Copy link
Contributor

@rawtaz rawtaz commented Feb 22, 2014

@Ragazzo Sorry, but where did you propose how to solve it / not use $query?

@Ragazzo
Copy link
Contributor

@Ragazzo Ragazzo commented Feb 22, 2014

@rawtaz
Copy link
Contributor

@rawtaz rawtaz commented Feb 22, 2014

It's indeed way nicer without the darn $query in the scope signature. But what matters is how one calls it, it should be ->foo(param1, param2, ...) regardless of the method signature having $query in it or not.

@Ragazzo
Copy link
Contributor

@Ragazzo Ragazzo commented Feb 22, 2014

@rawtaz currently scopes should be defined in query-object if you need scopes, so this problem does not exists currenlty. link to issue was just FYI, and maybe if one will migrate them back to AR.

@Gcaufy
Copy link

@Gcaufy Gcaufy commented Jan 14, 2015

@qiangxue

class Post extends ActiveRecord
{
    public static function scopeVisible($query)
    {
        $query->andWhere(['visible' => 1]);
    }
}
Post::find()->visible()->all();

is this working now in current yii2?

@cebe
Copy link
Member

@cebe cebe commented Jan 14, 2015

no, scopes are only supported in custom query classes.
http://www.yiiframework.com/doc-2.0/guide-db-active-record.html#scopes

@SamMousa
Copy link
Contributor

@SamMousa SamMousa commented Apr 7, 2015

I know this has been discussed in depth, but i'm unclear what's the argument against the example code by @qiangxue ..

This example query basically implements support for this feature without breaking anything else.

    class TestQuery extends ActiveQuery {
        public function __call($name, array $arguments) {
            // Check if this is a scope.
            $method = 'scope' . ucfirst($name);
            if (method_exists($this->modelClass, $method)) {
                array_unshift($arguments, $this);
                $result = forward_static_call_array([$this->modelClass, $method], $arguments);
            } else {
                $result = parent::__call($name, $arguments);
            }
        }
    }

This code has little overhead and as far as I can tell does not break BC. The argument against having different names for scopes in an ActiveQuery class vs scopes in an ActiveRecord class is not valid in my opinion. The added ease of use of this code outweights the "inconsistency" in my opinion.

Obviously I can and do implement this in my base ActiveQuery / ActiveRecord classes, but I think it would improve the framework to include this.

@klimov-paul
Copy link
Member

@klimov-paul klimov-paul commented Apr 7, 2015

This sounds interesting.
However instead of composing method name with prefix 'scope' it is better to check if the method is static using reflection. This can lead us back to original implementation, where static methods of Active Record class serve as a scopes.

@klimov-paul klimov-paul reopened this Apr 7, 2015
@SamMousa
Copy link
Contributor

@SamMousa SamMousa commented Apr 7, 2015

Making all static methods a scope automatically seems bad to me. If we use reflection we could go as far as to check the signature to see if it is a scope. However for me this adds no real benefit.

I like having the scope prefix in my ActiveRecord class and I like not having to use the scope prefix when running the query.

Also there are some static methods that are never scopes (like getDb, createQuery, tableName) I'd rather not have to think about these possible collisions; also for BC the prefix is safer since I can easily verify that any existing static methods don't start with "scope"; it is harder for me to verify that any static methods I have defined will not pass the check for being interpreted as a scope.

What is bad about the prefix method? Is it bad practice?

@klimov-paul
Copy link
Member

@klimov-paul klimov-paul commented Apr 7, 2015

Using the prefix for scopes method introduces special magic. Also not everyone may actually like it.
For example: I prefer to declare scope methods in format 'where...':

public static function whereActive($query)
{
    ...
    return $query;
}

This way scope methods looks pretty both while declaring and while applying.

Also originally (back at 'beta' version) Active Record declared its scopes in this way - just a static methods without any prefix.

@klimov-paul
Copy link
Member

@klimov-paul klimov-paul commented Apr 7, 2015

It is obvious we are running circles around here.
I have reread the entire conversation and found final resolution reference:
#2016 (comment)

Change does not make sense.

@klimov-paul klimov-paul closed this Apr 7, 2015
@SamMousa
Copy link
Contributor

@SamMousa SamMousa commented Apr 7, 2015

Ok, clear on the final arguments. Will consider releasing this as a separate module. Thanks for the input!

@cebe cebe modified the milestones: 2.0 Beta, 2.0.x Apr 11, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Linked pull requests

Successfully merging a pull request may close this issue.

You can’t perform that action at this time.