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

[7.x] Adds higher order "when" proxy #30306

Closed
wants to merge 1 commit into from
Closed

[7.x] Adds higher order "when" proxy #30306

wants to merge 1 commit into from

Conversation

kayrunm
Copy link
Contributor

@kayrunm kayrunm commented Oct 16, 2019

As per the suggestion in laravel/ideas#1881, this PR adds the ability to use higher order messaging when using the when (and unless) functions in query builders/collections.

This would make using the when (and unless) functions a bit cleaner when someone is only adding one conditional method call, e.g.:

// Before...
User::query()
    ->when($condition, function ($query) use ($bar) {
        $query->where('foo', $bar);
    })
    ->when($another, function ($query) use ($qux) {
        $query->where('baz', $quz)
    })
    ->get();

// After...
User::query()
    ->when($condition)->where('foo', $bar)
    ->when($another)->where('baz', $qux)
    ->get();

@kayrunm
Copy link
Contributor Author

kayrunm commented Oct 16, 2019

The two failing tests in the first build (for PHP 7.2) don't seem to be related to these changes, as far as I can tell, as per #29777

@n10000k
Copy link

n10000k commented Oct 16, 2019

#29777

@taylorotwell
Copy link
Member

Thanks for your pull request to Laravel!

Unfortunately, I'm going to delay merging this code for now. To preserve our ability to adequately maintain the framework, we need to be very careful regarding the amount of code we include.

If possible, please consider releasing your code as a package so that the community can still take advantage of your contributions!

If you feel absolutely certain that this code corrects a bug in the framework, please "@" mention me in a follow-up comment with further explanation so that GitHub will send me a notification of your response.

@Arkanius
Copy link
Contributor

That's sad, really sad. @kayrunm could you please consider to create a package with this?

@kayrunm
Copy link
Contributor Author

kayrunm commented Oct 22, 2019

That's sad, really sad. @kayrunm could you please consider to create a package with this?

Given the very limited scope of the package I don't really see the point in creating one given some of the hoops that would have to be jumped through. Sorry, @Arkanius!

The code's there for you to see if you want to do it yourself, though.

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.

4 participants