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

Rules with numeric keys do not validate properly #51499

Closed
Tofandel opened this issue May 18, 2024 · 10 comments
Closed

Rules with numeric keys do not validate properly #51499

Tofandel opened this issue May 18, 2024 · 10 comments

Comments

@Tofandel
Copy link
Contributor

Tofandel commented May 18, 2024

Laravel Version

10.48.10

PHP Version

8.3

Database Driver & Version

No response

Description

Numeric rules (Eg: [4 => 'required', 8 => 'required']) when added to a validator are not saved properly, they are transformed into[0 => 'required', 1 => 'required'] in the constructor

I tracked it down to this line

$this->rules = array_merge_recursive(
$this->rules, $response->rules
);

It seems the array merge recursive treats any numeric key differently and just appends it to the array instead of preserving the keys, this is explained in the php doc, but undesirable behavior in this case https://www.php.net/manual/en/function.array-merge-recursive.php

array_merge_recursive([], ['foo' => 'required', 4 => 'required', 8 => 'required']);
= [
    "foo" => "required",
    0 => "required",
    1 => "required",
  ]

Steps To Reproduce

Running the following in a tinker session is enough to reproduce

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

A possible solution is a different implementation of the array_merge_recursive as seen here https://www.php.net/manual/en/function.array-merge-recursive.php#106985

Prior attempt at a fix #39368

@crynobone
Copy link
Member

Since this behavior has been present ever since the beginning I don't think we can change this. Should you want to propose this change I suggest you to attempt a PR to the master branch. Thanks

@Jacobs63
Copy link
Contributor

Jacobs63 commented May 20, 2024

What is the actual, valid, use-case for this?

This seems very unlikely to be anyhow useful in a real use-case.

@Tofandel
Copy link
Contributor Author

Tofandel commented May 20, 2024

If I'm reporting it it's because I encountered this bug while coding for one. I'm sending ids as keys in the validator, the ids are basically ids of fields in the db and each field has its own set of rule. The form is dynamic in this way, I don't use the error message, I'm making my own based on the failing rule/field

@Tofandel
Copy link
Contributor Author

Tofandel commented May 20, 2024

This "behaviour" is an unintended bug of a native php function and it existing forever is not a valid reason to not want to improve the tool/framework we are working on for future use

A validator works on key and there is no reason the key you pass in should ever change

Copy link

Thank you for reporting this issue!

As Laravel is an open source project, we rely on the community to help us diagnose and fix issues as it is not possible to research and fix every issue reported to us via GitHub.

If possible, please make a pull request fixing the issue you have described, along with corresponding tests. All pull requests are promptly reviewed by the Laravel team.

Thank you!

@crynobone
Copy link
Member

@Tofandel feel free to send a PR to master branch as suggested earlier.

@rodrigopedra
Copy link
Contributor

As a workaround, you can wrap your fields' IDs into the same name key.

Code will explain better:

// routes/web.php
<?php

use Illuminate\Http\Request;
use Illuminate\Support\Facades\Route;

// from DB
$fields = [
    4 => ['label' => 'name', 'rule' => 'required'],
    8 => ['label' => 'surname', 'rule' => 'required'],
];

Route::get('/', fn () => view('form', ['fields' => $fields]));

Route::post('/', function (Request $request) use ($fields) {
    $rules = collect($fields)->mapWithKeys(fn ($field, $id) => [
        'field.' . $id => $field['rule'],
    ]);

    $attributes = collect($fields)->mapWithKeys(fn ($field, $id) => [
        'field.' . $id => $field['label'],
    ]);

    $request->validate($rules->all(), attributes: $attributes->all());

    return back();
});
{{-- resources/views/form.blade.php --}}
<div style="white-space: pre-wrap;">@json($errors->all(), JSON_PRETTY_PRINT)</div>

<form action="/" method="POST">
    @csrf

    @foreach($fields as $id => $field)
        <label for="field-{{ $id }}">{{ $field['label'] }}</label>
        <input type="text" id="field-{{ $id }}" name="field[{{ $id }}]">
        <br>
    @endforeach

    <button type="submit">send</button>
</form>

@Tofandel
Copy link
Contributor Author

Tofandel commented May 21, 2024

@rodrigopedra Thanks, that's what I did but then the problem is that the path of the field in the error bag doesn't match with the name of the field in Inertia which is yet another workaround needed, very bad DX for a valid use case

@driesvints
Copy link
Member

Thanks. This has come up before quite a few times. We decided we won't be changing this behaviour because this could have quite an impact on existing apps, sorry for that. We could maybe make the docs more clear about this so feel free to attempt a PR to the docs.

@Tofandel
Copy link
Contributor Author

Tofandel commented May 21, 2024

@driesvints

quite an impact on existing app

I would like to see what exactly that could be, because I don't see it at all, and not a single test is failing on #51516. A validator works only on keyed arrays so this would have zero impact on existing apps, it's only fixing a bug that made this use case not possible in the first place (a use case being having those keys simply be numbers as well)

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

No branches or pull requests

5 participants