validating icon indicating copy to clipboard operation
validating copied to clipboard

$model->isValid() doesn't fire validating event

Open adamthehutt opened this issue 7 years ago • 9 comments

Not sure if I'm doing something wrong or if this is a bug or intentional...

But when I call $model->save() on a model that uses the ValidatingTrait, the eloquent.validating event is fired before validation, which allows for some cleanup, setting of default properties, etc.

However, if I just call $model->isValid(), the eloquent.validating event does not seem to be triggered, so validation fails (when it would have succeeded in the context of saving).

Being able to call isValid() without saving would be particularly useful in tests, since there's no need to hit the database.

From looking at the code, it appears that both ValidatingTrait and ValidatingObserver have methods called performValidation(). The ValidatingObserver one invokes the events, while the trait one doesn't.

Any idea how to fix this?

adamthehutt avatar Jul 21 '16 12:07 adamthehutt

I suppose in this context you would expect that the validating events fire in response to the isValid() and isInvalid() methods as well as in response to a save action, however the way they're implemented at the moment is more as hooks into the saving pipeline.

It could definitely be argued whether this is right or not, but I would be hesitant to change this without a new major version as it could be backwards incompatible. The alternative would be to introduce new events that are fired by the isValid() and isInvalid() methods, but that feels a little counterintuitive.

My view would probably be that it's incorrect to use the pre-validating event as a hook to make your model valid. I would probably expect that if you needed to make certain alterations to bring a model to a valid state then you would do that through Eloquent mutators or other events.

I hope this helps, let me know if I can assist in any other way. I'll leave this ticket open as it's something that could be revisited for another major version release.

dwightwatson avatar Jul 21 '16 12:07 dwightwatson

Thanks, I appreciate the reply and it makes sense. I'll look into the idea of using mutators for this; that hadn't occurred to me.

adamthehutt avatar Jul 21 '16 12:07 adamthehutt

Actually, if you could elaborate on the idea of using eloquent mutators that would be really helpful. At first I thought you were talking about accessors - creating something like getRequiredFieldAttribute that would set it to the valid default data if it didn't exist. But apparently that doesn't work, since the validation is seemingly performed directly on $model->attributes, bypassing any special accessors.

adamthehutt avatar Jul 21 '16 13:07 adamthehutt

Sure - I'm assuming that when you're hooking into the validating your event you're generating changes on the model that change it into a valid state. Instead, you might want to perform this "self-correcting" functionality when a relevant change is made.

I'm making up an example here, I'm not sure what your exact use-case is, but let's say we have an address attribute that we would actually want to break up into it's components and store them as separate attributes in order to make our model valid. We could do something like this.

public function setAddressAttribute($value)
{
    // Split the address into components using a space as a delimiter.
    $components = explode(' ', $value);

    $this->attributes['street_number'] = $components[0];
    $this->attributes['street'] = $components[1];
    $this->attributes['suburb'] = $components[2];
}

Totally arbitrary example, but I hope it highlights what I mean. If you just have another method to call that will fix data up you could do that in a mutator as well.

public function setFirstNameAttribute($value)
{
    // Set the attribute first.
    $this->attributes['first_name'] = $value;

    // Call whatever method you were calling on the validating event.
    $this->preValidationHook();
}

As you see with a mutator we need to manually and explicitly set the value in the attributes array, but we then have the opportunity to do other things. Instead of hooking into the validating event you might find it better to manually call your helper method whenever a relevant attribute changes. I hope this is helpful!

dwightwatson avatar Jul 21 '16 13:07 dwightwatson

Thanks. I really appreciate it. My use case is a little different, and maybe I've over-engineering here. But basically I'm dealing with default values that need to be set when a new object is created. For example, a model might have a column for "uuid" that needs to generate a UUID before initial insertion into the database. I could do that by hooking into the "creating" event, but that is fired after validation. That's why I tried hooking into the validating event and ran into the issue above.

The workaround is simply not to mark the uuid attribute as required, but that seems less than ideal.

What I'd really love is something like a "constructed" event in Eloquent, but after extensive Googling I can't find anyone else who has even suggested the idea, so I'm guessing I'm all alone on that one...

adamthehutt avatar Jul 21 '16 13:07 adamthehutt

Hm, I see what you're going for. Can't say I've done anything like that in the past, but have you tried creating a UUID in the constructor or in the boot() method?

dwightwatson avatar Jul 21 '16 13:07 dwightwatson

I can override __construct(), that's true. But that feels a bit brittle to me.

I'm pretty new to Laravel, I admit. But I couldn't figure out how to do it during boot since it's a static method without access to $this.

adamthehutt avatar Jul 21 '16 13:07 adamthehutt

Ah yeah, you're right - that's a bit tricky with the boot method. There definitely doesn't appear to be other events that would be helpful to hook into either. Normally for a default value like that I'd have the database column look after that for me, but I don't know if a database can do that with a UUID. If I was you I'd probably end up going with popping it in the constructor then, I don't think it's semantically wrong - setting a UUID is part of the initialisation process for your model it seems.

dwightwatson avatar Jul 21 '16 13:07 dwightwatson

Okay, thanks. That's probably what I'll do then. Thanks for all your help with this.

adamthehutt avatar Jul 21 '16 13:07 adamthehutt