Skip to content

Conversation

@Tofandel
Copy link
Contributor

@Tofandel Tofandel commented May 21, 2024

Fixes #51499 without changing the merge behavior (see testMergeRules in the PR that #39368 wouldn't have passed), simply preserve all the keys passed to the validator rules

Bug description

Running the following in a tinker session

Validator::validate(['foo' => 'baz', 4 => 'foo', 8 => 'baz'], ['foo' => 'required', 4 => 'required', 8 => 'required']);

Results in

 Illuminate\Validation\ValidationException The 0 field is required. (and 1 more error). 

Because array_merge_recursive rekeys any numeric keys it encounters

array_merge_recursive([], ['foo' => 'required', 4 => 'required', 8 => 'required']); = [ "foo" => "required", 0 => "required", 1 => "required", ]
@Tofandel Tofandel force-pushed the patch-2 branch 2 times, most recently from 1a5406a to 1372039 Compare May 21, 2024 00:37
$this->rules, $response->rules
);
foreach ($response->rules as $key => $rule) {
$this->rules[$key] = array_merge($this->rules[$key] ?? [], $rule);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We only need array_merge, given that rules are always nested only 2 levels deep and are always associative arrays (Asserted by the ValidationRuleParser)

@taylorotwell
Copy link
Member

Any reason this was sent to master vs. 11.x?

@taylorotwell taylorotwell marked this pull request as draft May 21, 2024 17:51
@driesvints
Copy link
Member

@taylorotwell as we suspected in the past this might be a breaking change: #39368

@driesvints driesvints marked this pull request as ready for review May 21, 2024 18:28
@Tofandel
Copy link
Contributor Author

Tofandel commented May 21, 2024

I'm arguing it's not a breaking change, because validators requires keyed arrays and so having numeric keys before was not possible without encountering this bug (and thus make the validation always fail/unusable) and it would not cause any change with assoc arrays either anyways, only with non sequential numeric keys

The previous PR was about changing the behavior to array_replace_recursive instead of array_merge_recursive which was a whole level of a breaking change, but this PR keeps the merge while fixing the numeric key issue. I've been told to submit to master anyways but I really think there is no breaking change here

Whether you want to err on the side of extreme caution or not is up to you, me I would just like to see this bug fixed sometime in the future but it doesn't matter major or minor

@taylorotwell taylorotwell merged commit 83e28d0 into laravel:master May 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

3 participants