Skip to content
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

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

Open
adamthehutt opened this issue Jul 21, 2016 · 9 comments
Open

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

adamthehutt opened this issue Jul 21, 2016 · 9 comments

Comments

@adamthehutt
Copy link

adamthehutt commented Jul 21, 2016

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?

@dwightwatson
Copy link
Owner

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.

@adamthehutt
Copy link
Author

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
Copy link
Author

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.

@dwightwatson
Copy link
Owner

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!

@adamthehutt
Copy link
Author

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...

@dwightwatson
Copy link
Owner

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?

@adamthehutt
Copy link
Author

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.

@dwightwatson
Copy link
Owner

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.

@adamthehutt
Copy link
Author

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants