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

Model query scopes #273

Closed
wants to merge 6 commits into
base: master
from

Conversation

Projects
None yet
@franzliedke
Copy link
Contributor

franzliedke commented Feb 8, 2013

Here comes the implementation for #272.

Unit-tested and very simple. I like it.

@franzliedke

This comment has been minimized.

Copy link
Contributor Author

franzliedke commented Feb 8, 2013

@jasonlewis Apparently you deleted your comment again, but still, what you could do is this if you prefer one-liners:

return $this->scopedQuery()->where('confirmed', 1)->getModel();
@franzliedke

This comment has been minimized.

Copy link
Contributor Author

franzliedke commented Feb 8, 2013

And of course you can use every scope as a query-finalizing statement, too, that actually fetches the result.

@jasonlewis

This comment has been minimized.

Copy link
Contributor

jasonlewis commented Feb 8, 2013

Yeah my original comment wasn't correct because where() would return a query builder object, thus not allowing to you to chain scoped queries. My bad. =)

@kapv89

This comment has been minimized.

Copy link
Contributor

kapv89 commented Feb 8, 2013

really like the implementation 👍

@rmobis

This comment has been minimized.

Copy link
Contributor

rmobis commented Feb 9, 2013

+1, awesome idea

@arvidbjorkstrom

This comment has been minimized.

Copy link
Contributor

arvidbjorkstrom commented Feb 11, 2013

👍 Nice feature!

@franzliedke

This comment has been minimized.

Copy link
Contributor Author

franzliedke commented Feb 14, 2013

Any news on this, @taylorotwell? What do you think?

@taylorotwell

This comment has been minimized.

Copy link
Member

taylorotwell commented Feb 16, 2013

This implementation doesn't look quite right. For example, what if you do this:

$user = new User;
$users = $user->where('foo', '=', 'bar')->get();
$users = $user->where('bar', '=', 'baz')->get();

Since the scoped query is hanging out there kind of as a type of global state the second query I ran looks like it would just keep appending to the first queries where clause instead of having a new query.

@JoostK

This comment has been minimized.

Copy link
Contributor

JoostK commented Feb 16, 2013

It may be sufficient to not call scopedQuery in __call, I don't currently see why it's necessary there (although that would not solve the problem in it's entirety, as there is indeed some global state shared by multiple method chains)

To solve that problem we may need to delegate unavailable methods in Eloquent\Builder first to the original model, passing the query as a parameter:

// Model.php
public function scopedOlderThan(Builder $query, $age)
{
    // return statement is not necessary, return value is ignored
    return $query->where('age', '>', $age);
}

// Eloquent\Builder::__call
if (method_exists($this->model, 'scoped' . Str::camel($method)))
{
    array_unshift($parameters, $this);
    call_user_func_array(array($this->model, $method), $parameters);
    return $this;
}

No hidden 'global state' and an even cleaner syntax. Tell me if I miss something here. I explicitly return $this in the query builder as then the return keyword in the scoped method can be skipped.

@franzliedke

This comment has been minimized.

Copy link
Contributor Author

franzliedke commented Feb 18, 2013

@taylorotwell Are model objects really supposed to be used for multiple queries like that? I've never really seen them being used that way.

@robclancy

This comment has been minimized.

Copy link
Contributor

robclancy commented Feb 19, 2013

I use a repository which has one instance of a model initialized and it is used each time the repository does something. Just as an example of using a model instance multiple times...

@JoostK

This comment has been minimized.

Copy link
Contributor

JoostK commented Feb 19, 2013

The code is still as broken as it was before. In your stub, newQuery returns a mocked Builder and you specify there that it should receive where twice. That's not actually true, the second time it should be returned on a new Builder instance.

A fix would be to use:

protected function resetScopedQuery()
{
    return $this->scopedQuery = $this->newQuery();
}

public function __call($method, $parameters)
{
    $query = $this->resetScopedQuery();

    return call_user_func_array(array($query, $method), $parameters);
}

However this approach still suffers from the global state problem, e.g. you cannot do:

$user = new User;
$q1 = $user->scope1();
$q2 = $user->scope2();

$q1->scope2(); // Will be applied to $q2

May not be used frequently but it's still behavior that you don't expect. My solution given above doesn't suffer from all these problems and is just as simple, if not more simple.

@franzliedke

This comment has been minimized.

Copy link
Contributor Author

franzliedke commented Feb 19, 2013

Yeah, I wasn't done fixing things yet. But I don't really want to get into modifying the builder class - the scoping stuff should clearly remain in the model class in my eyes.

One could say it is by design that scopes affect the state of the model object that you use to build your query. That way, you can pass around model objects as pre-configured queries and allow different constraints to be assembled. Does that make sense?

There's either this simple solution with state (which would have to be documented) or probably no simple solution at all. What do you think, Taylor?

@JoostK

This comment has been minimized.

Copy link
Contributor

JoostK commented Feb 19, 2013

I wonder how it's done in Rails?

Modifying the Eloquent Builder (not the general Builder) to delegate back to the model covers your example of multiple chained scoped methods. Passing objects around is possible just as well but it can be either a Model instance or a Builder instance that's passed around.

@taylorotwell

This comment has been minimized.

Copy link
Member

taylorotwell commented Mar 20, 2013

Can you remind me what this allows you to do again?

@rmobis

This comment has been minimized.

Copy link
Contributor

rmobis commented Mar 20, 2013

I believe the idea is to allow you to chain custom query commands. For instance, if I had a Student model, and I created approved() and minor() to add specific where clauses, I'd be able to chain them, like this:

// Get all students that passed the exam and are above 18 years old 
Student::approved()->minor()->get();
@franzliedke

This comment has been minimized.

Copy link
Contributor Author

franzliedke commented Mar 20, 2013

It is a little complicated, due to the state we introduce to model instances.

If you think that's not worth it, you can close this issue, Taylor...

@Anahkiasen

This comment has been minimized.

Copy link
Contributor

Anahkiasen commented Mar 20, 2013

I hope not, this would be an amazing feature, and one I've hopelessely tried to reproduce in L3.

@rmobis

This comment has been minimized.

Copy link
Contributor

rmobis commented Mar 20, 2013

Ye, having this feature would be awesome.

@franzliedke

This comment has been minimized.

Copy link
Contributor Author

franzliedke commented Mar 20, 2013

Well, this implementation has one shortcoming, but that could be documented as intended. Or maybe Taylor has a better idea?

@taylorotwell

This comment has been minimized.

Copy link
Member

taylorotwell commented Mar 21, 2013

Scopes have been implemented. The scoping logic (all 2 lines of it) does belong on the Builder as scoping is really just a query builder mechanism in that it limits the scope of a given query. Anyways, prefix a method on your model with "scope":

class User extends Eloquent {

     public function scopeApproved($query)
     {
          $query->where('approved', 1);
     }

    public function scopePopular($query)
    {
          $query->where('votes', '>', 100);
     }

}

You can then query the scope:

$users = User::approved()->popular()->get();

You can also chain scopes, etc.

@helmut

This comment has been minimized.

Copy link
Contributor

helmut commented Mar 21, 2013

That is lovely! Is it possible to be able to pass in params to the scoped query?

For example...

class User extends Eloquent {

     public function scopeApproved($query)
     {
          $query->where('approved', 1);
     }

    public function scopePopular($query, $minimum = 100)
    {
          $query->where('votes', '>', $minimum);
     }

}

So you can then query the scope:

$users = User::approved()->popular(50)->get();
@mannysoft

This comment has been minimized.

Copy link

mannysoft commented Mar 21, 2013

Cool! :-)

@PhiloNL

This comment has been minimized.

Copy link
Contributor

PhiloNL commented Mar 21, 2013

Awesome 👍

@agentphoenix

This comment has been minimized.

Copy link

agentphoenix commented Mar 21, 2013

👍

@jcwatson11

This comment has been minimized.

Copy link

jcwatson11 commented Mar 21, 2013

Small code change. Giant leap forward in usability.

@jackmcdade

This comment has been minimized.

Copy link

jackmcdade commented Mar 21, 2013

👍

@productick

This comment has been minimized.

Copy link

productick commented Mar 21, 2013

sweets!!!

@drsii

This comment has been minimized.

Copy link

drsii commented Mar 21, 2013

So good!

@gauravtiwari

This comment has been minimized.

Copy link

gauravtiwari commented Mar 22, 2013

This is Great 👍 Thanks

@maxhoffmann

This comment has been minimized.

Copy link

maxhoffmann commented Mar 22, 2013

Great feature! 👍

Is there any way to pass a variable to nested where clauses in scopes?

Example:

// Model
public function scopeSearch($query, $q)
{
    $query->where(function($query) {
        $query->where('title', 'LIKE', '%' . $q . '%');
        $query->orWhere('text', 'LIKE', '%' . $q . '%');
    });
}

//Controller
$results = Data::search( Input::get('q') )->get();
@Anahkiasen

This comment has been minimized.

Copy link
Contributor

Anahkiasen commented Mar 22, 2013

function($query) use ($q)

@maxhoffmann

This comment has been minimized.

Copy link

maxhoffmann commented Mar 22, 2013

Awesome! Thanks @Anahkiasen.

@Fuhrmann

This comment has been minimized.

Copy link
Contributor

Fuhrmann commented Mar 22, 2013

👍

@Spir

This comment has been minimized.

Copy link

Spir commented Mar 22, 2013

You can also use other join/select and other grammar. This is very helpful.

@jonathanmarvens

This comment has been minimized.

Copy link

jonathanmarvens commented Mar 23, 2013

This is great!

@duellsy

This comment has been minimized.

Copy link
Contributor

duellsy commented Mar 26, 2013

this makes my balls tingle

@niallobrien

This comment has been minimized.

Copy link

niallobrien commented Mar 26, 2013

This is a great feature, I'm implementing soft-deletes and this will help enormously! Thanks @taylorotwell

@taylorotwell

This comment has been minimized.

Copy link
Member

taylorotwell commented Mar 26, 2013

No problem!

@micaweb

This comment has been minimized.

Copy link

micaweb commented Apr 4, 2013

Is it possible to have a default scope which will be triggered in all methods?

@micaweb

This comment has been minimized.

Copy link

micaweb commented Apr 7, 2013

For the moment I believe I could override newQuery in the model, as I saw it at http://usman.it/filter-eloquent-results-overriding-laravel/

@ghprod

This comment has been minimized.

Copy link

ghprod commented Jul 19, 2013

I'm try to use it in L4, but no luck, give me this warning

Missing argument 1 for Post::scopePublished()

@rmobis

This comment has been minimized.

Copy link
Contributor

rmobis commented Jul 19, 2013

@ghprod , you should be calling it as Post::published()

@ghprod

This comment has been minimized.

Copy link

ghprod commented Jul 19, 2013

@rmobis , yes i've called like that, but Whoops give me that error message .. so i know, scope i active, but why it could load $query ?

@PhiloNL

This comment has been minimized.

Copy link
Contributor

PhiloNL commented Jul 19, 2013

Could you paste the code of your scope method?

@ghprod

This comment has been minimized.

Copy link

ghprod commented Jul 19, 2013

Hi, sorry, now its fix, its all because i call published() from another function as chain :)
Thanks all

@rmobis

This comment has been minimized.

Copy link
Contributor

rmobis commented Jul 19, 2013

Should be working when chained.

@jasonlewis

This comment has been minimized.

Copy link
Contributor

jasonlewis commented Jul 20, 2013

Please use the forums if you need help.

@laravel laravel locked and limited conversation to collaborators Sep 26, 2014

@franzliedke franzliedke deleted the franzliedke-pulls:patch-1 branch May 9, 2017

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.