-
Notifications
You must be signed in to change notification settings - Fork 11.1k
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
[5.7] Fixed nested rules in validated data #25128
Conversation
ed1fef6
to
8af6002
Compare
Can you please add new tests and leave the old tests untouched? |
@jmarcher |
@ttsuru What I mean is that you should mostly never change existing tests, this could hide possible breaking changes. |
Yeah, it's kinda hard to see what's going on with the tests since you changed a bunch of tests from #23708. Please leave those as they were (since they should still pass) and just add a new test to assert that the nested array rules case works as expected. |
29dfb4e
to
13e6f2c
Compare
@jmarcher Revert tests. But the previous test is not a good.
|
Why not? I use it if you have something like:
You need to validate it as:
But there are much more uses for it. |
@jmarcher Sorry for not explaining enough. https://stackoverflow.com/questions/42258185/how-to-validate-array-in-laravel |
@ttsuru Since you are now calling the |
@X-Coder264 I think so, too. eg
|
Yeah, that's what happens when people keep adding new features to the framework and they don't add the methods to the interface (even though they should) just so that the MR can be seen as a "no breaking changes" MR which means that instead of having to wait for the feature on the next framework version release (which are released every six months), they get the feature "immediately" (just a few days after when the new point release is tagged). |
Can I change |
Yes, you can add those methods to the interface, as Laravel allows for breaking changes in major versions (eg. 5.6 -> 5.7) since it doesn't follow semver. And if you ask me, yes, they should be added. But the final decision is on Taylor :) |
@X-Coder264 Does Laravel have a policy for changing Contracts? I can't find. |
There isn't such a thing, at least to my knowledge. Just add the methods to the interface and we'll see what Taylor is gonna say when he comes to check the PR. |
@ttsuru: Yes, we only change interfaces in major releases. |
@tillkruss Yeah, I told him that already 😛 |
@tillkruss Thanks. Does Major means 5.6 -> 5.7 or 5.X -> 6.X ? |
Major means 5.6 -> 5.7. 5.X -> 6.0 is a "paradigm" version according to Laravel terminology. |
Other idea.
|
Fixed this issue. Please review this pull request. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the PR is not breaking the current behaviour, and it fixes the issue reported by the author.
However currently users.*.name
will return all the content of the "users" input, not just the name, this PR fixes that but people might be relying on the old behaviour so noting this in the docs is needed.
I believe ValidatesRequests::extractInputFromRules() is not needed anymore though.
Thanks review. |
24b448b
to
aaa08d6
Compare
I'd say a very common use case is
which will always make the new behavior still not fully usable in those cases as 'foo' will grab all items without filtering fields :( |
@donnysim which is kind of expected, the built-in filteration is meant for general usage and it's not meant to cover all edge cases, you'll need to apply special filters to work for some edge cases, that applies to everything in the framework :) |
I think it would be better to extract the filtering logic outside of the validator? Then we could use custom extraction on top of the validator extraction? Validator filters 'foo' and then we filter only Edit: I mean not removing the nested filtering from the validator, but make it available outside of the validator too. |
@donnysim you can just ignore it and do your own filtering. |
Fixed #23708, #24607
In #23708, This is not work with nested array rule.
This PR add 2 types of test cases -> Nested rule (foo.bar) and Nested array rule (foo.*.bar)