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

[5.7] an array of messages for custom validation rules #26327

Merged
merged 3 commits into from Nov 2, 2018

Conversation

Projects
None yet
5 participants
@rubenvanassche

rubenvanassche commented Oct 31, 2018

At a current project we're working on we have a lot(+200) rules for validating a request. The solution is splitting these rules into separate custom rule classes. Like so

// Before
'units.*.articles.*.date_start' => 'required|date', 
'units.*.articles.*.date_end' => 'required|date',

// After
'units.*.articles' => new UnitArticlesRule,

Then in our UnitArticlesRule class we do the following:

class HabitantContractArticlesRule implements Rule
{
    /** @var array */
    protected $messages = [];

    public function passes($attribute, $value)
    {
        foreach ($value as $article){
            $validator = Validator::make($article, $this->rules($article));

            if($validator->fails()){
                $this->messages = array_merge($this->messages, $validator->errors()->all());
            }
        }

        return count($this->messages) === [];
    }

    public function message()
    {
        return $this->messages;
    }

    public function rules(array $article){
        $rules = [
            'date_start' => 'nullable|date',
            'date_end' => 'nullable|date',
        ];

        if($article['date_start'] !== null){
            $rules["date_end"][] = 'after:' . Carbon::make($article['date_start']);
         }

        return $rules;
    }
}

As you can see, multiple articles can have validation errors. In the current implementations of custom validation rules we can only return a string. This PR adds the ability to return an array of messages so the example above will work.

The PR won't break any functionality and two tests are added.

@driesvints

Can you fix the StyleCI failures?

Ruben Van Assche
@rubenvanassche

This comment has been minimized.

rubenvanassche commented Oct 31, 2018

Should be fixed now!

@@ -16,7 +16,7 @@ public function passes($attribute, $value);
/**
* Get the validation error message.
*
* @return string
* @return string|array

This comment has been minimized.

@GrahamCampbell

GrahamCampbell Oct 31, 2018

Member

Could you also update the phpdoc of the implementations?

@brendt

This comment has been minimized.

Contributor

brendt commented Oct 31, 2018

How about being able to return \Illuminate\Support\MessageBag ? If you're using validator in your custom rule, as the example shows, you could just return $validator->errors() instead of having to parse it into an array.

@brendt

This comment has been minimized.

Contributor

brendt commented Oct 31, 2018

Also: I think you should keep in mind that the field where the error is shown. Right now multiple errors are shown on the parent field:

foreach ($messages as $message) {
    $this->messages->add($attribute, $this->makeReplacements(
        $message, $attribute, get_class($rule), []
    ));
}

While I expected it to be more like:

foreach ($messages as $childAttribute => $message) {
    $nestedAttribute = "{$attribute}.{$childAttribute}";

    $this->messages->add($nestedAttribute, $this->makeReplacements(
        $message, $nestedAttribute, get_class($rule), []
    ));
}
@taylorotwell

This comment has been minimized.

Member

taylorotwell commented Oct 31, 2018

@brendt Interesting. Why would it make assumptions about a child attribute? I'm not sure I would expect that behavior.

@taylorotwell taylorotwell merged commit f5b9acd into laravel:5.7 Nov 2, 2018

2 checks passed

continuous-integration/styleci/pr The analysis has passed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@brendt

This comment has been minimized.

Contributor

brendt commented Nov 5, 2018

@taylorotwell let's assume the following example, it's nothing I'd do IRL, but it illustrates my point.

// The rules in the request class

$rules = [
    'articles' => new ArticleRule(),
];
// ArticleRule will validate two properties: type and amount

public function passes($attribute, $value)
{
    if (!in_array($value['type'], ArticleType::toArray())) {
        $this->messages['type'] = 'The article type must be defined in the ArticleType enum.';
    }

    if ($value['amount'] < 0) {
        $this->messages['amount'] = 'You can only specify a positive amount.';
    }

    return empty($this->messages);
}

Say both rules fail, we'd get two errors in the final result, mapping to the type and amount field respectively. I don't think this is the correct behaviour, as they should map to articles.*.type and articles.*.amount.

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