[5.8] Less Greedy Mutator #29392

browner12 · 2019-08-02T18:18:11Z

this is a retry of #29375. it's much cleaner, and allows users to tie into existing mutator methods if they wish to utilize it.

currently mutators manually set values in the attributes property and return early in the setAttribute() method. This behavior will continue unaffected with this change.

class User
{
    public function setFirstNameAttribute($value)
    {
        $this->attributes['first_name'] = strtolower($value);
    }
}

However, now you can return a value from the mutator method instead, which will cause it to also run through any additional logic in the setAttribute() method.

class User
{
    public function setFirstNameAttribute($value)
    {
        return strtolower($value);
    }
}

The most common use case is when we want to alter some data before it is passed to the normal casting logic.

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

    public function setColorsAttribute($value)
    {
        return array_map('trim', $value);
    }
}

this maintains backwards compatibility because currently no values are returned from mutators, from which PHP will inherently return null.

https://www.php.net/manual/en/functions.returning-values.php

this allows us to return a non-`null` value from our Model mutator in order to continue through the `setAttribute()` method. this maintains backwards compatibility because currently no values are returned from mutators, from which PHP will inherently return `null`. https://www.php.net/manual/en/functions.returning-values.php

mfn · 2019-08-02T18:51:43Z

Your change would not account for date casts as it's in the else of the condition you would fall through. So it's already not quite exactly.

Although I understand it's an optional feature:

setters already quite magic and this adds magic on top of it
you can now have setters which don't "set" anything =>IMHO confusing

I think a setter (and getter) should be as transparent as possible and this goes into the opposite direction.

the only way you would make it to this conditional is if the previous `if` was false, so being an `if` or an `elseif` doesn't really make a difference.

browner12 · 2019-08-02T19:06:00Z

Fair point about the date casting. I've switched the elseif to an if to solve that.

I don't think this is adding any more magic than currently exists. With a couple lines of docs it will be easily understandable.

Can you explain this line a little more?

you can now have setters which don't "set" anything =>IMHO confusing

We are still setting the attribute, we are just allowing the setAttribute() method to handle it instead of our own model code.

https://github.com/laravel/framework/blob/5.8/src/Illuminate/Database/Eloquent/Concerns/HasAttributes.php#L592

staudenmeir · 2019-08-02T19:24:25Z

I think it's not ideal that you can't set an attribute to null with this approach. If you use something like return $condition ? $value : null;, the attribute doesn't change in the second case.

If we merge this, we should target 6.0. I'm sure there's someone who returns $this from their mutator (for whatever reason). The PR would break their code.

browner12 · 2019-08-02T19:48:00Z

@staudenmeir good point about setting a null value. Originally I had wanted to check if the mutator function returns something or is void, but apparently void functions still inherently return null in PHP, so that was the best "flag" I could choose.

For someone returning $this, that is not a documented way to use the mutator. I feel like we are not responsible for BC for people who misuse the features. Thoughts on that?

mfn · 2019-08-02T19:50:42Z

Thanks for your work @browner12 , I've changed my mind regarding "all that magic".
And I actually think the approach is really smart 👍

However I'm too a bit worried about the null case and compatibility. It's at pity PHP can't differentiate between actual void and null in this case, as you mentioned.

taylorotwell · 2019-08-03T11:39:54Z

With the null case and potential breaking changes here I would prefer to just stick with the solution offered in the previous attempt of explicitly calling castAttribute in your mutator. Thanks.

fix date casting

2b79208

the only way you would make it to this conditional is if the previous `if` was false, so being an `if` or an `elseif` doesn't really make a difference.

taylorotwell closed this Aug 3, 2019

mfn mentioned this pull request Aug 6, 2019

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

Closed

browner12 deleted the greedy-mutator branch November 21, 2019 23:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[5.8] Less Greedy Mutator #29392

[5.8] Less Greedy Mutator #29392

[5.8] Less Greedy Mutator #29392

[5.8] Less Greedy Mutator #29392

Conversation