-
-
Notifications
You must be signed in to change notification settings - Fork 210
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
Pass in payload relative to the data object when generating rules #264
Pass in payload relative to the data object when generating rules #264
Conversation
If it's not possible to merge a change like this into the package would it be possible to do something like provide a choice of two different The users could then bind the resolver of their choice. Not sure if that's a terrible idea or not! |
ffa8b01
to
3d10808
Compare
Hi @bentleyo, First of all, thanks for the PR. I'm not going to merge it because creating so many duplicate rules feels a bit wrong. We use this package on large datasets and this PR would create an array of more than 400 rules which is a bit much. That being said, I'm totally in with making the |
@rubenvanassche thanks for your reply! No problem, I expected that this approach might be too far detached from how validation traditionally works in Laravel to be merged as-is into the package. I still think this is something that might be really useful for situations when more dynamic validation is required, as the I'll have a think about it, but do you have any thoughts on how you would prefer the resolver to be configurable? Another approach I've thought of is potentially passing through the path to the DTO e.g. I'm open to suggestions, I haven't contributed much to open-source projects before. |
I just realised that to provide the path to the DTO in the Would an option like Naming is hard! |
e40a449
to
c5f7f07
Compare
@rubenvanassche I have updated this PR so that |
ec9f7e1
to
ab1dd1d
Compare
Hi @bentleyo, Now going through the PR, my colleague told me about this gem: https://laravel.com/docs/9.x/validation#accessing-nested-array-data. Is this something we could use in this case? |
Looking good, one point if we can use the thing I've mentioned above and drop the 0.property, 1.property that would be great. Otherwise this can be merged! |
@rubenvanassche I somehow managed to miss that feature, I'll have a look over the next day or so and see if there's a way I can utilise that functionality. Thanks for your reply! |
Thanks @bentleyo, let me know if something changes and I take another look! |
@rubenvanassche I spent some time investigating this and have just pushed a potential solution. Instead of making the functionality a global config setting I've managed to implement the functionality by creating a Some benefits of this new approach is that a huge array of rules is no longer generated and we're using the Unfortunately I've had to extend the E.g. this would work:
However, this wouldn't:
As I'm now using I've added some extra tests. Let me know what you think! |
@rubenvanassche I've added a check to see if the This means that only this new feature requires laravel 9+ and the package can still be used on 8. The tests now pass 😄 |
Thanks, @bentleyo this is great! I've restructured your code a bit but this is such a nice feature to have, thanks! |
Changed it even a little bit by removing the interface |
@rubenvanassche I just had a look at the changes you made. Super impressed! I really like the approach you took 😄 |
Thanks, the original PR was also awesome, good work! 🎉 |
This PR attempts to solve the problem identified by @medvinator in #256 which is something our team has also run into.
If you want to dynamically build validation rules depending on the payload it's very difficult at the moment as the data object receives the full payload and doesn't know the path to its data.
E.g. a data object inside a collection won't know that the path to its data is
collection.2.example
and this makes rule generation very difficult to work with at the moment.This PR approaches this problem by building the validation rules for each set of data. There's some pros and cons to this approach. The pros are that you can now build validation rules using the data relative to the object. However, collections with many many objects inside them will generate rules like:
collection.0.example
,collection.1.example
,collection.2.example
etc.In addition, if there's no data inside the collection no rule will be generated for the child field. E.g. no
collection.*.example
I'm not sure of the best way to approach that problem and for our team the overheard in rule generation wouldn't be an issue and is worth the dynamic validation functionality.
This PR probably requires more tests and some code improvements, but I thought I'd create it to see what your thoughts are and start the discussion 😄
Thanks for creating an amazing package!