[9.x] Add ability to resolve a parser

[jryd]

This is an attempt at solving the issues outlined in #43192 where validating field names or values that contain a pipe no longer work in Laravel 9.

There's more detail in the issue but, as a simple summary, in Laravel 8 it was possible to perform validation on fields/values that used a pipe character when you opted to use the array syntax of defining validation rules rather than the string-based rules that use a pipe delimiter.

As an example, the below would not work in Laravel 8

Validator::make(
    [
        '|field' => 'somevalue|foo'
    ], 
    [
        '|field' => 'ends_with:|foo,|bar'
    ]
)->passes() // BadMethodCallException with message 'Method Illuminate\Validation\Validator::validateFoo, does not exist.'

But the below would work in Laravel 8

Validator::make(
    [
        '|field' => 'somevalue|foo'
    ], 
    [
        '|field' => [
            'ends_with:|foo,|bar'
        ]
    ]
)->passes() // true

In Laravel 9, Rule::forEach was added and to support its functionality the rules need to be flattened so that it can be detected whether a rule is an instance of NestedRules.
This is done inside of the explodeWildcardRules method where this is the behaviour in Laravel 9

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

Whereas in Laravel 8 it was

foreach ((array) $rules as $rule) {
    // ...
}

The result of this change is that when you're working with an array of validation rules, they're now parsed one-by-one rather than as an array of rules that they were passed through as.

Considering that these are string rules, they then inevitably enter the string check inside of the explodeExplicitRule method:

if (is_string($rule)) {
    [$name] = static::parseStringRule($rule);

    return static::ruleIsRegex($name) ? [$rule] : explode('|', $rule);
}

And as such are unintentionally being split on the pipe now. As is evident in the code block above, and detailed more in #40924, this issue also affected regex rules but a workaround was added to address that specific rule.

I explored a few different options to add this support, but they all had drawbacks, assumptions, or breaking changes, and so I've opted for a solution that doesn't really alter any framework code, but allows the flexibility for someone inside their project to override how this works in any way that they want.

By moving the resolution of the ValidationRuleParser to a method, we can extend the Validator class and override the parser method to return a different, extended instance of the ValidationRuleParser that allows a developer to change any functionality they like; including changing how string rules are split if you need to work with fields/values that use pipes in them.

This solution offers the ability to specify at a per validator level, custom resolution logic (or not) such that you can isolate your behaviour to single instances of the validator rather than overriding how it works globally in your app.
Of course, you could still extend the Validator and update the two methods that new up the ValidationRuleParser but this means you need to override and maintain that entire method just to use an extended ValidationRuleParser - using the proposed solution you can just change it in one place and it's used everywhere a ValidationRuleParser is required.

An alternative solution could use statics or container bindings to resolve/override a custom delimiting function to offer similar control to the developer to change how rules are split but suffers from the fact that this is a global change and as such could have unintended side-effects if you were performing multiple levels of validation (although I'm unsure how common this would actually be).

[stevebauman]

Hi @jryd, author of the Rule::forEach() implementation here.

To answer your question from your raised issue:

Given this wasn't the behaviour in Laravel 8, and doesn't appear to be an intended effect of adding the Rule::forEach feature, I wanted to raise this and ask whether this was a desired side-effect?

It was not an intended or desired side-effect of the feature. Though, having fields containing the validation delimiter is tricky.

Since there are currently no tests in the framework or documentation that I can find that test this, it's unlikely that having fields containing pipes was meant to be supported.

Having said that, I think your other proposed solution sounds like it would work well for this. Maybe we could add the ability to override the delimiter that the validator uses for these circumstances. Something like:

Validator::make(['...'])->delimitBy('#')->validate();

In a FormRequest:

/**
 * Configure the validator instance.
 *
 * @param  \Illuminate\Validation\Validator  $validator
 * @return void
 */
public function withValidator($validator)
{
    $validator->delimitBy('#');
}

Then, you wouldn't have to workaround this and create arrays for validation rules not needing them:

public function rules()
{
    return [
        'foo' => 'string#same:|myfield#in:foo,bar'
    ];
}

public function withValidator($validator)
{
    $validator->delimitBy('#');
}

Thoughts?

[jryd]

Hey @stevebauman !

It's funny you suggest that, because that was my initial approach (h/t to @timacdonald for the idea); although I used a withDeliminator - same idea though.

The issue I ran into was how to pass this through to the ValidationRuleParser without it being a breaking change.

There's a couple of quirks that make it challenging:

1. Currently the Validator class eventually calls the ValidationRuleParser via the constructor when initialised. With the current implementation, you'd need to re-call the addRules method after changing the delimiter
2. The NestedRules class also uses the ValidationRuleParser and so it would need to be passed the delimiter so that it could also inject your chosen delimiter into the ValidationRuleParser when it is compiling

I couldn't find a way to achieve your suggested approach without it becoming a breaking change - but if you can see a way to do it then that'd be a perfect solution!

[timacdonald]

I just want to drop and point out the 🐘 in the room, which is the pipes in the input names...

I know what you are thinking and you are correct: it isn't great. However, this format was not one developed by the team building the app - it is based on an extremely popular medical data format standard that is built for interoperability. It think that context matters.

I guess we could also simply have a property on the Validator:

class Validator
{
    protected $parser = ValidationRuleParser::class;


   protected function parser()
   {
       return new ($this->parser)($data);
   }
}

Then the statics can remain static, and the instances can remain instances...

$this->parser::staticMethod();

$this->parser()->instanceMethod();

We could then make a non-breaking change to the Parser constructor

class ValidationRuleParser

    public function __construct(array $data, $explody = null)
    {
        $this->data = $data;

        $this->explody = $explody ?? fn ($rules) => explode('|', $rules));
    }

And then...

- return static::ruleIsRegex($name) ? [$rule] : explode('|', $rule);
+ return static::ruleIsRegex($name) ? [$rule] : ($this->explody)($rule);

Then in an application to override the rule delimination logic, you would end up with...

class CustomValidator extends Validator
{
   protected function parser()
   {
       return new ($this->parser)($data, fn ($rules) => /* ... */);
   }
}

There still is the issue that the NestedRules::compile() func uses the parser as well, but I believe you can work around the issue there (I'm not familiar with the nested rules stuff right now).

The main issue, as James outlined, is that the constructor of the validator triggers side-effects that use the Parser, so we can't use a "setter" - we need to be able to specify the class for the instance before the validator is instantiated.

[stevebauman]

Gotcha, totally understand @jryd, those are some great points...

To add onto this, this issue exists with other validation "escapes", like the colon (:) and comma (,):

$data = [
    'foo' => ',bar',
];

$rules = [
    'foo' => 'required|in:,bar',
];

// false
Validator::make($data, $rules)->passes();

It looks like we should patch this, by offering an escape backslash (\) like Laravel already describes in the documentation, to not only support pipes, but colons, and commas in string based rules:

https://laravel.com/docs/9.x/validation#a-note-on-nested-attributes

$request->validate([
    'title' => 'required|unique:posts|max:255',
    'v1\.0' => 'required',
]);

Ex:

Validator::make(
    [
        '|field' => 'somevalue|foo'
    ], 
    [
        '|field' => 'ends_with:\|foo,\|bar'
    ]
)->passes();

Thoughts?

[timacdonald]

We also discussed this, but it would be a breaking change.

Validator::validate(['foo' => '\\'], ['foo' => 'in:a,\|required'])

where a and \ are allowed values.

[stevebauman]

Ah okay, you're way ahead of me!

What about being able to provide some kind of object instance that you could use with any validation rule?

This would allow us to parse the instance like other validation rules, and resolve issues of parsing values with reserved escape characters.

For example (pseudo-code):

Validator::make(
    [
        '|foo' => '|bar',
    ], 
    [
        '|field' => Rule::make('ends_with', ['|bar']),
    ]
)->passes();

Another example for use-case with a colon:

Validator::make(
    [
        '|foo' => ':bar',
    ], 
    [
        '|field' => ['required', Rule::make('in', [':bar'])],
    ]
)->passes();

// src/Illuminate/Validation/Rule.php

// ...

public static function make($rule, $args)
{
    return new StringRule($rule, $args);
}

Thoughts?

[stevebauman]

Got this working with the above example, let me know if you guys like/hate it:

9.x...stevebauman:framework:feature-string-validation-rule

I've also added tests for all the code examples that you (@jyd) mentioned in your issue #43192:

https://github.com/stevebauman/framework/actions/runs/2886786983

Example:

Validator::make(
    [
        '|field' => 'somevalue|foo'
    ], 
    [
        '|field' => Rule::make('ends_with', ['|foo', '|bar']),
    ]
)->passes();

This rule object acts as an escape hatch for any validation rule that needs arguments with reserved characters (:, ,, |).

[jryd]

Nice solution @stevebauman.

I'd originally tried a different approach in a "rule wrapper" to do something similar but it went down a different tack and had issues.

In your example, how would I provide multiple rules? Would I do so like this?

Validator::make(
    [
        '|field' => 'somevalue|foo',
        'anotherfield' => '|anothervalue',
    ], 
    [
        '|field' => [
            Rule::make('ends_with', ['|foo', '|bar']),
            Rule::make('prohibited_if', ['anotherfield', '|anothervalue']) // or Rule::prohibitedIf(...)
        ]
    ]
)->passes();

[stevebauman]

Thanks @jryd 🙏

Yeah you would do it that way (supplying an array with multiple Rule::make() calls) 👍

This does however make things a bit muddy between the class based rules that have a __toString() method defined and StringRule...

Class based rules that compile themselves down into rule string's (i.e. in:foo,bar) won't work with reserved characters.

Maybe we can add an additional method to these classes that can convert them to a StringRule instance, so that they automatically support reserved characters? I.e.

// src/Illuminate/Validation/Rules/In.php

// ...

public function toRule()
{
    return Rule::make($this->rule, $this->values);
}

Validator::make(
    [
        '|field' => '|bar',
    ], 
    [
        '|field' => [
            Rule::in(['|foo', '|bar']),
        ]
    ]
)->passes();

[timacdonald]

I'm loving this approach TBH if it solves the problem. Much cleaner IMO.

[timacdonald]

Naming suggestion:

'|field' => [
    Rule::raw('ends_with', ['|foo', '|bar']),
],

StringRule => RawRule

[jryd]

I agree - love the approach.

I also think the solution you propose for class-based rules that have a __toString() method sounds reasonable.

[stevebauman]

Ok excellent @jryd 👍

@timacdonald I like that naming, let’s go with that. Thanks for the suggestion 🙏

Let me create a new pull request with these changes and all the tests updated to follow convention. Once submitted, I’ll mention you both in it, along with this PR and the original issues.

[garygreen]

All of this discussion seems pretty redundant. Laravel has supported passing validator rules with an array since Laravel 5, maybe even earlier, which negates pretty much the need for this whole discussion.

See here: #43779 (comment)

There is a bug at the moment that was introduced just in Laravel 9, but that's mentioned in the above PR and should be addressed.