dwightwatson / validating

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

Complete Laravel-style implementation of validating events #40

Closed dwightwatson closed 10 years ago

dwightwatson commented 10 years ago

Right now we have a simple implementation of validating events which will get you by if you need it. However, I'd like to better match up with the naming convention of events in Laravel itself. @dalabarge goes into detail in this comment.

This is something I'd like for when we hit version 1.

dwightwatson commented 10 years ago

Looking to see laravel/framework#5040 to get merged and tagged to provide necessary functionality.

markalanevans commented 10 years ago

Hey, so it looks like this allows me to use my model observer classes?

I need the ability to add into my model observers, custom model related events.

As far as I can tell, the current event mechanism is generic, its not specific to the Model?

Looks like this model request would allow me to do:

MyModel{
 public function validating($model){
  //Do some stuff to the model....before validating
}
 public function validated($model){
  //Do some stuff to the model....after validating
}

}

Is that correct?

dwightwatson commented 10 years ago

This is all on the dev-develop branch and requires some code to be merged into Laravel.

If you read up on model events in the docs you can see that you can register event listeners on a model observer, or in the boot method of the model. So, when this issue is closed, you can do one of two things...

class Model extends Eloquent {
    public static function boot()
    {
        parent::boot();

        static::validating(function($model)
        {
            // Do stuff
        });
    }
}

Alternatively, model observer classes will do the trick also.

markalanevans commented 10 years ago

Great.

ETA on this PR? We are moving off of Ardent this week and we use Model observers, but not being able to tie into model validation events requires us to do a fair amount of hacks all over the place.

dwightwatson commented 10 years ago

I just gave a shout-out to Taylor to take a look at the PR but unfortunately I can't provide an ETA - I'm sure he's very busy working on 4.3 at the moment (which is where we might end up seeing this functionality instead).

markalanevans commented 10 years ago

ok.

These hacks i'm doing might have to be a temp fix. I spoke w/ Jeffrey Way yesterday and he is pretty big on removing the model based validation, which has a lot of merit.

Perhaps its best that I decouple much of the validation now to get ready for 4.3.

I appreciate your response Dwight.

Thanks very much,

-Mark

On Sun, Sep 7, 2014 at 5:20 PM, Dwight Watson notifications@github.com wrote:

I just gave a shout-out to Taylor to take a look at the PR but unfortunately I can't provide an ETA - I'm sure he's very busy working on 4.3 at the moment (which is where we might end up seeing this functionality instead).

— Reply to this email directly or view it on GitHub https://github.com/dwightwatson/validating/issues/40#issuecomment-54766234 .

Regards,

Mark Evans Cell: 805.340.7390

*Note: Please excuse a 2 - 4 day response delay... It's not that i am ignoring you... I am just really busy. If its urgent call me.

CTO - ShopGenius - www.shopgeniusapp.com

Vice President - Catalyst For Thought Invest In yourself, and your community http://www.catalystforthought.org

dwightwatson commented 10 years ago

Yeah, I'm probably going to be moving in that direction too now that it's built into the next version. It's nice to see that there will finally be an agreed-upon way to perform validation in the framework. However I still see some benefit to having your model validate itself - even if someone on your team makes a validation mistake somewhere in your codebase (a form somewhere forgets a field) you can still be assured the data in your database will be valid.

I wonder why model-based validation never really took off in the Laravel community, I find it to be one of the nicest features in Rails. But who knows, maybe Laravel 4.3 is onto something better here!

dalabarge commented 10 years ago

@dwightwatson as an upstream consumer of this package, I generally agree with you. I'm finding that the new 4.3 validation method is by far the best way to do form validation, meanwhile simpler, least-common-denominator database validation is more natural to be on the model. For instance acceptance type validation has no place on the model and for that matter neither do require validations unless the database strictly requires the value at all times (e.g.: foreign keys, etc.) So while the current state of this package provides a centralized place to do all the validation, it does get awkward fast and I propose minimizing some of the capabilities of the model validation perhaps completely dropping support for rulesets other than those needed by the observer class (e.g: creating, updating, etc.).

markalanevans commented 10 years ago

Hey Dwight,

While i'm playing around w/ your lib, I have the need to intercept the validation cycle and update a custom rule to act just like the unique rule. In my case, I have a validation rule "notprimary" which needs to ensure that the parent_id of a record does not = the id of the same record.

Is there an appropriate place to do that with you package?

On Sun, Sep 7, 2014 at 10:07 PM, Dwight Watson notifications@github.com wrote:

Yeah, I'm probably going to be moving in that direction too now that it's built into the next version. It's nice to see that there will finally be an agreed-upon way to perform validation in the framework. However I still see some benefit to having your model validate itself - even if someone on your team makes a validation mistake somewhere in your codebase (a form somewhere forgets a field) you can still be assured the data in your database will be valid.

I wonder why model-based validation never really took off in the Laravel community, I find it to be one of the nicest features in Rails. But who knows, maybe Laravel 4.3 is onto something better here!

— Reply to this email directly or view it on GitHub https://github.com/dwightwatson/validating/issues/40#issuecomment-54777113 .

Regards,

Mark Evans Cell: 805.340.7390

*Note: Please excuse a 2 - 4 day response delay... It's not that i am ignoring you... I am just really busy. If its urgent call me.

CTO - ShopGenius - www.shopgeniusapp.com

Vice President - Catalyst For Thought Invest In yourself, and your community http://www.catalystforthought.org

dalabarge commented 10 years ago

@markalanevans one such solution would be to add the rules to the updating ruleset since this rule sounds like it should apply to only updates. You can set the ruleset using a helper method on the base model by calling it from the constructor. I do this for rules like in when the options change depending on a situation. It's not the most elegant but it's a solution.

Another option would be just to overwrite a particular method on your custom model that the validating trait implements to update the rule prior to validation. That's one of the benefits of using traits to begin with: just create another trait that you include on your models that use your notprimary rule in order to overwrite a previous trait's method.

Finally another solution would be to register a custom observer that updates the rule before validating. This might be the best solution of them all as it only requires overwriting the bootValidatingTrait method on the model to swap out the observer. You can then extend the base ValidatingObserver with your custom handler for updating().

markalanevans commented 10 years ago

Thanks Daniel!

I will start w/ the last approach now. Thanks very much I appreciate the recommendations.

-Mark

On Tue, Sep 9, 2014 at 7:28 AM, Daniel LaBarge notifications@github.com wrote:

@markalanevans https://github.com/markalanevans one such solution would be to add the rules to the updating ruleset since this rule sounds like it should apply to only updates. You can set the ruleset using a helper method on the base model by calling it from the constructor. I do this for rules like in when the options change depending on a situation. It's not the most elegant but it's a solution.

Another option would be just to overwrite a particular method on your custom model that the validating trait implements to update the rule prior to validation. That's one of the benefits of using traits to begin with: just create another trait that you include on your models that use your notprimary rule in order to overwrite a previous trait's method.

Finally another solution would be to register a custom observer that updates the rule before validating. This might be the best solution of them all as it only requires overwriting the bootValidatingTrait method on the model to swap out the observer. You can then extend the base ValidatingObserver with your custom handler for updating().

— Reply to this email directly or view it on GitHub https://github.com/dwightwatson/validating/issues/40#issuecomment-54977013 .

Regards,

Mark Evans Cell: 805.340.7390

*Note: Please excuse a 2 - 4 day response delay... It's not that i am ignoring you... I am just really busy. If its urgent call me.

CTO - ShopGenius - www.shopgeniusapp.com

Vice President - Catalyst For Thought Invest In yourself, and your community http://www.catalystforthought.org

dwightwatson commented 10 years ago

Just an update on this, the PR has now been accepted into Laravel 4.2. Now just need to wait on the next tagged release (which should be pretty soon).

markalanevans commented 10 years ago

Sweet!!! perhaps I can pull in dev master?

Sent from my iPhone

On Sep 10, 2014, at 5:39 PM, Dwight Watson notifications@github.com wrote:

Just an update on this, the PR has now been accepted into Laravel 4.2. Now just need to wait on the next tagged release (which should be pretty soon).

— Reply to this email directly or view it on GitHub https://github.com/dwightwatson/validating/issues/40#issuecomment-55204784 .

dwightwatson commented 10 years ago

I haven't written the code to register the events with Eloquent (it should only take a minute) because it won't work until I can pull in the new Laravel. As soon as it's ready this will go over to master and I'll tag 0.10.0.

markalanevans commented 10 years ago

Awesome. Let me know because it will make our life way easier and we can upgrade to 4.2 the moment this is done.

Thanks so much!

Sent from my iPhone

On Sep 10, 2014, at 10:00 PM, Dwight Watson notifications@github.com wrote:

I haven't written the code to register the events with Eloquent (it should only take a minute) because it won't work until I can pull in the new Laravel. As soon as it's ready this will go over to master and I'll tag 0.10.0.

— Reply to this email directly or view it on GitHub https://github.com/dwightwatson/validating/issues/40#issuecomment-55219668 .

dwightwatson commented 10 years ago

Okay, the stuff I just did is totally wrong. The model events need to be registered before I call the static observe method.

@dalabarge, might you have any suggestion as to where I should add the observable events? We've now got addObserveableEvents on the Model, but not sure how to best hook into the model instance.

dalabarge commented 10 years ago

As best as I can tell you would register your observables using the $observables model property. Just add validating and validated to it as an array and then your observer model needs to implement them. So in the case of a model hooking into those events then they will need to register their observer in a boot method of their trait or as an appendage to the boot method on the model after bootTraits is called.

Make sure your validated event has the model and the validation status in the payload. That way people can take different actions based on valid/invalid state.

Sent from my iPhone

On Sep 15, 2014, at 12:45 AM, Dwight Watson notifications@github.com wrote:

Okay, the stuff I just did is totally wrong. The model events need to be registered before I call the static observe method.

@dalabarge, might you have any suggestion as to where I should add the observable events? We've now got addObserveableEvents on the Model, but not sure how to best hook into the model instance.

— Reply to this email directly or view it on GitHub.

dwightwatson commented 10 years ago

Awesome, thanks for your help. Shame I wasn't able to put the new addObservableEvents() and so on to use, but I've documented the approach to placing the observables in the model property.