Skip to content
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

[12.x] Preserve numeric keys on the first level of the validator rules #51516

Merged
merged 1 commit into from
May 30, 2024

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
28 checks passed
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