8000 Validator::valid() returning unvalidated fields · Issue #55755 · laravel/framework · GitHub
[go: up one dir, main page]
Skip to content

Validator::valid() returning unvalidated fields #55755

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.

Sign up for GitHub

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

Jump to bottom
Closed
8000
bluesheep100 opened this issue May 16, 2025 · 6 comments
Closed

Validator::valid() returning unvalidated fields #55755

bluesheep100 opened this issue May 16, 2025 · 6 comments
Labels
needs more info

Comments

@bluesheep100
Copy link

Laravel Version

12.12.0

PHP Version

8.4.7

Database Driver & Version

No response

Description

Expected Behaviour

Validator::valid() returns all validated valid fields like Validator::validate()/Validator::validated(), without throwing \Illuminate\Validation\ValidationException

Actual Behaviour

Validator::valid() returns fields that are not explicitly invalid by an existing rule as in:

dd(\Validator::make(['bad_thing' => '<html>', 'foo' => 'bar'], ['foo' => 'required|string'])->valid());
// returns ['bad_thing' => '<html>', 'foo' => 'bar'] instead of ['foo' => 'bar']

Steps To Reproduce

// Create a Validator instance:
$validator = \Validator::make(
    ['bad_thing' => '<html>', 'foo' => 'bar'],
    ['foo' => 'required|string'],
);
// Dump results:
dd($validator->validated(), $validator->valid());
@bluesheep100
Copy link
Author

I also want add why this behaviour might be a problem:
For example, if I have a model like:

class Foobar extends Model
{
    protected $fillable = ['metadata'];
    protected $casts = ['metadata' => 'array'];
}

Then I want to save something to it from my frontend:

// FoobarController.php
public function saveMetadata(Request $req, Foobar $foo)
{
    $validation = \Validator::make(['metadata' => 'required|array', 'metadata.serial_number' => 'required|string'], $request->all());
    // Maybe I have some more custom Validation flow, and want to use my own Exception type
    if ($validation->fails()) {
        throw new FoobarValidationException($validation);
    }

    // Validation didn't fail, so we save the data
    $foo->update($validation->valid());

    return to_route('foo.index');
}

Here, the user can now have injected arbitrary data into the nested fields of my metadata array, because Validator::valid() returns those nested fields, regardless of whether there's a rule validating them or not. This can also happen as in the original example, albeit less likely because of the syntax and steps required.

@crynobone
Copy link
Member

Does Laravel documentation suggest $foo->update($validation->valid());? or does it suggest $foo->update($validation->validated());?

Sorry, something went wrong.

@bluesheep100
Copy link
Author

In this section, $validator->validated() is used, which I agree with in a general sense. The problem for me is that if you ever want to not throw an exception and/or cause a redirect, it seems like the only option is to use the (in my opinion) broken $validator->valid() method.
My own particular use case is that I have a frontend-heavy and complex form with ~50 fields, and I save it in JSON (largely for flexibility).
So because it has autosaving, I wanna be able to set everything as required, because it is, but simply use the validator to tell me what to save and what not to save. Then use the proper $validator->validate() later, when it's "committed". I can't use the $validator->failed() method to check myself, because it ignores nested fields if their siblings fail validation. (which I've heard is in fact intentional)
I've managed to get a workaround working for the latter, though.

@crynobone
Copy link
Member

it seems like the only option is to use the (in my opinion) broken $validator->valid() method.

No, you should be using validated() as that's what is recommended by the framework. While valid() may not work as you attended, the behavior is what's being expected, and changing this will cause breaking changes to existing application.

@bluesheep100
Copy link
Author

I don't necessarily want valid() to change, I'd also be fine with an entirely new ->validateWithoutThrowing() method or something, to cover this case.

@bluesheep100
Copy link
Author
bluesheep100 commented May 20, 2025 •
edited
Loading

While valid() may not work as you attended, the behavior is what's being expected, and changing this will cause breaking changes to existing application.

Could you maybe also give some more explanation as to why this behaviour is expected?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
needs more info
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@crynobone @bluesheep100
0