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

[8.x] Add a mergeIfMissing method to the Illuminate Http Request class #40116

Merged
merged 4 commits into from
Dec 22, 2021

Conversation

davidpeach
Copy link
Contributor

When unticking a checkbox and submitting my form, no value for that field is sent to the server. I couldn't guarantee that the column actually wanted to be turned off.

So I added a little check to merge in the "off" state when that key was missing from the request.

Thought it might make a nice little addition.

Before:

if ($request->missing('boolean_setting')) {
    $request->merge(['boolean_setting' => 0]);
}

After:

$request->mergeIfMissing(['boolean_setting' => 0])

@davidpeach davidpeach changed the title Add a mergeIfMissing method to the Illuminate Http Request class. [8.x] Add a mergeIfMissing method to the Illuminate Http Request class. Dec 21, 2021
@JustSteveKing
Copy link

Love this idea!

@GrahamCampbell GrahamCampbell changed the title [8.x] Add a mergeIfMissing method to the Illuminate Http Request class. [8.x] Add a mergeIfMissing method to the Illuminate Http Request class Dec 21, 2021
@taylorotwell
Copy link
Member

taylorotwell commented Dec 21, 2021

I'm a little concerned there is some subtle unexpected behavior here maybe.

You call getInputSource()->has(...); however, the missing method itself just calls has on the request, which could have slightly different behavior. Therefore, I'm worried the definition of "missing" is not consistent between the actual missing method and mergeIfMissing.

Is there a reason you did getInputSource()->has(...) instead of just calling has?

I'm marking this as draft until that is answered. Please mark as ready for review when answered.

@taylorotwell taylorotwell marked this pull request as draft December 21, 2021 23:01
@davidpeach
Copy link
Contributor Author

Thanks for the feedback @taylorotwell . Much appreciated.

I see what you mean ye. Not just calling has was an oversight on my part.

I've now reversed the check too, so it uses the missing method in a filter, which makes more sense to me, as its inside a mergeIfMissing method.

Thanks again.

@davidpeach davidpeach marked this pull request as ready for review December 22, 2021 18:59
@taylorotwell taylorotwell merged commit 4afca3c into laravel:8.x Dec 22, 2021
@davidpeach davidpeach deleted the feature/request-merge-if-missing branch January 24, 2022 13:16
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.

3 participants