dwightwatson / validating

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

Issue with Laravel 7.x custom Casts and validation #222

Closed Mechstud closed 4 years ago

Mechstud commented 4 years ago

Hi, there seems to be an issue with Laravel 7.x providing custom Casts feature: https://laravel.com/docs/7.x/eloquent-mutators#custom-casts

I defined a custom Cast, for eg: ZoneEnum, which implements CastsAttributes interface. Eg: North, South, East and West And in the get() method of this, I return an object of ZoneEnum, based on the value stored in the database. While setting the attribute, I mutate from a ZoneEnum object to its string value, in the set() method implementation.

The problem here is that in the getModelAttributes() function (refer: https://github.com/dwightwatson/validating/blob/master/src/ValidatingTrait.php#L131), you are getting attribute values utilizing the Accessor logic. This makes Laravel's code cast the (already mutated/set) value back to ZoneEnum object. This then fails the validation logic.

Also, I noticed that getAttributes() function alone should be enough; you dont really need to mutate again using getAttributeValue(), unless I am missing something ?

Originally posted by @Mechstud in https://github.com/dwightwatson/validating/pull/221#issuecomment-596195754

dwightwatson commented 4 years ago

Just been taking a look at this. I don't remember the original context, but it looks like we pass the mutated/cast values into the validator (with the exception of date casts - because the date validators don't know how to deal with Carbon instances). I believe the intention was that you should be validating a model "as-is" rather than the raw attributes underneath.

I've taken a look at writing the code to have the raw underlying values of a custom cast passed into the validator, but this introduces an inconsistency with existing casts. I don't think it makes sense that the validator gets the cast value of an "out of the box" caster, but gets the raw value of a "custom" caster.

Perhaps your validations need to be updated to work with the cast value rather than the original, but I can see how that's less than ideal. However, I think that's part of the compromise of doing model validations when the framework has leaned heavily toward request validations.

I hope this makes sense, but as it stands now this is expected behaviour and unlikely to change.

Mechstud commented 4 years ago

In that case, just like with date values can't be handled because of Carbon instances; you will need to modify the code to ignore Custom Casts as well (added in Laravel 7.x). Because Custom Casts can also return an object instead of string values (validator can work with primitive types only)

On Mon, Mar 9, 2020, 2:54 AM Dwight Watson notifications@github.com wrote:

Just been taking a look at this. I don't remember the original context, but it looks like we pass the mutated/cast values into the validator (with the exception of date casts - because the date validators don't know how to deal with Carbon instances). I believe the intention was that you should be validating a model "as-is" rather than the raw attributes underneath.

I've taken a look at writing the code to have the raw underlying values of a custom cast passed into the validator, but this introduces an inconsistency with existing casts. I don't think it makes sense that the validator gets the cast value of an "out of the box" caster, but gets the raw value of a "custom" caster.

Perhaps your validations need to be updated to work with the cast value rather than the original, but I can see how that's less than ideal. However, I think that's part of the compromise of doing model validations when the framework has leaned heavily toward request validations.

I hope this makes sense, but as it stands now this is expected behaviour and unlikely to change.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/dwightwatson/validating/issues/222?email_source=notifications&email_token=AETUOW4V4ISGVRXUBXHMEN3RGQEIHA5CNFSM4LDYUAJKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEOFB6OI#issuecomment-596254521, or unsubscribe https://github.com/notifications/unsubscribe-auth/AETUOW2ROGIXC4AC2JV4LITRGQEIHANCNFSM4LDYUAJA .

dwightwatson commented 4 years ago

An exception was made for Carbon instances, but not for other casts - all other casts are applied, and so are custom casts.

Mechstud commented 4 years ago

This comment is for other library users, and facing similar issue(s) with Custom Casts in Laravel 7.x, may utilize my fix. In my Base Model class, I overrided the getModelAttributes() function to use getAttributes() only. As, in Laravel 6+ (afaik), it is already mutated using set mutators. Here is a code snippet:

use Watson\Validating\ValidatingModel;

abstract class BaseModel extends ValidatingModel
{
    /**
     * @overload
     */
    public function getModelAttributes()
    {
        return $this->getAttributes();
    }
}

Your respective Concrete Model classes, will need to extend from the BaseModel class:

class Post extends BaseModel
{
  // other implementations
}