Skip to content

[5.8] Less Greedy Mutator #29392

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
wants to merge 2 commits into from
Closed

[5.8] Less Greedy Mutator #29392

wants to merge 2 commits into from

Conversation

browner12
Copy link
Contributor

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

@browner12
allow mutated properties to be cast
f01e7d1 
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
Copy link
Contributor

mfn commented Aug 2, 2019

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.

@browner12
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.
@browner12
Copy link
Contributor Author

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
Copy link
Contributor

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
Copy link
Contributor Author

@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
Copy link
Contributor

mfn commented Aug 2, 2019

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
Copy link
Member

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.

@mfn mfn mentioned this pull request Aug 6, 2019
Closed
@browner12 browner12 deleted the greedy-mutator branch November 21, 2019 23:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@browner12 @mfn @staudenmeir @taylorotwell