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

Merged
merged 5 commits into from Aug 20, 2018

Conversation

Projects
None yet
10 participants
@ttsuru
Contributor

ttsuru commented Aug 7, 2018

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)

@ttsuru ttsuru force-pushed the ttsuru:fix_nested_validated branch from ed1fef6 to 8af6002 Aug 7, 2018

@jmarcher

This comment has been minimized.

Contributor

jmarcher commented Aug 7, 2018

Can you please add new tests and leave the old tests untouched?

@ttsuru

This comment has been minimized.

Contributor

ttsuru commented Aug 7, 2018

@jmarcher
Already added tests.
I split test into two as existing tests were testing multiple tests in one test.

@jmarcher

This comment has been minimized.

Contributor

jmarcher commented Aug 7, 2018

@ttsuru What I mean is that you should mostly never change existing tests, this could hide possible breaking changes.

@X-Coder264

This comment has been minimized.

Contributor

X-Coder264 commented Aug 7, 2018

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.

@ttsuru ttsuru force-pushed the ttsuru:fix_nested_validated branch from 29dfb4e to 13e6f2c Aug 7, 2018

@ttsuru

This comment has been minimized.

Contributor

ttsuru commented Aug 7, 2018

@jmarcher Revert tests. But the previous test is not a good.

array.* rule is not a commonly used rule.

@jmarcher

This comment has been minimized.

Contributor

jmarcher commented Aug 7, 2018

Why not? I use it

if you have something like:

<input type="hidden" name="locales[]" value="en">
<input type="hidden" name="locales[]" value="it">
<input type="hidden" name="locales[]" value="de">

You need to validate it as:

'locales.*' => 'in:en,de,it'

But there are much more uses for it.

@ttsuru

This comment has been minimized.

Contributor

ttsuru commented Aug 7, 2018

@jmarcher Sorry for not explaining enough.
Only array.* rule is not a commonly used.
I think that should be used together 'array' => 'array'

https://stackoverflow.com/questions/42258185/how-to-validate-array-in-laravel

@X-Coder264

This comment has been minimized.

Contributor

X-Coder264 commented Aug 7, 2018

@ttsuru Since you are now calling the getRules method on a \Illuminate\Contracts\Validation\Validator instance, you should add that method to the interface.

@ttsuru

This comment has been minimized.

Contributor

ttsuru commented Aug 7, 2018

@X-Coder264 I think so, too.
But even in other places in Laravel, validate method is called.

eg


@X-Coder264

This comment has been minimized.

Contributor

X-Coder264 commented Aug 7, 2018

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).

@ttsuru

This comment has been minimized.

Contributor

ttsuru commented Aug 7, 2018

Can I change \Illuminate\Contracts\Validation\Validator interface for 5.7 release ?
Should I add validate and getRules method?

@X-Coder264

This comment has been minimized.

Contributor

X-Coder264 commented Aug 7, 2018

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 :)

@ttsuru

This comment has been minimized.

Contributor

ttsuru commented Aug 7, 2018

@X-Coder264 Does Laravel have a policy for changing Contracts? I can't find.

@X-Coder264

This comment has been minimized.

Contributor

X-Coder264 commented Aug 7, 2018

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.

@tillkruss

This comment has been minimized.

Member

tillkruss commented Aug 7, 2018

@ttsuru: Yes, we only change interfaces in major releases.

@X-Coder264

This comment has been minimized.

Contributor

X-Coder264 commented Aug 7, 2018

@tillkruss Yeah, I told him that already 😛

@ttsuru

This comment has been minimized.

Contributor

ttsuru commented Aug 7, 2018

@tillkruss Thanks. Does Major means 5.6 -> 5.7 or 5.X -> 6.X ?

@X-Coder264

This comment has been minimized.

Contributor

X-Coder264 commented Aug 7, 2018

Major means 5.6 -> 5.7. 5.X -> 6.0 is a "paradigm" version according to Laravel terminology.

@ttsuru

This comment has been minimized.

Contributor

ttsuru commented Aug 8, 2018

Other idea.

Validator::validate() returns validated values in #23397
Should I not use Validator::getRules()?

@ttsuru

This comment has been minimized.

Contributor

ttsuru commented Aug 9, 2018

Fixed this issue.

Please review this pull request.

@themsaid themsaid self-requested a review Aug 10, 2018

@themsaid

themsaid approved these changes Aug 10, 2018 edited

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.

@ttsuru

This comment has been minimized.

Contributor

ttsuru commented Aug 11, 2018

Thanks review.
I remove ValidatesRequests::extractInputFromRules() method.

@ttsuru ttsuru force-pushed the ttsuru:fix_nested_validated branch from 24b448b to aaa08d6 Aug 11, 2018

@donnysim

This comment has been minimized.

Contributor

donnysim commented Aug 11, 2018

I'd say a very common use case is

[
    'foo' => ['required', 'array', 'min:1'],
    'foo.*.name' => ['required'],
    ...
]

which will always make the new behavior still not fully usable in those cases as 'foo' will grab all items without filtering fields :(

@themsaid

This comment has been minimized.

Member

themsaid commented Aug 11, 2018

@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 :)

@donnysim

This comment has been minimized.

Contributor

donnysim commented Aug 11, 2018

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 ['foo.*.name', 'foo.*.last_name']?

Edit: I mean not removing the nested filtering from the validator, but make it available outside of the validator too.
Edit2: could be like a helper: data_keys_intersect(array $keys, $target)

@themsaid

This comment has been minimized.

Member

themsaid commented Aug 11, 2018

@donnysim you can just ignore it and do your own filtering.

@taylorotwell taylorotwell merged commit aaa08d6 into laravel:5.7 Aug 20, 2018

2 checks passed

continuous-integration/styleci/pr The analysis has passed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@apreiml

This comment has been minimized.

apreiml commented on aaa08d6 Sep 19, 2018

@ttsuru Is there a specific reason why you removed this?

This comment has been minimized.

Contributor

devcircus replied Sep 19, 2018

See comment by @themsaid.

@antonkomarev

This comment has been minimized.

Contributor

antonkomarev commented on src/Illuminate/Foundation/Http/FormRequest.php in 941c8c7 Sep 21, 2018

This could be related to issue #25736

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment