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

[9.x] Regex validation rules for array items containing 'or' seperators cause ErrorException on input validation #40924

Closed
jnoordsij opened this issue Feb 10, 2022 · 11 comments · Fixed by #40941
Assignees
Labels

Comments

@jnoordsij
Copy link
Contributor

  • Laravel Version: 9.0.1
  • PHP Version: 8.1.2
  • Database Driver & Version: irrelevant

Description:

Using an or sign (|) within a regex validation rule for an array item breaks on Laravel 9.x. This works fine on Laravel 8.x.

It seems somewhere the rules are parsed and the | is interpreted as start of another validation rule instead of as being part of the regex rule, causing the parsed regex rule to be regex:/^(typeA|.

Most probably related to #40498.

Steps To Reproduce:

See https://github.com/jnoordsij/laravel/tree/regex-validation-bug-8.x for the (working) 8.x version and https://github.com/jnoordsij/laravel/tree/regex-validation-bug-9.x for the test breaking on 9.x.

  1. clone repo
  2. composer install
  3. cp .env.example .env.testing
  4. php artisan key:generate --env=testing
  5. php artisan test tests/Feature/SimpleRequestTest.php

Resulting exception:
ErrorException: preg_match(): No ending delimiter '/' found in ...

@jbrooksuk jbrooksuk self-assigned this Feb 10, 2022
@jbrooksuk
Copy link
Member

You should wrap your regex rule in an array. For example, this will fail:

$v = new Validator($trans, ['x' => 'foo'], ['x' => 'Regex:/^(taylor|james)$/i']);
$this->assertTrue($v->passes());

But this will pass:

-$v = new Validator($trans, ['x' => 'foo'], ['x' => 'Regex:/^(taylor|james)$/i']);
+$v = new Validator($trans, ['x' => 'foo'], ['x' => ['Regex:/^(taylor|james)$/i']]);
$this->assertTrue($v->passes());

@jbrooksuk
Copy link
Member

Note that this is what the documentation instructs you to do, when your regex contains a |. https://laravel.com/docs/9.x/validation#rule-regex

@jbrooksuk
Copy link
Member

Sorry, I see this is more nuanced as it's affecting nested rules only. I'll re-open whilst I investigate.

@jbrooksuk jbrooksuk reopened this Feb 10, 2022
@jnoordsij
Copy link
Contributor Author

jnoordsij commented Feb 10, 2022

The regex rule is actually in array form; I could understand that using something like 'contact.phonenumbers.*.type' => 'regex:/^(typeA|typeB|typeC)$/|required|string' would be too ambiguous to parse, but the example I provided seems to comply with what the documentation instructs.

@jbrooksuk
Copy link
Member

Right, this passes in 8.x but fails in 9.x.

$v = new Validator($trans,
    ['x' => ['y' => ['z' => 'james']]],
    ['x.*.z' => [
        'required',
        'string',
        'Regex:/^(taylor|james)$/i'
    ]]
);
$this->assertTrue($v->passes());
1) Illuminate\Tests\Validation\ValidationValidatorTest::testValidateRegex
preg_match(): No ending delimiter '/' found

@taylorotwell
Copy link
Member

@stevebauman any idea how to solve this? Otherwise, we need to revert Rule::forEach

@stevebauman
Copy link
Contributor

Give me a moment -- investigating now 👍

@rodrigopedra
Copy link
Contributor

rodrigopedra commented Feb 10, 2022

@stevebauman from my investigation so far the error is around here:

foreach (Arr::flatten((array) $rules) as $rule) {
if ($rule instanceof NestedRules) {
$compiled = $rule->compile($key, $value, $data);
$this->implicitAttributes = array_merge_recursive(
$compiled->implicitAttributes,
$this->implicitAttributes,
[$attribute => [$key]]
);
$results = $this->mergeRules($results, $compiled->rules);
} else {
$this->implicitAttributes[$attribute][] = $key;
$results = $this->mergeRules($results, $key, $rule);
}
}

If I replace the lines above by their 8.x counterpart...

foreach ((array) $rules as $rule) {
$this->implicitAttributes[$attribute][] = $key;
$results = $this->mergeRules($results, $key, $rule);
}

...it works (tested ->passes(), ->fails() and ->validated())

I am still digging into it, but I thought it would be nice to share

@rodrigopedra
Copy link
Contributor

@stevebauman in 8.x when $rule was a regex and reached here:

protected function explodeExplicitRule($rule, $attribute)
{
if (is_string($rule)) {
return explode('|', $rule);
} elseif (is_object($rule)) {
return Arr::wrap($this->prepareRule($rule, $attribute));
}
return array_map(
[$this, 'prepareRule'],
$rule,
array_fill(array_key_first($rule), count($rule), $attribute)
);
}

It was wrapped into an array, now it reaches here as a string, thus it gets on the first if clause that explodes it.

One other thing: if the regex validation rule is a top-level rule it works. It only fails when it is nested.

@rodrigopedra
Copy link
Contributor

rodrigopedra commented Feb 10, 2022

@stevebauman if we change this line:

foreach (Arr::flatten((array) $rules) as $rule) {

back to as it was in 8.x

foreach ((array) $rules as $rule) {

It works. From the blame this line was changed by you on PR #40498

I will run the tests with this change and report back.

EDIT: it works with the test case provided by OP

@rodrigopedra
Copy link
Contributor

@stevebauman Changing it back as of my last comment makes these two tests fail:

There were 2 failures:

1) Illuminate\Tests\Validation\ValidationRuleParserTest::testExplodeHandlesArraysOfNestedRules
Failed asserting that two arrays are equal.
--- Expected
+++ Actual
@@ @@
 Array (
     'users.0.name' => Array (
-        0 => 'required'
-        1 => 'in:"taylor"'
+        0 => Array (...)
+        1 => Array (...)
     )
     'users.1.name' => Array (
-        0 => 'required'
-        1 => 'in:"abigail"'
+        0 => Array (...)
+        1 => Array (...)
     )
 )

/home/rodrigo/code/open-source/framework/tests/Validation/ValidationRuleParserTest.php:171

2) Illuminate\Tests\Validation\ValidationRuleParserTest::testExplodeHandlesRecursivelyNestedRules
Failed asserting that null matches expected 'Taylor Otwell'.

/home/rodrigo/code/open-source/framework/tests/Validation/ValidationRuleParserTest.php:192
/home/rodrigo/code/open-source/framework/src/Illuminate/Validation/NestedRules.php:37
/home/rodrigo/code/open-source/framework/src/Illuminate/Validation/ValidationRuleParser.php:122
/home/rodrigo/code/open-source/framework/src/Illuminate/Validation/ValidationRuleParser.php:96
/home/rodrigo/code/open-source/framework/src/Illuminate/Validation/ValidationRuleParser.php:71
/home/rodrigo/code/open-source/framework/src/Illuminate/Validation/ValidationRuleParser.php:201
/home/rodrigo/code/open-source/framework/src/Illuminate/Validation/ValidationRuleParser.php:187
/home/rodrigo/code/open-source/framework/src/Illuminate/Validation/ValidationRuleParser.php:159
/home/rodrigo/code/open-source/framework/src/Illuminate/Validation/ValidationRuleParser.php:67
/home/rodrigo/code/open-source/framework/src/Illuminate/Validation/ValidationRuleParser.php:49
/home/rodrigo/code/open-source/framework/tests/Validation/ValidationRuleParserTest.php:209

I have an appointment in 10 minutes, so I will only be able to look into it further later. Hope it helps you out :)

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