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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

[7.x] Add when() method to InteractsWithInput trait #33829

Merged
merged 4 commits into from Aug 14, 2020
Merged

[7.x] Add when() method to InteractsWithInput trait #33829

merged 4 commits into from Aug 14, 2020

Conversation

pascalbaljet
Copy link
Contributor

@pascalbaljet pascalbaljet commented Aug 11, 2020

This PR adds a when method to the InteractsWithInput trait. It's inspired by the when method of the Collection class. The method checks if the request contains the given input item key, and then calls the callback with the input value.

$request->when('city', function($city) {
    //
});

The method returns the Request instance itself so you can easily chain it. One use-case is to apply scopes and constraints to a Query Builder or a similar repository:

$request->when('city', function ($city) {
    $this->feed->city($city);
})->when('minimum_price', function ($minimumPrice) {
    $this->feed->minimumPrice($minimumPrice);
})->when('maximum_price', function ($maximumPrice) {
    $this->feed->maximumPrice($maximumPrice);
});

This is even prettier with the arrow functions introduced in PHP 7.4 馃槂

$request->instance()
    ->when('city', fn ($city) => $feed->city($city))
    ->when('minimum_price', fn ($minimumPrice) => $feed->minimumPrice($minimumPrice))
    ->when('maximum_price', fn ($maximumPrice) => $feed->maximumPrice($maximumPrice));

@rodrigopedra
Copy link
Contributor

rodrigopedra commented Aug 11, 2020

Like the idea, but wonder if it would better to name the method ->whenHas() instead of when() to make the intent clearer.

For instance ->when() in a collection and in the query builder expect a truthy value as their first parameter.

->whenHas() would resemble the query builder's ->whereHas() which expects a name of a relation.

Just my two cents, overall I can see a lot of simplification on my code base if this PR gets merged.

@GrahamCampbell GrahamCampbell changed the title [7.x] Add when() method to InteractsWithInput trait. [7.x] Add when() method to InteractsWithInput trait Aug 11, 2020
@derekmd
Copy link
Contributor

derekmd commented Aug 12, 2020

Triggering the callback when Request::has() is true instead of Request::filled(), the docs will have to be clear this considers the parameter existing on the payload but a value may not be present. The framework's other when() methods (Collection::when(), query Builder::when(), JsonResource@when(), etc.) all consider "truthiness" of the passed value when choosing to invoke the callback.

Some examples to consider:

  1. GET query parameters ?is_published=&foo=bar

     request()->when('is_published', fn () => doSomePublishAction())

    triggers that function when the parameter's value is null (assuming ConvertEmptyStringsToNull middleware is enabled.)

  2. POST <input type="text" name="title" value="">

     request()->when('title', fn () => saveOldTitle())

    Leaving the field empty also triggers the arrow function on a null 'title' value.

However still using Request::has() I can see this being useful for implementing RESTful PATCH endpoints that handle partial model updates. e.g., that may clear a field so you would be sending null values to the database.

@pascalbaljet
Copy link
Contributor Author

pascalbaljet commented Aug 12, 2020

Maybe we could introduce two new methods: whenHas() and whenFilled().

@taylorotwell
Copy link
Member

taylorotwell commented Aug 12, 2020

Yeah I think we would maybe want two methods here for completeness.

@pascalbaljet
Copy link
Contributor Author

pascalbaljet commented Aug 12, 2020

I've updated the PR with a whenHas() and whenFilled() method.

@taylorotwell taylorotwell merged commit 3a518c7 into laravel:7.x Aug 14, 2020
10 checks passed
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

4 participants