dwightwatson / validating

Automatically validating Eloquent models for Laravel
MIT License
968 stars 76 forks source link

MessageBag emtpy when using "Unique" rule on transaction #229

Open giulioprovasi opened 3 years ago

giulioprovasi commented 3 years ago

Hey, after digging a while on a weird issue, here what I get :

Model rule:

'name' => [
                'bail',
                'required',
                'string',
                'max:255',
                (new Rules\Unique('folders', 'name'))
                    ->ignore($this->id)
                    ->where('parent_id', $this->parent_id)
                    ->whereNull($this->getQualifiedDiscardedAtColumn())
                    ->whereNull($this->getQualifiedDeletedAtColumn())
                ,
            ],
 DB::transaction(static function() {
            $x = new Test();
            $x->name = 'test'; # name is not unique, so it will trigger Unicity rule
            $x->saveOrFail(); # fails correctly
        });

The issue is that in the following method

    /**
     * Throw a validation exception.
     *
     * @throws \Watson\Validating\ValidationException
     */
    public function throwValidationException()
    {
        $validator = $this->makeValidator($this->getRules());

        throw new ValidationException($validator, $this);
    }

The validator is not "triggered" with a ->fail(), so message bag is empty right now. Then, my transaction rollback and the unique rule does not fails anymore, so the validator has no messages :/

Won't it be better to do something like this :


    public function throwValidationException()
    {
        $validator = $this->makeValidator($this->getRules());

        throw new ValidationException(tap($validator)->fails(), $this);
    }

in order to populate the validator with errors before anything else in the app happens ?

dwightwatson commented 3 years ago

I see where you're coming from, however a concern here is that we would now run the rules twice - and this is especially an issue when talking about the Unique rule which would mean duplicate database queries. It's almost like you'd want to persist the last validator that was used (for an isValid or isInvalid call as an instance variable so you could pass it into the exception.

I'm not really sure that I'm going to tackle that any time soon, or that I'm keen to make what could be a substantial breaking change right now. In the meantime, overriding throwValidationException in your app would be the best bet if it solves your use-case. Hope this is helpful!

giulioprovasi commented 3 years ago

It's almost like you'd want to persist the last validator

Indeed this should be the proper behavior.

I will indeed override the throwValidationException method right now ;)