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] Backport #26486 - Fix FormRequest validation triggering twice #26731

Merged
merged 1 commit into from Dec 4, 2018

Conversation

Projects
None yet
4 participants
@rodrigopedra
Contributor

rodrigopedra commented Dec 4, 2018

As discussed in issue #25736 , after some changes introduced in 941c8c7 the FormRequest rules were being triggered twice.

This issue was first addressed in the 5.8 branch by the PR #26419 which was backported to 5.7 in PR #26604 .

This fixes the issue in the Validator side, but when using the FormRequest this fix wasn't sufficient as every time the getValidatorInstance() method was called it creates a new validator instance which triggers the validation's rules again.

If one uses the validated() method two instances would be created one when resolving the request, other when calling this method.

Later another PR was sent to the 5.8 branch ( #26486 ) which caches the validator instance instantiated in the FormRequest object.

This solves the problem as there will be only one Validator instance.

This PR backports the fix in PR #26486 to the 5.7 branch.

@rodrigopedra rodrigopedra force-pushed the rodrigopedra:5.7 branch from 15beac1 to f4f0a6c Dec 4, 2018

@rodrigopedra rodrigopedra changed the title from [5.7] Backport #26486 - Fix validation triggering twice to [5.7] Backport #26486 - Fix FormRequest validation triggering twice Dec 4, 2018

@rodrigopedra

This comment has been minimized.

Contributor

rodrigopedra commented Dec 4, 2018

Reading through issue #26651 it seems that all these PR's were needed after the changes introduced by PR #25128.

Although I find some of the solutions introduced in the subsequent PR's, such as caching the validator instance, better than the previous approach, some code that was introduced seems fragile to me, for example: https://github.com/laravel/framework/pull/26486/files#diff-93a08199642b328e7c151fef15179216R184

Maybe we should try another way to solve the problem that PR #25128 was trying to solve initially and revert the changes introduced after it.

@deleugpn

This comment has been minimized.

Contributor

deleugpn commented Dec 4, 2018

Reading through the PR back then it looks like the double validation problem was introduced because of the getRules call which wasn't on the contract and therefore would require a breaking change.
For 5.8, at least, that solution should be revisited.

Ping @ttsuru

@taylorotwell taylorotwell merged commit f1cfd61 into laravel:5.7 Dec 4, 2018

2 checks passed

continuous-integration/styleci/pr The analysis has passed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment