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 merge() function to ValidatedInput #38640

Merged
merged 5 commits into from
Sep 2, 2021
Merged

[8.x] Add merge() function to ValidatedInput #38640

merged 5 commits into from
Sep 2, 2021

Conversation

HajMo
Copy link
Contributor

@HajMo HajMo commented Sep 1, 2021

This PR will add the ability to merge items to validated inputs. As an example

    $validator = Validator::make(['name' => 'Taylor'], ['name' => 'required']);

    $validator->safe()->merge(['role' => 'Admin']);

    User::create($validator->safe()->all());

Previously you can't do this when using Form Request, you can't merge any data to the returned data from validated().

@HajMo HajMo changed the title Add merge function [8.x] Add merge() function to ValidatedInput Sep 1, 2021
@driesvints
Copy link
Member

I don't think this is wanted. The point of ValidatedInput is that it's data that's been validated and data you merge in may not be. I realize __set and offsetSet are defined on ValidatedInput as well but the later needs to be implemented to offer ArrayAccess support.

@garygreen
Copy link
Contributor

garygreen commented Sep 2, 2021

I realize __set and offsetSet are defined on ValidatedInput as well but the later needs to be implemented to offer ArrayAccess support.

If this class is intended to be immutable then shouldn't the set method throw an exception rather than trying to set the value? That way it's still complies with ArrayAccess but keeps with the intended design goal.

@driesvints
Copy link
Member

If this class is intended to be immutable then shouldn't the set method throw an exception rather than trying to set the value?

@garygreen I agree. I don't know though why it was added.

@taylorotwell
Copy link
Member

I honestly don't mind this that much... sometimes you may need to throw a few default values into the validated data if you are going to pass it straight to a model creation method.

@driesvints
Copy link
Member

No worries @taylorotwell. Was just wondering if you intended it to be mutable.

@driesvints
Copy link
Member

Just a thought: would it be better if this method was used that a new non-ValidatedInput object was returned? Like a collection instance?

@taylorotwell
Copy link
Member

@driesvints then Eloquent would no longer be able to use it for mass assignment.

@taylorotwell taylorotwell merged commit 0a38fd6 into laravel:8.x Sep 2, 2021
@garygreen
Copy link
Contributor

It's kind of a shame this class doesn't have a clearer design goal - it's essentially just a stripped back collection but called ValidatedInput instead. Maybe it could have just extended the collection instance? 🤷‍♀️

@HajMo HajMo deleted the add-merge-function branch September 2, 2021 18:09
victorvilella pushed a commit to cdsistemas/framework that referenced this pull request Oct 12, 2021
* Add `merge()` function

* Add test for `merge()` function

* Fix the test

* Update ValidatedInput.php

* Update ValidatedInputTest.php

Co-authored-by: Taylor Otwell <[email protected]>
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