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

Add Request validation data pipe #381

Closed
wants to merge 4 commits into from
Closed

Add Request validation data pipe #381

wants to merge 4 commits into from

Conversation

Yi-pixel
Copy link

Overwrite the data that needs to be verified in the request. You can add a validationData method like FromRequest to
provide the data used to participate in the form validation

@rubenvanassche
Copy link
Member

Not sure if this is correct, since we use normalizers to convert objects into arrays of data. And I don't think we want to create a pipe which does the same because that would become confusing.

Maybe a beforeValidation method hook is a bit safer in this context where you pass in all the properties, you would lose the benefit of having a request object, but the whole system would be consistent which is always a win in my opinion.

@Yi-pixel Yi-pixel deleted the request-validation-data-pipe branch March 30, 2023 07:45
@Yi-pixel
Copy link
Author

Not sure if this is correct, since we use normalizers to convert objects into arrays of data. And I don't think we want to create a pipe which does the same because that would become confusing.

Maybe a beforeValidation method hook is a bit safer in this context where you pass in all the properties, you would lose the benefit of having a request object, but the whole system would be consistent which is always a win in my opinion.

It looks like Normalizers would be more appropriate 😇 .

This ensures that the data is already determined at the beginning.

Thanks

@Yi-pixel
Copy link
Author

Yi-pixel commented Apr 3, 2023

@rubenvanassche

Maybe there should be an optional prepareForValidation static method on Data to do this (using Normalizers collection)?

In my example, this may require a new Normalizers, as I intend to use Validation from laravel-data entirely and not use FormRequest from Laravel anymore (the duplicate code is annoying).

eg:

class Profile extends Controller
{
    public function update(UserData $data)
    {
        // TODO
    }
}

So I needed to organize my data before Data was created (and injected in the Controller method).

I noticed that when injecting directly with Data as a parameter, it always uses $dataClass::from($container['request'] where $container['request'] is not a FormRequest, so it shouldn't go to the existing FormRequestNormalizer, so we also need a new Normalizers.

This new Normalizers should be behind the FormRequestNormalizer.

@rubenvanassche
Copy link
Member

Maybe there should be an optional prepareForValidation static method on Data to do this (using Normalizers collection)?

Mhh this would probably be something breaking, since Normalizers don't have any idea about what data object they are working for and you'll need that to call the method. But I'm not against adding such thing.

Maybe we should also change how the code works, at this moment when multiple payloads are provided to the from method of a data object. We run for each payload a normalizer + pipeline. It would make a lot more sense to run a normalizer on each payload merge them and then send them altogether through the pipeline.

We're working on a next major release of data but it will be a few months before we can release it. That being said, you're always welcome to send in a PR doing this to the beyond-data branch.

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.

2 participants