[6.0] Allow mutators and accessors to execute casting #29424

browner12 · 2019-08-05T22:58:41Z

There are 2 parts to a piece of data. There is the value and the type. You can have simple data like an integer of 12, or you can have complex data like a Collection of ['red', 'green', 'blue']. Laravel's casting system strictly handles the type aspect of the data, converting back and forth between native PHP types and valid data store types. Eloquent's mutators and accessors tend to be more on the value side of things, although they can dip into type manipulation, especially when a native casting doesn't exist.

A problem occurs, however, if we try to use castings together with mutators or accessors. Mutators and accessors 'short circuit' any other behavior in the setAttribute() and getAttribute() methods, meaning casting can no longer occur. This PR solves that problem by allowing casted attributes to work together with mutators and accessors.

Rather than returning immediately when a cast, mutator, or accessor is hit, we allow the data to be passed through the pipeline, and be manipulated by each piece.

For setting fields:

If a mutator exists, we mutate and pass along the value.
If the field is a date attribute, we convert the value to our desired date format and pass along the value.
If the field is JSON castable, we cast it to JSON, and pass along the value.
We take the final value and set the field on the internal attributes property.

For getting fields:

Retrieve the value from the internal attributes property.
If the field is castable, we cast it to the desired type and pass along the value.
Otherwise if the field is a date, we cast it to a Carbon object and pass along the value.
Now that we have the data in our desired PHP type, if there is an accessor we will pass the data through in to be adjusted as needed.
Finally, we return it to the user.

By following this pipeline/middleware style pattern it allows the developer to avoid duplicating casting logic, and focus on manipulating the value of their data.

Usage

There will be some minor changes developers will have to make to their Models. Currently in mutators, the developer will directly set the value of a field on the internal attributes property.

class User extends Model
{
    public function setPasswordAttribute($value)
    {
        $this->attributes['password'] = sha1($value);
    }
}

Now you will return values from mutators. I think one nice side benefit of this is the programmer no longer needs to be concerned with the internal workings of the Model. Previously, the user needed to know Eloquent stored data in the attributes array. Also, by exposing that internal representation to users, we are now more bound to it, making it more difficult for us to refactor it.

class User extends Model
{
    public function setPasswordAttribute($value)
    {
        return sha1($value);
    }
}

Keen eyed observers will note that previously the key we set on the model did not necessarily need to match our mutator. In fact, some of our tests even do this.

class User extends Model
{
    public function setPasswordAttribute($value)
    {
        $this->attributes['hashed_password'] = sha1($value);
    }
}

However, you can still do this if you like. By default, no return will set the 'password' attribute to null, or you can still be explicit about a return value to set the 'password' property, too.

class User extends Model
{
    public function setPasswordAttribute($value)
    {
        $this->attributes['hashed_password'] = sha1($value);

        return '';
    }
}

I would go one step further and say you should not directly set something on the attributes property.

class User extends Model
{
    public function setPasswordAttribute($value)
    {
        $this->hashed_password = sha1($value);

        return '';
    }
}

Benefits

Allows business logic to be pulled into the Models, out of places like Controllers or Jobs.
The developer does not have to reimplement casting logic for attributes that have mutators or accessors.
Developer does not access the internal Model property attributes directly, making possible refactors less damaging.

Backwards Compatibility Changes

2 changes will need to be made by the developer when upgrading to Laravel 6.0 for their code to work with this change.

Update any mutators to return a value, rather than setting it directly on the attributes property. If they don't, their field will be set as null.
Remove any casting logic from mutators or accessors of cast fields, to avoid duplicate casting.

Examples

Let's say we have a Product model with a colors attribute. The colors are cast as a Collection. We have a textarea where users can enter comma separated values. We need to explode the string into separate colors and trim any whitespace. Previously we may have handled this in our Controller.

class ProductController
{
    public function store(Request $request)
    {
        $product = new Product;
        $product->colors = array_map('trim', explode(',', $request->input('colors')));
        $product->save();
    }

    public function update(Request $request, Product $product)
    {
        $product->colors = array_map('trim', explode(',', $request->input('colors')));
        $product->save();
    }
}

Now we can move that duplicated logic into the Model:

class Product extends Model
{
    protected $casts = ['colors' => 'collection'];

    protected function setColorsAttribute($value)
    {
        if(is_array($value)) {
            return array_map('trim', $value);
        }

        return array_map('trim', explode(',', $value));
    }
}

I understand this would be an impactful PR due to how important Eloquent is to everything we do. More than happy to answer any questions, or address any concerns about it. I'm encouraged by the fact I had to change very little in the tests for this to work. Aside from the additional test I added, I only needed to change 2 lines. One to return a value from the mutator, and one for a mutator where the field name did not match the actual changed property.

Thanks!

currently mutators and accessors 'short circuit' any other behavior in the `setAttribute()` and `getAttribute()` methods. this commit allows fields to be cast **AND** mutated or accessed.

apparently StyleCI didn't like me doing `return null`. Even though it's valid here, I'll just return an empty string instead, and check for that.

mfn · 2019-08-06T08:26:56Z

So it's impact is the same as #29392 ?

The concern regarding null has been turned into an acknowledged breaking change then.

I come to like the idea, but the amount of breaking change is truly massive IMHO: if you have lots of setters and you might miss one and have imperfect tests, you will loose data 💥

What about third party packages providing traits for models with setters? The would go unnoticed maybe…

browner12 · 2019-08-06T14:20:27Z

The impact is similar, but by switching to returning values instead of setting the attributes, we do get around the null issue.

It is a breaking change, but I wouldn't call it massive. I would call it high impact, though, which is something we have every time we do a major upgrade.

I believe pretty standard mutator tests will cover the changes here. If the user fails to update their mutator, the password property will be set to null, and this test will fail.

User extends Model
{
    public function setPasswordAttribute($value)
    {
        $this->attributes['password'] = bcrypt($value);
    }
}

UserTest extends TestCast
{
    public function test_password_mutator()
    {
        $user = factory(User::class)->make([
            'password' => 'secret',
        ]);

        $this->assertTrue(password_verify('secret', $user->password));
    }
}

As long as we document the change well, I believe the actual work required to update the mutators is minimal.

Just like with all major releases, 3rd party packages will have to make appropriate changes to make sure they are compatible with the new framework code.

IMO, the benefits we get outweigh the cost.

taylorotwell · 2019-08-06T14:56:09Z

Before we go any further down this road was simply calling ->castAttribute from your mutator not possible and I missed that or did you just want to avoid that for some reason?

browner12 · 2019-08-06T16:16:23Z

You can manually handle this in your mutators and accessors. For the mutator it would actually be either castAttributeAsJson() or fromDateTime(), since those are the only types we convert when setting values. For the accessor we call castAttribute() or asDateTime(), as those are the methods that converts back to native PHP types.

However, I do want to avoid this for 2 main reasons. I believe it unnecessarily exposes internal workings of the Model to the user, and it simplifies our accessors/mutators by utilizing existing systems, rather than forcing the user to reimplement them.

taylorotwell · 2019-08-11T14:46:19Z

Personally I'm just not comfortable breaking all Eloquent setters right now. You can already avoid duplicate logic in a few different ways... personally if it were me I would probably just have a Product->setColorsFromString($string) method that is called from both controller methods.

taylorotwell · 2019-08-11T14:47:07Z

To reconsider this a bit... what if I define a setter and I don't want the casting to run after I return something? Is it possible? Maybe such a scenario doesn't make sense... just thinking out loud.

browner12 · 2019-08-11T17:11:46Z

Yah, I had thought about that a little as well. I don't know that it's possible, but I also struggled to think of a situation when I would want that behavior. In my mind, keeping the types consistent is really important, so if I cast a field, I always want it cast.

Maybe it would be beneficial to bring some focus to this PR via Twitter, so we can get some more perspectives?

garygreen · 2019-08-13T23:05:20Z

The philopshy behind setters has always been that it's essentially an escape hatch to add your own logic for setting a properties value. To have it mutated in some form before the setter receives it goes against this core behaviour that has been around in Laravel for a long time.

It may seem too verbose to access ->attributes or make manual calls to methods on the model like ->castAttributeAsJson - in reality I think most developers who are using setters know exactly how attributes work and have custom logic in casting/handling that raw value from the database.

The impact this PR would have on many applications is pretty high and doesn't really add a huge amount of value imo. I'm not sure I even agree with returning a value from a setter is any clearer or better than just setting it attributes yourself - the function is a setter so your expected to set something in the function and not defer it onto something else to set -- that's a bit odd. It's not really a setter at that point, it's a getter.

That's just my perspective on it anyway, would be interesting to hear what others think. 😃

BrandonSurowiec · 2019-08-14T12:08:31Z

Doesn't this break setters that aren't connected to a database column? In the example of setPasswordAttribute, its very existence sets $attributes['password']. If I use protected $guarded = []; and password is not in my DB schema, wouldn't this throw an error and force me to now guard the password column?

class User extends Model
{
    protected $guarded = ['password']; // would now need to do this?

    public function setPasswordAttribute($value)
    {
        $this->hashed_password = sha1($value);

        return '';
    }
}

browner12 · 2019-08-14T15:43:18Z

@garygreen

The philopshy behind setters has always been that it's essentially an escape hatch to add your own logic for setting a properties value.

I want to point out a part of your comment there. "setting a properties value". It is still exactly that. The mutator works to update the value of your data, but not the type. The only thing this change will do is after you make your value changes, it will make sure the data is in the correct format for storage in your database, just like it would do for the field if you did not have a mutator for the attribute. For setters, this only affects date and JSON castable fields.

I don't think we're changing the core philosophy here, because Laravel has never, and I'm guessing will never, change the value of your data unless you specifically choose to in your mutator. Laravel only affects the type, and that's still only if you define it in your casts attribute. I'm trying to bring consistency to how our magic methods work.

with normal PHP setters where we are directly updating a class property, I'd agree with you, we should be responsible for setting the property.

class Product
{
    private $price;

    public function setPrice($price)
    {
        $this->price = $price;
    }
}

but with Laravel we are using a Framework defined property (attributes) and not even calling these "setters" directly, but using magic methods to handle it. Because of this tight integration, I would say that Laravel carries a little more responsibility for setting data.

class Product
{
    protected function setPriceAttritube($price)
    {
        $this->attributes['price'] = $price;
    }
}

@BrandonSurowiec interesting point. I've never understood why exactly someone would need to change the name of the field, so yes, we would either need to discourage that use, or explain how to get around it.

BrandonSurowiec · 2019-08-14T16:04:14Z

@browner12 I've used it for various things. It can be a nice helper for associating a model to a morphable column. Or if capturing a length of time, like a setClockAttribute() or setHoursAttribute() that then converts and saves the data to $this->attributes['minutes']. I've also used it for capturing temporary properties on a model, like items for a pending order.

browner12 · 2019-08-14T17:14:32Z

Can you help me understand a little better?

When you say "morphable column", are you talking about a relationship? Can you explain this one a little more, maybe with some code?

Is this what you do with your minutes/hours?

class Model
{
    protected function setHoursAttribute($value)
    {
        $this->attributes['minutes'] = $value * 60;
    }
}

$model = new Model;
$model->hours = 5;
echo $model->minutes;  // 300

Can you explain the temporary variables comment a little more? Wouldn't you run into the same issue where your temporary variables do not match up with fields in the DB?

BrandonSurowiec · 2019-08-14T19:23:24Z

Yes, a relationship. Here's a belongsTo example:

class SubscriptionsController extends Controller
{
    public function store()
    {
        $podcast = Podcast::published()->findOrFail(request('podcast_id'));

        $subscription = Subscription::create([
            'user' => Auth::user(),
            'podcast' => $podcast,
        ]);

        return $subscription;
    }
}

...

class Subscription extends Model
{
    protected $guarded = [];

    public function user()
    {
        return $this->belongsTo(User::class);
    }

    public function podcast()
    {
        return $this->belongsTo(Podcast::class);
    }

    public function setUserAttribute($user)
    {
        $this->user()->associate($user);
    }

    public function setPodcastAttribute($podcast)
    {
        $this->podcast()->associate($podcast);
    }
}

The associate methods in this case sets the user_id, podcast_id and user/podcast relation on the Subscription model.

Without the setters there are 2 additional queries to reload the user and podcast relations for the JSON request IF there's no call to ->setRelation() after the fact. (Which probably isn't a big deal, but shows the advantage of the setters.)

class SubscriptionsController extends Controller
{
    public function store()
    {
        $podcast = Podcast::published()->findOrFail(request('podcast_id'));

        $subscription = Subscription::create([
            'user_id' => Auth::id(),
            'podcast_id' => $podcast->getKey(),
        ])->setRelations([
            'user' => Auth::user(),
            'podcast' => $podcast,
        ]);

        return $subscription;
    }
}

...

derekmd · 2019-08-14T20:55:03Z

public function setRequestAttribute(Request $request)
{
    $this->fill([
        'impersonator_id' => $request->impersonatorId(),
        'ip' => $request->ip(),
        'user_agent' => $request->formatUserAgent(),
    ]);
}

Calling that shouldn't leave a ['request' => null] item in the model attributes payload. This is a typical pattern used to copy multiple model attributes into another model when keeping an audit trail to cover when the source model is eventually updated/delete.

browner12 · 2019-08-14T21:13:46Z

I'm having a little trouble understanding your use case. Could you provide some Controller code to help explain it?

My gut reaction is this feels like a hijacking of what the mutators are intended to be. This behavior feels like it should be a method to me.

class User
{
    public function saveAuditTrail(Request $request)
    {
        $this->fill([
            'impersonator_id' => $request->impersonatorId(),
            'ip' => $request->ip(),
            'user_agent' => $request->formatUserAgent(),
        ]);
    }

    protected function setRequestAttribute(Request $request)
    {
        $this->fill([
            'impersonator_id' => $request->impersonatorId(),
            'ip' => $request->ip(),
            'user_agent' => $request->formatUserAgent(),
        ]);
    }
}

$user = new User;
$user->saveAuditTrail($request);

vs

$user->request = $request;

garygreen · 2019-08-15T01:45:59Z

I feel like this PR is attempting to do two different things:

Cast attributes before passing on setSomeAttribute()
Add a "shortcut" for setting a value by allowing return of a value from a setter.

I can get behind 1 - it would seem like a sensible default to always cast attributes before passing them onto a setter because casts are considered more "low level" and should take priority over setters.

So example code from one of our projects:

class User {

    protected $casts = [
        'genders' => 'array',
        'core_gender' => 'int',
        'other_genders' => 'array',
    ];

    // With this PR (29424) you would be able to type hint an array?
    // Presumably it would auto cast to an array before being passed to the setter, so you can always guanratee it's an array or null.
    public function setGendersAttribute(?array $genders)
    {
        $genders = new Genders($genders);
        $this->attributes['core_gender'] = $genders->filterCore();
        $this->attributes['other_genders'] = $genders->filterOther();
    }

}

$user = new User;
$user->genders = '[1,3]';
$user->core_gender; // 1 (male)
$user->other_genders; // [3] (Transsexual)

This PR could also relate to a long ongoing and numerous attempts of "custom casting" #18229

If setters always received a casted (or custom casted) property, it could open up the possibility of more strongly typed code:

class User {

    protected $casts = [
        'genders' => Gender::class,
        'core_gender' => 'int',
        'other_genders' => 'array',
    ];

    public function setGendersAttribute(Gender $genders)
    {
        $this->attributes['core_gender'] = $genders->filterCore();
        $this->attributes['other_genders'] = $genders->filterOther();
    }
}

Could this be buried behind a $user->setGenders() method? Sure. But the expressive nature of eloquent model by allowing filling of property values via ->fill() or setting as a property is a pleasant syntax.

browner12 · 2019-08-15T04:28:10Z

Hey @garygreen, thanks for all your feedback. I do want to clear up one significant point, though.

The purpose of casting is to translate data between PHP data types and types that can be stored in a Database. Not to cast them from one PHP type to another PHP type.

Casting does not happen until after a setter method has run, not before. And something to note, the only attributes that actually get casted are ones that are either dates or some type of JSON castable ('array', 'json', 'object', 'collection'). Things like 'integer', 'string', etc do not get touched at all when setting them.

taylorotwell · 2019-08-15T21:23:02Z

I still think I'll just hold off on this. I fear it will cause too much problems on upgrade. Thanks.

browner12 added 2 commits August 5, 2019 15:48

allow mutators and accessors to execute casting

dcd9930

currently mutators and accessors 'short circuit' any other behavior in the `setAttribute()` and `getAttribute()` methods. this commit allows fields to be cast **AND** mutated or accessed.

Update DatabaseEloquentModelTest.php

7265822

apparently StyleCI didn't like me doing `return null`. Even though it's valid here, I'll just return an empty string instead, and check for that.

taylorotwell closed this Aug 11, 2019

taylorotwell reopened this Aug 11, 2019

GrahamCampbell changed the title ~~[6.0] allow mutators and accessors to execute casting~~ [6.0] Allow mutators and accessors to execute casting Aug 13, 2019

garygreen mentioned this pull request Aug 15, 2019

[6.0] Give ability to cast from mutator #29584

Closed

taylorotwell closed this Aug 15, 2019

browner12 deleted the accessors-mutators branch November 21, 2019 23:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[6.0] Allow mutators and accessors to execute casting #29424

[6.0] Allow mutators and accessors to execute casting #29424

browner12 commented Aug 5, 2019

mfn commented Aug 6, 2019

browner12 commented Aug 6, 2019

taylorotwell commented Aug 6, 2019

browner12 commented Aug 6, 2019

taylorotwell commented Aug 11, 2019

taylorotwell commented Aug 11, 2019 •

edited

Loading

browner12 commented Aug 11, 2019

garygreen commented Aug 13, 2019 •

edited

Loading

BrandonSurowiec commented Aug 14, 2019 •

edited

Loading

browner12 commented Aug 14, 2019

BrandonSurowiec commented Aug 14, 2019 •

edited

Loading

browner12 commented Aug 14, 2019

BrandonSurowiec commented Aug 14, 2019 •

edited

Loading

derekmd commented Aug 14, 2019

browner12 commented Aug 14, 2019

garygreen commented Aug 15, 2019

browner12 commented Aug 15, 2019

taylorotwell commented Aug 15, 2019

[6.0] Allow mutators and accessors to execute casting #29424

[6.0] Allow mutators and accessors to execute casting #29424

Conversation

browner12 commented Aug 5, 2019

Usage

Benefits

Backwards Compatibility Changes

Examples

mfn commented Aug 6, 2019

browner12 commented Aug 6, 2019

taylorotwell commented Aug 6, 2019

browner12 commented Aug 6, 2019

taylorotwell commented Aug 11, 2019

taylorotwell commented Aug 11, 2019 • edited Loading

browner12 commented Aug 11, 2019

garygreen commented Aug 13, 2019 • edited Loading

BrandonSurowiec commented Aug 14, 2019 • edited Loading

browner12 commented Aug 14, 2019

BrandonSurowiec commented Aug 14, 2019 • edited Loading

browner12 commented Aug 14, 2019

BrandonSurowiec commented Aug 14, 2019 • edited Loading

derekmd commented Aug 14, 2019

browner12 commented Aug 14, 2019

garygreen commented Aug 15, 2019

browner12 commented Aug 15, 2019

taylorotwell commented Aug 15, 2019

taylorotwell commented Aug 11, 2019 •

edited

Loading

garygreen commented Aug 13, 2019 •

edited

Loading

BrandonSurowiec commented Aug 14, 2019 •

edited

Loading

BrandonSurowiec commented Aug 14, 2019 •

edited

Loading

BrandonSurowiec commented Aug 14, 2019 •

edited

Loading