AbstractValidator should offer a way to validate against missing attributes

[clarkwinkelmann]

At the moment, our AbstractValidator class only validates attributes that are present in the $attributes array

https://github.com/flarum/core/blob/1fc24635f6bc1126cc3385732b862c19165e3b0b/src/Foundation/AbstractValidator.php#L91

This works great for our STORE/PATCH endpoints the way we implement them in core.

On STORE, we first put the values in the model attribute by attribute, ensuring all keys are defined, even if the values are null or missing.

https://github.com/flarum/core/blob/eaac78650ffa0cf23321fcf624172bc1bd202036/src/Discussion/Command/StartDiscussionHandler.php#L65-L68

https://github.com/flarum/core/blob/1fc24635f6bc1126cc3385732b862c19165e3b0b/src/Discussion/Discussion.php#L131-L133

We then run the validator against $model->getAttributes(), where all the keys are present.

https://github.com/flarum/core/blob/eaac78650ffa0cf23321fcf624172bc1bd202036/src/Discussion/Command/StartDiscussionHandler.php#L74

On PATCH, we handle logic attribute by attribute and put the new values in the model only if it was provided.

https://github.com/flarum/core/blob/eaac78650ffa0cf23321fcf624172bc1bd202036/src/Discussion/Command/EditDiscussionHandler.php#L58-L62

We then run the validator against $model->isDirty() which takes care of validating only what changed.

https://github.com/flarum/core/blob/eaac78650ffa0cf23321fcf624172bc1bd202036/src/Discussion/Command/EditDiscussionHandler.php#L78

---

This issue is about extensions that have many attributes on a single model, or might not use models at all, and want to validate large amounts of attributes at once.

In this contexts extensions might want to use a variation of

$this->validator->assertValid($attributes);
$model->fill($attributes);

or

$model->fill($attributes);
$this->validator->assertValid($model->getAttributes());

Unfortunately here, if any attribute is missing (has no key in $attributes at all), it will never be validated, unlike what would happen with a Laravel Validator.

This forces extensions to enumerate attributes at some point, either by setting $model->attr1 = Arr::get($attrs, 'attrs1'); $model->attr2 = Arr::get($attrs, 'attrs2') before calling $model->getAttributes(). Or alternatively, build an array like $attrs = ['attr1' => Arr::get($attrs, 'attrs1'), 'attr2' => Arr::get($attrs, 'attrs2')] before calling the validator on it and then $model->fill($attrs) it to the model.

In those situations, the Laravel Validator becomes the better solution over the Flarum Validator, but this means we loose the validating event from Flarum.

My suggestion is to add a way for validators to selectively validate all, or validate existing data.

Maybe something like:

abstract class AbstractValidator
{
    // Can be overridden by classes to set their default
    protected $validateMissingKeys = false;

    // Can be used to switch the mode at run time
    public function validateMissingKeys($validate = true)
    {
        $this->validateMissingKeys = $validate;

        return $this;
    }

    // [...]

    protected function makeValidator(array $attributes)
    {
        $rules = $this->validateMissingKeys ? $this->getRules() : Arr::only($this->getRules(), array_keys($attributes));

        // [...]
    }
}

[stale[bot]]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. We do this to keep the amount of open issues to a manageable minimum. In any case, thanks for taking an interest in this software and contributing by opening the issue in the first place!

[clarkwinkelmann]

Not sure if anyone wanted to chime in on this? I don't think we necessarily need to do it, but I'd love if we could clarify this in the documentation somewhere.

[askvortsov1]

That API would make sense to me, and I'd be in favor of making this change so that AbstractValidator would be more useful for non-model data.

[stale[bot]]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. We do this to keep the amount of open issues to a manageable minimum. In any case, thanks for taking an interest in this software and contributing by opening the issue in the first place!

[askvortsov1]

Not sure if anyone wanted to chime in on this? I don't think we necessarily need to do it, but I'd love if we could clarify this in the documentation somewhere.

Maybe we could add $validateMissingKeys = false as an argument to assertValid instead of having it on the class level?

[SychO9]

I didn't know we had an issue about this, definitely something we should improve, and when we do we can change the SettingsValidator to be as simple as '*' => 'max:...