dwightwatson / validating

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

Purging of _confirmation, and other "_" attributes #73

Closed slakbal closed 10 years ago

slakbal commented 10 years ago

Hi,

Love trait, thank! But purging is bit of an issue.

Can someone please help. Seems like the trait doesn't purge _confirmation attributes from the model before saving to the persistence layer.

Any help would be appreciated.

Thanks!

dwightwatson commented 10 years ago

Don't pass _confirmation or any other _ attributes to your model?

slakbal commented 10 years ago

Hi,

thanks for the quick response. How do you use the the validation to check if e.g. password is confirmed?

E.g. is there a way to validate with the trait the Input::all() before one pass it to the model.

Thanks!

dwightwatson commented 10 years ago

I mean, feel like this would be out of the scope of this package - it's not a function of validation. This package will take _confirmation input into consideration for validation, but that's about it. If you want to pass all the input from a request into your Eloquent model I'd suggest that you have the mass assignment $fillable attribute added so that only the appropriate data is passed into it. You might take a look at esensi/model for purging.

dwightwatson commented 10 years ago

Ah sorry, I think you'll need to be on the dev-develop branch for the feature I mentioned above to work. It will make it into 0.10.0 shortly.

slakbal commented 10 years ago

Hi sorry but I don't see how using the $fillable attribute is going to solve the issue. Will have a look at the dev-develop branch. I however do think that "_confirmation" is absolutely 100% part of validation, most validation framework on e.g. Java sprint, etc. sees confirmation, etc. as part of their validation spec. It is also just in a few cases where this is always used. But even the Laravel validation class offer the "confirmed" rules for validation and since your package extend those base classes, does it mean that the package doesn't support all rules the framework supports? But thanks for the chat, I'll continue to research further.

slakbal commented 10 years ago

Sorry me again :) I guess I should have asked you also... assuming one doesn't pass the _confirmation property to the model, how would you solve e.g. the password confirmation test and still get the framework pre-defined error messages from the validation rules back in a MessageBag format?

dwightwatson commented 10 years ago

What happens during validation (on the develop branch) is this; it takes the attributes of the model and any _confirmation input items and passes them to a validator. You don't need to pass confirmation attributes to the model - the package knows to get them from the input instead. My argument is that this package is solely responsible for validating the input, not purging it from your model.

If you pass Input::all() to your model, your model is going to have (for example) both it's password and password_confirmation fields set - this is wrong. There is no password_confirmation field in your database. If you set $fillable on your model only the fillable properties will be set from Input:all(). This package will then take that data, as well as the _confirmation data from the request, and use it for validation.

I hope this makes sense. Basically you only pass the attributes that belong to the model to the model, the package will get the _confirmation fields it needs itself.

dwightwatson commented 10 years ago

You can pass the validation errors MessageBag using withErrors($model->getErrors()).

slakbal commented 10 years ago

Regarding the previous reply..... Ah!!! gotcha! That is even cooler!! I'm going to give that a go right now! Will feedback to you on the experience. Appreciated!

dwightwatson commented 10 years ago

No problems, let me know how you go!

slakbal commented 10 years ago

Works like a charm! Easy to use, etc.

And I think I understand your approach with it, to summarize

1) it doesn't do purging, becuase in the first place you don't want the "_attributes" to go into the model... which I tend to start liking, much cleaner models and according to one's model spec. So I agree it is definitely cleaner.

2) it will look into the request Input attribute and go an get the _attributes there and work with them.

I like!

Dammit, spend last night way too much time on this. :)

Thanks for this fun package, it makes my code much cleaner.

dwightwatson commented 10 years ago

Perfect! I might just try and put that version out ASAP, it's been waiting on a PR in Laravel for a little while but some of the new stuff like _confirmation fields really should be available for everyone in a tagged version.

slakbal commented 10 years ago

What do you mean with a pull request in Laravel...? Is the plan to get the trait into the Laravel framework directly?

dwightwatson commented 10 years ago

No, sorry if that was confusing. There is another new feature on the dev-develop branch that is half done and just waiting on a new feature in Laravel before it can be finished. It will make the events system in the validator work better.

slakbal commented 10 years ago

Cool, looking forward to that. So I'm going to ride dev-develop branch for the moment and will keep a lookout for the new tagged version.

Thanks for you help!

tortuetorche commented 10 years ago

There is another new feature on the dev-develop branch that is half done and just waiting on a new feature in Laravel before it can be finished. It will make the events system in the validator work better.

References: #40 and https://github.com/laravel/framework/pull/5040