A suggestion to improve performValidation() function

[ghost]

Hello, me again here!

Below follows the performValidation() function from ValidatingTrait.

    /**
     * Validate the model against it's rules, returning whether
     * or not it passes and setting the error messages on the
     * model if required.
     *
     * @param  array $rules
     * @return bool
     * @throws \Watson\Validating\ValidationException
     */
    protected function performValidation($rules = [])
    {
        $validation = $this->makeValidator($rules);
        $result = $validation->passes();
        $this->setErrors($validation->messages());
        return $result;
    }

I'm just wondering if we can improve this function to validate against related models in addition to the base model. Do you think this can be a good idea?

My use case is just because i want to simplify some of my controllers. Today, they are handling validation from my services this way:

$profile = ProfileService::update($profile, $request->all());

// handle validation for all models
if ($profile->isInvalid() || $profile->address->isInvalid()) {
    return redirect()
        ->back()
        ->withInput()
        ->withErrors(array_merge_recursive(
            $profile->getErrors()->toArray(), 
            $profile->address->getErrors()->toArray()
        ));
}

As you see, my ProfileService::update() function save profile's data into both base and related model. Both models are using this trait. I wanna keep it simple and remove that mess after redirect().

Something like:

$profile = ProfileService::update($profile, $request->all());

// handle validation for all models at once
if ($profile->isInvalid()) {
    $profile->throwValidationException();
}

[dwightwatson]

I see what you're going for, but I'm not sure this functionality belongs in the trait. As it is now the validation only works on the model itself, and Eloquent generally isn't very aware of all the associations that are possible. I don't think we would want to go through and validate all the loaded associations automatically as I suspect that would lead to unexpected behaviour.

I would expect in this case you might want to create a base service for your update classes that provides the helper functionality you want. I've just scaffolded something like this where your update services would expose the models/associations they want to validate and then you can provide accessors for the validation state/errors. If you really wanted I suppose you could also move the whole redirect()->back() up there somewhere as well.

class ProfileService extends BaseService
{
    public function models()
    {
        return [$this->profile, $this->profile->address];
    }
}

class BaseService
{
    public function isValid()
    {
        foreach ($this->models() as $model) {
            if ($model->isInvalid()) {
                return false;
            }
        }
       return true;
    }

    public function getErrors()
    {
        return collect($this->models)->map->getErrors()->flatten();
    }
}

[ghost]

Thanks for your attention buddy. It looks like a good idea, but after thinking for a while, I prefer to keep my services really SOLID. I think this is obligation of the validation class. In our case, who make validations are models.

I understood your concern about "unexpected behavior", maybe a better approach to this is implementing a new method, Ex: performValidationRecursively()

Can you keep this issue open while I look further?

[dwightwatson]

I would argue that the single responsibility of the validating trait as-is is to ensure the validity of the model it is on. Perhaps an additional trait that can be used to extend this functionality rather than adding it to the existing trait.

My concern with something like performValidationRecursively rather than being explicit about which records you want to validate is what happens when additional associations are loaded for whatever reason. Say for example you have a User model that has its Address association eager loaded, should that be validated as well? What if that Address association is only loaded conditionally? It could drastically change the validation context.

[ghost]

Yes, you're right. Be explicit on which relationships should be validate is the right way to achieve this. The "models" definition you suggested is almost there. I just think it should be declared at the model, not at the service. Furthermore, seems to be overwhelming create and use a new trait just for this. Maybe just adding some configuration to this trait can solve it all.

Maybe configuration should be defined this way:

    // ValidatingTrait.php

    /**
     * Define which relationships should be validate.
     *
     * @var array
     */
    protected $validateRelated = [];

    // User model at \App\User

    use ValidatingTrait;

    protected $rules = [
        // rules
    ]

    protected $validateRelated = ['address'];

I'm still thinking how to perform validations after this configuration is done. But i'll take a look in depth with real code to check for implications. I just need to align the idea with you, and of course, if you say that your package should not implement this feature, I stop right now and start thinking in alternative solutions.

Thanks again, your packages helps a lot.

[dwightwatson]

I guess from my perspective is that for a long time I've considered this feature-complete and that Laravel's FormRequests have become a viable alternative to model validation. The package has come this long without relationship validation and so I'm reluctant to make any significant changes that could impact existing functionality.

That's why my preference is to a secondary trait that sites on top, so you might have something like use ValidatingTrait, ValidatingRelationsTrait so it's explicitly opt-in and the logic is distinctly separated. I think that's the only way I'd consider it to be in this package. Hope this makes sense and happy to discuss it further, I appreciate the time you've put in to this.