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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants