Skip to content
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.

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

[8.x] Attribute Cast / Accessor Improvements #40022

Merged
merged 12 commits into from
Dec 16, 2021
Merged

Conversation

taylorotwell
Copy link
Member

@taylorotwell taylorotwell commented Dec 13, 2021

Summary

This pull request adds a new way to define attribute "accessors / mutators" as they are currently called in the
documentation: https://laravel.com/docs/8.x/eloquent-mutators#accessors-and-mutators

Currently, accessors and mutators are added to a model by defining get{Foo}Attribute and set{Foo}Attribute methods on the model. These conventionally named methods are then used when the developers attempts to access the $model->foo property on the model.

This aspect of the framework has always felt a bit "dated" to me. To be honest, I think it's one of the least elegant parts of the framework that currently exists. First, it requires two methods. Second, the framework does not typically prefix methods that retrieve or set data on an object with get and set - it just hasn't been part of Laravel's style (e.g., $request->input() vs. $request->getInput(), $request->ip() vs. $request->getIp, etc.

This pull request adds a way to define attribute access / mutation behavior in a single method marked by the Illuminate\Database\Eloquent\Casts\Attribute return type. In combination with PHP 8+ "named parameters", this allows developers to define accessor and mutation behavior in a single method with fluent, modern syntax by returning an Illuminate\Database\Eloquent\Casts\Attribute instance:

/**
 * Get the user's title.
 */
protected function title(): Attribute
{
    return new Attribute(
        get: fn ($value) => strtoupper($value),
        set: fn ($value) => strtolower($value),
    );
}

Accessors / Mutators & Casts

As some might have noticed by reading the documentation already, Eloquent attribute "casts" serve a very similar purpose to attribute accessors and mutators. In fact, they essentially serve the same purpose; however, they have two primary benefits over attribute accessors and mutators.

First, they are reusable across different attributes and across different models. A developer can assign the same cast to multiple attributes on the same model and even multiple attributes on different models. An attribute accessor / mutator is inherently tied to a single model and attribute.

Secondly, as noted in the documentation, cast classes allow developers to hydrate a value object that aggregates multiple properties on the model (e.g. Address composed of address_line_one, address_line_two, etc.), immediately set a property on that value object, and then save the model like so:

use App\Models\User;

$user = User::find(1);

$user->address->lineOne = 'Updated Address Value';

$user->save();

The current, multi-method implementation of accessor / mutators currently does not allow this and it can not be added to that implementation minor breaking changes.

However, this improved implementation of attribute accessing does support proper value object persistence in the same way as custom casts - by maintaining a local cache of value object instances:

/**
 * Get the user's address.
 */
protected function address(): Attribute
{
    return new Attribute(
        get: fn ($value, $attributes) => new Address(
            $attributes['address_line_one'],
            $attributes['address_line_two'],
            $attributes['city'],
        ),
        set: fn ($value) => [
            'address_line_one' => $value->addressLineOne,
            'address_line_two' => $value->addressLineTwo,
            'city' => $value->city,
        ],
    );
}

Mutation Comparison

In addition, as you may have noticed, this implementation of accessors / mutators does not require you to manually set the properties in the $this->attributes array like a traditional mutator method requires you to. You can simply return the transform value or array of key / value pairs that should be set on the model:

"Old", two-method approach:

public function setTitleAttribute($value)
{
    $this->attributes['title'] = strtolower($value);
}

New approach:

protected function title(): Attribute
{
    return new Attribute(
        set: fn ($value) => strtolower($value),
    );
}

FAQs

What if I already have a method that has the same name as an attribute?

Your application will not be broken because the method does not have the Attribute return type, which did not exist before this pull request.

Will the old, multi-method approach of defining accessors / mutators go away?

No. It will just be replaced in the documentation with this new approach.

@tontonsb
Copy link
Contributor

You can simply return the transform value or array of key / value pairs that should be set on the model

Is return []; the intended early abort then? And return; would not abort the setter, but set the value to null?

@taylorotwell
Copy link
Member Author

taylorotwell commented Dec 13, 2021

@inxilpro suggested using return type of the method instead of a PHP 8 attribute to determine if a method is an accessor / mutator. That would probably allow this feature to work on PHP 7.3+, etc:

public function title() : Attribute
{
   // ...
}

However, @mpociot suggested that accessors are more common than mutators (in his opinion) and it would be nice to be able to do this to just define an accessor:

CleanShot 2021-12-13 at 16 11 15@2x

These two suggestions aren't compatible - so will need to decide on a route forward here.

@tuarrep
Copy link
Contributor

tuarrep commented Dec 13, 2021

What about extending your PR a bit like suggested here https://twitter.com/robboclancy/status/1469203713470386182?s=21

image

@michaeldyrynda
Copy link
Contributor

@inxilpro suggested using return type of the method instead of a PHP 8 attribute to determine if a method is an accessor / mutator. That would probably allow this feature to work on PHP 7.3+, etc:

public function title() : Attribute
{
   // ...
}

However, @mpociot suggested that accessors are more common than mutators (in his opinion) and it would be nice to be able to do this to just define an accessor:

I like this suggestion but I wonder if it clashes with the long-established convention around these types of “floating” methods where property vs method call is for relationship vs query builder.

If it can be made to work with just the Attribute return type, it becomes more compatible with the supported PHP versions that Laravel 8.x runs on (and perhaps the Attribute attribute can be the documented/preferred method for Laravel 9.x, which required PHP 8)

@tontonsb
Copy link
Contributor

However, @mpociot suggested that accessors are more common than mutators (in his opinion) and it would be nice to be able to do this to just define an accessor:

I vote to keep getFooAttribute and setFooAttribute in the docs. They do exactly this job — simple and concise one way accessors/mutators. And the new syntax would serve the cases where you need both ways.

These two suggestions aren't compatible

I don't understand why. #[AsAccessor] could be a decent syntax alternative for getFooAttribute(). Just as #[AsMutator]. But I think these could wait Laravel 9 as it's almost identical to getFooAttribute/setFooAttribute.

@GrahamCampbell GrahamCampbell changed the title Attribute Cast / Accessor Annotations [8.x] Attribute Cast / Accessor Annotations Dec 14, 2021
@taylorotwell
Copy link
Member Author

@tontonsb I don't follow your comment. The two things that aren't compatible are @inxilpro's suggestion and @mpociot's suggestion.

@inxilpro
Copy link
Contributor

If we wanted to support read-only or write-only attributes we could do something like this:

interface ReadableAttribute
{
  public function get();
}

interface WritableAttribute extends ReadableAttribute
{
  public function set($value);
}

class Mutator implements WritableAttribute
{
  // ...
}

class Accessor implements ReadableAttribute
{
  // ...
}

class Attribute implements ReadableAttribute, WritableAttribute
{
  // ...
}

That way you can do any of the following:

public function firstName(): Accessor
{
  return new Accessor(fn($value) => ucfirst($value));
}

public function email(): Mutator
{
  return new Mutator(fn($value) => strtolower($value));
}

public function name(): Attribute
{
  return new Attribute(
    get: fn() => "{$this->attributes['first_name']} {$this->attributes['last_name']}",
    set: function($value) {
      [$first, $last] = explode(' ', $value, 2);
      $this->attributes['first_name'] = $first;
      $this->attributes['last_name'] = $last;
    },
  );
}

That said, I really don't see a huge upside for supporting the individual use-cases, especially with named parameters. It just feels like always returning an Attribute object keeps things clear, as long as you can only set the parts that you care about.

This is barely more work than @mpociot's suggestion, and keeps everything under Attribute object:

public function firstName(): Attribute
{
  return new Attribute(get: fn($value) => ucfirst($value));
}

@inxilpro
Copy link
Contributor

We could add a static helper method to make it a tiny bit cleaner:

public static function get(Closure $mutator)
{
  return new static(get: $mutator);
}

That way you could just do Attribute::get(fn($value) => ucfirst($value)) (although that might be overkill).

@taylorotwell
Copy link
Member Author

I have updated this PR to implement @inxilpro's suggestion of "marking" the methods using a return type instead of an attribute:

/**
 * Get the user's title.
 */
protected function title(): Attribute
{
    return new Attribute(
        get: fn ($value) => strtoupper($value),
        set: fn ($value) => strtolower($value),
    );
}

@inxilpro
Copy link
Contributor

@taylorotwell if we use return types here, and especially if #39947 gets merged, it probably makes sense to add a method to the Reflector like methodReturnsType:

Reflector::methodReturnsType($reflectionMethod, Attribute::class);

@tontonsb
Copy link
Contributor

@taylorotwell my point was that I see no incompatibility, both approaches are able to coexist without a conflict:

protected function title(): Attribute
{
    return new Attribute(
        get: fn ($value) => strtoupper($value),
        set: fn ($value) => strtolower($value),
    );
}

#[AsAccessor]
protected function firstName($name)
{
    return ucfirst($name);
}

But the second one is not very necessary, because getFirstNameAttribute does pretty much the same. So my suggestion was to keep getFirstNameAttribute alternative in the documentation as that will remain the cleanest, most concise solution for access-only/mutate-only cases. And consider the #[AsAccessor] syntax for Laravel 9.x.

@tontonsb
Copy link
Contributor

@inxilpro I think that the "new" one-way attribute approach would be too verbose compared to the old one or attribute suggestion:

// return type
protected function firstName(): Accessor
{
  return new Accessor(fn($value) => ucfirst($value));
}

// Attribute
#[AsAccessor]
protected function firstName($name)
{
    return ucfirst($name);
}

// Old
public function getFirstNameAttribute($name)
{
    return ucfirst($name);
}

Easier to comprehend what's going on when the function body does the lifting instead of a class-wrapped callback.

@davisngl
Copy link

I don't think it should get replaced in documentation with this new method. Rather add it as the "other option".
Nevertheless, much cleaner and simpler this way.

@mikield
Copy link

mikield commented Dec 14, 2021

IMHO the proposed option is much cleaner, simpler to understand, and extendable.

As I understand, we could have one "style" of working with attributes, casts array, or defining a custom attribute modifier.
Also, that will bring the same experience as working with relationships, where when I call a method - I get the relationship builder object, and if I call it like attribute - I get the result.

I will allow me to call something like $post->title()->replace(" ", "_") for example to get title with replaced spaces and $post->title to get not replaced title of post :)

@williamxsp
Copy link

Is it possible to somehow define the type of an Attribute using this approach so my IDE can understand?

@taylorotwell
Copy link
Member Author

Is it possible to somehow define the type of an Attribute using this approach so my IDE can understand?

Add a @property annotation to the top of your class.

@mikield
Copy link

mikield commented Dec 14, 2021 via email

@bsv-hub
Copy link

bsv-hub commented Dec 23, 2021

In my opinion, something like this will be more declarative and simpler.

Screenshot from 2021-12-23 19-23-32

roberto-aguilar added a commit to roberto-aguilar/docs that referenced this pull request Dec 24, 2021
When I saw Taylor's comment I though this would be a great practice to
encourage. In fact, another benefit of marking this methods as protected
is that they will not be displayed by the IDE during auto-completion in
other classes.

laravel/framework#40022 (comment)
@Marwelln
Copy link
Contributor

Is PHP 8+ requirement too soon? Constructor promotion would be great for this.

(The methods should also get a return type)

<?php
class Attribute
{
    public function __construct(public ?callable $get = null, public ?callable $set = null) {}

    public static function get(callable $get) : static
    {
        return new static($get);
    }

    public static function set(callable $set) : static
    {
        return new static(null, $set);
    }
}

@Esirei
Copy link

Esirei commented Dec 27, 2021

Is PHP 8+ requirement too soon? Constructor promotion would be great for this.

(The methods should also get a return type)

<?php
class Attribute
{
    public function __construct(public ?callable $get = null, public ?callable $set = null) {}

    public static function get(callable $get) : static
    {
        return new static($get);
    }

    public static function set(callable $set) : static
    {
        return new static(null, $set);
    }
}

Laravel 8.x supports PHP 7.3+, so constructor promotion is a no.
Can be updated for 9.x branch though. 🤷🏾‍♂️

@lupinitylabs
Copy link
Contributor

Is PHP 8+ requirement too soon? Constructor promotion would be great for this.
(The methods should also get a return type)

<?php
class Attribute
{
    public function __construct(public ?callable $get = null, public ?callable $set = null) {}

    public static function get(callable $get) : static
    {
        return new static($get);
    }

    public static function set(callable $set) : static
    {
        return new static(null, $set);
    }
}

Laravel 8.x supports PHP 7.3+, so constructor promotion is a no. Can be updated for 9.x branch though. 🤷🏾‍♂️

This. Also the reason why we cannot use named parameters instead of passing null to the constructor as the first parameter.

@lupinitylabs
Copy link
Contributor

I am a little late to the party, but now that it's possible to do something like

    protected function name(): Attribute
    {
        return Attribute::get(fn($value) => strtoupper($value));
    }

wouldn't it also be desirable to add non-static get() and set() methods in order to do something like this?

    protected function name(): Attribute
    {
        return Attribute::get(fn($value) => strtoupper($value))
                       ->set(fn($value) => strtolower($value));
    }

This would really offer the developer the full monty 😁

@Healyhatman
Copy link

Healyhatman commented Dec 28, 2021

Is there a way to have other named parameters, or extend the Attribute class to allow them? I'm just thinking it'd be nice for Laravel Collective to have

get: fn() => ...,
set: fn() => ...,
form: fn() => ...

and so on.

@WahidinAji
Copy link

, accessors and mutators are added to a model by defining get{Foo}Attribute and set{Fo

is it only running on PHP 8 ?

@WahidinAji
Copy link

image
81037/147847075-a6358aa4-728c-4309-b4e1-9c90ec6888d8.png)
image

hooo, its can running on PHP 7.4 too. thanks @taylorotwell for a great feature

@lupinitylabs
Copy link
Contributor

, accessors and mutators are added to a model by defining get{Foo}Attribute and set{Fo

is it only running on PHP 8 ?

The named parameters approach will only work from PHP 8. So if you only define a getter or a getter and setter (in that order), it will work on lower versions as well, but if you try to only define a setter, you will need to manually pass null as the first parameter for Argument instantiation.

@hubertnnn
Copy link

Could you explain what is the difference between the new Attributes and existing Casts feature?
Is it only using a method instead of an array, or are there any other differences?

So lets take an example strait from Laravel's documentation

class Json implements CastsAttributes
{
    public function get($model, $key, $value, $attributes)
    {
        return json_decode($value, true);
    }

    public function set($model, $key, $value, $attributes)
    {
        return json_encode($value);
    }
}

class User extends Model
{
    protected $casts = [
        'options' => Json::class,
    ];
}

vs

class Json extends Attribute
{
    public function __construct()
    {
        parent::__construct(
            get: fn ($value) => json_decode($value, true),
            set: fn ($value) => json_encode($value),
        );
    }
}

class User extends Model
{
    protected function options(): Attribute
    {
        return new Json();
    };
}

@argonzalez2016
Copy link

Have an issue with this,

protected function email(): Attribute
    {
        return new Attribute(
            set: fn ($value) => Str::of($value)->lower()->trim()
        );
    }

protected function defaultProfilePhotoUrl()
    {
        $hash = md5($this->email);
        return $this->has_gravatar 
            ? "https://gravatar.com/avatar/{$hash}?s=200&d=404"
            : 'https://ui-avatars.com/api/?name='.urlencode($this->name).'&color=7F9CF5&background=EBF4FF';
    }

getting App\Models\User::email must return a relationship instance.

this worked before switching to new way of accessors/mutators.

@nachopitt
Copy link

I haven't fully switched to this new approach, at least for the getter/accessor part, because I haven't found a way to not loose access to the original attribute value. I mean, the one that hasn't been modified by the accessor mechanism. At least with the getXXXAttribute() approach you can add a new attribute with a different name and you don't loose access to the original attribute value.

Am I missing something?

Thanks a lot.

@williamxsp
Copy link

williamxsp commented Feb 7, 2023

@nachopitt If I understood you correctly, you can create new attributes the same way.
Lets suppose your model have a name attribute and you want to create a new one, lets say 'full_name'
You can create:

public function fullName(){
return new Attribute(get: fn() => $this->first_name . ' ' . $this->last_name);
}

Then you can access your new attribute

$model->full_name

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.