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

[6.x] Allow callback scopes on Query Builder #30299

Closed
wants to merge 2 commits into from

Conversation

deleugpn
Copy link
Contributor

@deleugpn deleugpn commented Oct 16, 2019

Laravel Query Builder allow for some pretty dynamic code execution. We can pass a callback to select, addSelect, where, join and so on. However, we cannot pass a callback/closure to groupBy.
It's been a while since I've wanted to apply any dynamic scope to Query Builder and this time, instead of contributing just to groupBy I thought I'd give it a shot.

How it works

DB::table('my-table')->apply(new MyDesiredScope)->get();

This will send the Query Builder to the callable method (if MyDesiredScope is callable) and let it mutate the Query Builder. I've been doing a lot of this with where already and it works great:

Post::newQuery()->where(new PostsWithNewReplies);

I just had the need to do that at the groupBy clause instead and noticed I couldn't, hence the contribution to a broader audience.

Signature

apply will accept a single Scope, multiple arguments of scope or an array of Scopes. all cases are covered by tests.

Docs: laravel/docs#5533

@deleugpn deleugpn changed the title Allow callback scopes on Query Builder [6.x] Allow callback scopes on Query Builder Oct 16, 2019

public function testApplyInvokableScope()
{
$result = DB::table('posts')->apply(new PostFrom2017Scope)->get();
Copy link
Contributor

Choose a reason for hiding this comment

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

I think CreatedIn2017Scope is more suitable? or What about this?

$result = DB::table('posts')->apply((new CreatedInYearScope(2017)))->get();

Copy link
Contributor Author

@deleugpn deleugpn Oct 16, 2019

Choose a reason for hiding this comment

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

I don't think that test class is relevant

@deleugpn
Copy link
Contributor Author

I just learned about $builder->tap() and it covers most of what this PR tries to cover and is 🔥 .
I think we just need to document tap() as I couldn't find it on the docs. I'll follow up on that.

@taylorotwell
Copy link
Member

OK, can you PR a documentation update for tap instead? It does seem to do basically what you want.

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.

None yet

3 participants