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

[5.2] Add method to implement multiple scopes with variable names #14049

Merged
merged 1 commit into from
Jun 19, 2016

Conversation

sileence
Copy link
Contributor

@sileence sileence commented Jun 18, 2016

Proposal to add the method scopes to allow the implementation of multiple variable scopes at once, for example:

Post::scopes(['question', 'pending' => true])->get() is the same as:

Post::questions()->pending(true)->get()

@JosephSilber
Copy link
Member

I don't see how using an array is in any way better than calling them directly.

$builder = $this;

foreach ($scopes as $scope => $parameters) {
if (is_numeric($scope)) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe this should be is_int?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sure

@sileence
Copy link
Contributor Author

sileence commented Jun 19, 2016

@JosephSilber

I have something like this in my code:

        $scopes = $this->getListScopes($request);

        $posts = Post::scopes($scopes)->paginate();

Based on the request I determine the dynamic scope(s) to filter the posts including its parameters. Doing the same with chained methods is not that easy and don't look that nice.

@JosephSilber
Copy link
Member

Then this is something very unique to your app, and you should keep it there.

Doesn't make much sense in the framework IMO.

@GrahamCampbell
Copy link
Member

Hmmm, I think it could be quite useful. Could make for nicer design rather than making method calls using variables as method names. I'm 👍 here.

@taylorotwell taylorotwell merged commit c869dbd into laravel:5.2 Jun 19, 2016
@taylorotwell
Copy link
Member

Can you send a PR for a test for this?

$parameters = [];
}

$builder = $builder->callScope('scope'.ucfirst($scope), (array) $parameters);
Copy link
Contributor

@acasar acasar Jun 20, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sileence This line doesn't work. callScope needs callable type as the first argument, so it should be:

$callable = [$this->model, 'scope'.ucfirst($scope)];

$builder->callScope($callable, (array) $parameters)

Also, scope can return anything, not necessarily a builder instance, so I wouldn't re-assign it here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

working on that.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants