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

[5.5] Eloquent object casting #18229

Closed
wants to merge 14 commits into
base: master
from

Conversation

Projects
None yet
@tillkruss
Copy link
Member

tillkruss commented Mar 6, 2017

Re-implementation of #13706 after a long discussion with @adamwathan and @JosephSilber.

$user = User::create([
    'name' => 'Taylor',
    'token' => new Secret('Adam123'),
]);

$user->token; // Secret instance
$user->token->secret; // "Adam123"

$user->price = (new Money(10))->setCurrency('BTC');

$user->price; // Money instance
$user->price->amount; // "10"
$user->toArray()['price']; // "10"
$user->currency; // "BTC"

Example classes are in the next comment below.

@tillkruss tillkruss force-pushed the tillkruss:ultra-secret-object-casting-conspiracy branch from 20bb3c9 to 36c982e Mar 6, 2017

@tillkruss

This comment has been minimized.

Copy link
Member

tillkruss commented Mar 6, 2017

class User extends Model
{
    protected $casts = [
        'token' => 'secret',
        'price' => 'money:price,currency',
    ];

    public function castToSecret($value)
    {
        return Secret::fromHash($value);
    }

    // no "castFromSecret" here, because Secret has a __toString() method

    public function castToMoney($amount, $currency)
    {
        return (new Money($amount))->setCurrency($currency);
    }

    public function castFromMoney($money)
    {
        return [$money->amount, $money->currency];
    }
}
class Secret
{
    public $secret;

    public function __construct($secret)
    {
        $this->secret = $secret;
    }

    public static function fromHash($secret)
    {
        return new static(base64_decode($secret));
    }

    public function __toString()
    {
        return base64_encode($this->secret);
    }
}
class Money
{
    public $amount;

    public $currency;

    public function __construct($amount)
    {
        $this->amount = $amount;
    }

    public function setCurrency($currency)
    {
        $this->currency = $currency;

        return $this;
    }
}
@devcircus

This comment has been minimized.

Copy link
Contributor

devcircus commented Mar 6, 2017

glad to see this come back to life

@joshmanders

This comment has been minimized.

Copy link
Contributor

joshmanders commented Mar 7, 2017

This is going to be awesome!

@sisve

This comment has been minimized.

Copy link
Contributor

sisve commented Mar 8, 2017

Relevant discussion: laravel/ideas#9

@sisve

This comment has been minimized.

Copy link
Contributor

sisve commented Mar 8, 2017

  1. Why isn't this implemented as separate "casting objects". An example; a CastsMoney that has castToPHP and castToDatabase? This would also give the possibility to expose data on how many columns are expected.
  2. How are we expected to handle casting of null values? castAttribute is doing a null-check before even calling castToClass.
  3. How is the casting logic expected to expose any dependencies needed? Constructor injection is common in other places in the framework, this can't be done here. Method injections, like controller actions?

@tillkruss tillkruss force-pushed the tillkruss:ultra-secret-object-casting-conspiracy branch from b36602b to 36c982e Mar 8, 2017

@sisve

This comment has been minimized.

Copy link
Contributor

sisve commented Mar 8, 2017

Couldn't the previous code in castAttribute use the new logic too? castToInt, castToFloat, castToString, castToBool, ...? Or perhaps just the non-scalar values like castToCollection, castToJson, castToDate, castToDateTime, castToTimestamp?

@RemiCollin

This comment has been minimized.

Copy link

RemiCollin commented Mar 8, 2017

Does the object properties have to be public ? We'd moslty want them protected for most value objects, to ensure better integrity. Also can we have multiple constructor arguments ?

Some basic example :


class Identity 
{
    protected $firstName;
    protected $lastName;

    public function __construct($firstName, $lastName) {
        $this->firstName = $firstName;
        $this->lastName = $lastName;
    }
}

And +1 with sisve about dependency injection. It should be ideally handled as well.

@tillkruss

This comment has been minimized.

Copy link
Member

tillkruss commented Mar 8, 2017

@RemiCollin: No, the properties' visibility doesn't matter, because your castFrom*() method can get the model attributes however it pleases.

@tillkruss

This comment has been minimized.

Copy link
Member

tillkruss commented Mar 8, 2017

@sisve

How are we expected to handle casting of null values? castAttribute is doing a null-check before even calling castToClass.

$user->price = null; will unset the price and currency attributes on the model.

If you want to handle null scenarios differently, you can set $user->price = new Money(null) and let the object decide what to do/return.

How is the casting logic expected to expose any dependencies needed? Constructor injection is common in other places in the framework, this can't be done here. Method injections, like controller actions?

You can use Laravel's resolve() helper in your castTo*() methods.

Couldn't the previous code in castAttribute use the new logic too? castToInt, castToFloat, castToString, castToBool, ...? Or perhaps just the non-scalar values like castToCollection, castToJson, castToDate, castToDateTime, castToTimestamp?

Yes, I already played around with that a little bit, but this is a separate PR in my opinion — to keep it simple.

@sisve

This comment has been minimized.

Copy link
Contributor

sisve commented Mar 8, 2017

When I'm talking about null values I am thinking of null values in the database. How can I transform them into a meaningful value in php? The castAttribute method checks for nulls, and return nulls, before calling the new casting code. I would prefer if my castToShazaam() method would receive the null value and handle it itself.

I don't like the pattern of calling resolve() or App::make() from within models (or any other code), it hides dependencies. I prefer it if method has a clear scope and intent, and only uses what has been provided; not call out and introduce dependencies to things that aren't obvious.

@RemiCollin

This comment has been minimized.

Copy link

RemiCollin commented Mar 8, 2017

@tillkruss : ok I see, at first I thought the intention was to handle this in base Model class for auto magic. But being verbose here makes sense, for flexibility.

For DI, agree with @sisve that calling resolve() would obviously be a code smell. Why not having the container as a property of the Model class, so we can make calls like $this->app->make(Money::class). ?

@tillkruss

This comment has been minimized.

Copy link
Member

tillkruss commented Mar 8, 2017

@sisve: All null database values are passed to the castTo*() method, so you can decide what to do with them.

If that's not the case for you when testing this PR locally, please provide a working code sample for me to test against.

@sisve

This comment has been minimized.

Copy link
Contributor

sisve commented Mar 9, 2017

I'm looking at the github source view, and it clearly shows the is_null check before doing calling castToClass. https://github.com/tillkruss/framework/blob/36c982ea23bd6759acfd088ad9972bcf16f813f7/src/Illuminate/Database/Eloquent/Concerns/HasAttributes.php#L474-L480

What you describe matches b36602b but that isn't part of this PR, according to the GitHub interface.

@sisve

This comment has been minimized.

Copy link
Contributor

sisve commented Mar 9, 2017

Can we change mergeAttributesFromClassCasts into preferring the castFrom* methods instead of __toString? I think that if I write a castFromShazaam method, it should have priority over my Shazaam::__toString() may have another focus than giving values to persist into the database.

@tillkruss

This comment has been minimized.

Copy link
Member

tillkruss commented Mar 9, 2017

@sisve: That makes sense, just switched it around. Thanks!

Regarding the null-scenario: When the model is instantiated from a database record, the castAttribute() method is not called and castTo*() will receive whatever database value(s).

@JosephSilber

This comment has been minimized.

Copy link
Contributor

JosephSilber commented Mar 14, 2017

@sisve

How is the casting logic expected to expose any dependencies needed? Constructor injection is common in other places in the framework, this can't be done here.

Value objects generally don't (or shouldn't) have dependencies. If you do need them, use the resolve method from within your castTo* method.

I don't like the pattern of calling resolve() [...], it hides dependencies. I prefer it if method has a clear scope and intent, and only uses what has been provided; not call out and introduce dependencies to things that aren't obvious.

These castTo* are factory methods. If they need external dependencies, they have to pull it in somehow. There's no going around that, and using resolve is a very elegant approach.

@RemiCollin

For DI [...] calling resolve() would obviously be a code smell.

Uh... really? Why? Calling resolve from within a factory method means the code is still fully, 100% testable. Don't let people scare you away from practical solutions.

Why not having the container as a property of the Model class, so we can make calls like $this->app->make(Money::class)?

And how would that magically be any better? You're reaching into the very same container, and resolving a class dynamically.


Dynamic service resolution is a very useful tool. Use it when applicable and stay happy 😀

@RemiCollin

This comment has been minimized.

Copy link

RemiCollin commented Mar 15, 2017

@JosephSilber : you're right the result is the same, I didn't see it from that angle. About Value Objects not having dependencies, I'd agree 99%, though sometime it's nice to have this possibility.

Also, while reading the examples above, I wonder if it wouldn't be better to explicitely return key value pairs from the castFrom* method. I feel it could become error prone to only rely on the attribute's order if the number of attributes is important.

Something like this maybe:


public function castFromMoney($money)
{
    return [
        'price' => $money->amount, 
        'currency' => $money->currency,
    ];
 }
@JosephSilber

This comment has been minimized.

Copy link
Contributor

JosephSilber commented Mar 15, 2017

@RemiCollin those column names shouldn't be hard coded. The casting method should not be aware of what columns the value is stored in.

Imagine you have 2 money values in the same table:

  • price
  • price_currency
  • shipping
  • shipping_currency

Then you'd have this in your casts:

$casts = [
    'price' => 'money:price,price_currency',
    'shipping' => 'money:shipping,shipping_currency',
];

As you can see, the actual column names have to stay abstract. It's up to the user to specify the columns.

@tillkruss tillkruss force-pushed the tillkruss:ultra-secret-object-casting-conspiracy branch from 319365d to 7af24a1 Mar 16, 2017

@tillkruss tillkruss force-pushed the tillkruss:ultra-secret-object-casting-conspiracy branch from 7af24a1 to 65c2f0a Mar 25, 2017

: $object->__toString();
if ($attributeCount !== count($castedAttributes)) {
throw new LogicException("Class cast {$attribute} must return {$attributeCount} attributes");

This comment has been minimized.

@jmarcher

jmarcher Mar 28, 2017

Contributor

Wouldn't be better to change this to:
"Class cast {$attribute} must return {$attributeCount} attribute(s)" ? I've seen it in some parts of the core

This comment has been minimized.

@tillkruss

tillkruss Mar 28, 2017

Member

Yep, just did that. Thanks!

@ConnorVG

This comment has been minimized.

Copy link
Contributor

ConnorVG commented Apr 27, 2017

This is marvellous; I'd be very eager to read the documentation.

@jimgwhit

This comment has been minimized.

Copy link

jimgwhit commented May 6, 2017

Just curious, why the over thinking of dealing with money? I have always just stored a 0.00 if there is no value.

@tillkruss tillkruss force-pushed the tillkruss:ultra-secret-object-casting-conspiracy branch 2 times, most recently from 80913c9 to 6139ad8 May 6, 2017

@tillkruss

This comment has been minimized.

Copy link
Member

tillkruss commented May 6, 2017

It's just an example.

@ConnorVG

This comment has been minimized.

Copy link
Contributor

ConnorVG commented May 8, 2017

What level of SQL query-ability would we have with this? Based on the money example, I would love to do something like:
User::where('money.currency', 'USD') etc

@tillkruss tillkruss force-pushed the tillkruss:ultra-secret-object-casting-conspiracy branch from 0464e0c to 22b4fe8 Jul 19, 2017

@tillkruss

This comment has been minimized.

Copy link
Member

tillkruss commented Jul 19, 2017

@DanielCoulbourne: Duplication for example, what if you model has 3 attributes that are all the same object, for example: price, sale_price, wholesale_price. Instead of having 6 methods, you only need one or two.

@garygreen

This comment has been minimized.

Copy link
Contributor

garygreen commented Jul 20, 2017

@tillkruss I too am struggling to see why all of this can't just be a simple accessor. For example:

<?php

class User {

    public function getSecretAttribute()
    {
        return $this->buildOrGetInstance('secret', function() {
            return new Secret($this->attributes['secret']);
        });
    }

    public function setSecretAttribute($value)
    {
        $this->secret->setSecret($value);
        $this->attributes['secret'] = $value;
    }

}

The buildOrGetInstance is just a simple function on your base model that will get the instance by name (similar to getRelation($name)) or call the closure and create it, caching it under ->instances

My thoughts are just kinda "meh" towards this when we've already got something similar. Seems pretty over-engineered.

@linaspasv

This comment has been minimized.

Copy link
Contributor

linaspasv commented Jul 27, 2017

@garygreen I like your pseudocode. Is there any available implementation of buildOrGetInstance? I guess the feature/possibility of accessor caching is missing in current eloquent object implementation.

On the other hand @tillkruss implementation focuses more on a possibility of creating custom casts. As of custom casts - maybe it should be in a similar style like we currently have for custom validation rules creation Validator::extend . It would make sense to have custom casts defined globally instead of re-defining on each model separately.

@garygreen

This comment has been minimized.

Copy link
Contributor

garygreen commented Jul 27, 2017

I guess the feature/possibility of accessor caching is missing in current eloquent object implementation.

That's true, to be honest I was kinda suprised to find out recently that when you access attributes that are cast to Carbon new instances of Carbon are created every time you access the attribute.

E.g.:

$user->created_at; // Creates new carbon model....
$user->created_at; // Creates 2nd carbon model...
$user->created_at; // Creates 3rd carbon model... etc

Seems a bit of a waste to keep creating new instances, but I guess it's useful if you want to mutate the date in some way... then again you can always use ->created_at()->copy() to guarantee you've got yourself a new mutable instance.

@unstoppablecarl

This comment has been minimized.

Copy link
Contributor

unstoppablecarl commented Aug 10, 2017

I downloaded the branch and tested it out. I have some serious problems with this so far. I'm not sure if I'm getting what this solves, maybe the example provided is not a good use case.

Problems:

1) Creates Variant interface for attributes

$user = new User();
$user->price->amount; // property of non object error
$user->price->currency; // property of non object error

$user->price is not an instance of Money until you set it, so the interface for retrieving the price attribute is now variant.

if($user->price instanceof Money){
	$user->price->amount;
} else {
	$user->price ?: 0;
}

2) Incomplete Symmetry

There are now 2 seemingly valid places to set the currency (which is fine) but they are not synced both ways.

$user->price = (new Money(10))->setCurrency('BTC');
$user->currency = 'USD';
$user->price->currency; // BTC, does not sync from attribute to value object
$user->price = (new Money(10))->setCurrency('BTC');
$user->price->currency = 'USD';
$user->price->currency; // USD
$user->currency; // USD, seems to sync from value object to attribute

// non-public internally referenced `$attributes` property is not updated
$user->attributes['currency'] // 'BTC'

Anything internally referencing the attributes array is now getting incorrect data.

@garygreen

This comment has been minimized.

Copy link
Contributor

garygreen commented Aug 10, 2017

@unstoppablecarl I completely agree with both points. Did you see my example above? It solves both those problems with simple getters and setters if you implement a very basic buildOrGetInstance

@taylorotwell

This comment has been minimized.

Copy link
Member

taylorotwell commented Aug 17, 2017

I think this is just more than I want to take on right now.

@antonkomarev

This comment has been minimized.

Copy link
Contributor

antonkomarev commented May 1, 2018

It would be great to have this functionality out of the box in Laravel 5.7

@regularlabs

This comment has been minimized.

Copy link
Contributor

regularlabs commented Oct 10, 2018

I ended up using this on my models via a trait:

    protected function castAttribute($key, $value)
    {
        if (isset($this->casts[$key]) && $this->casts[$key] == 'datetime') {
            return new MyOwnCarbon($value);
        }

        return parent::castAttribute($key, $value);
    }

Then in my models I have $casts, like:

    protected $casts = [
        'date_time_start'        => 'datetime',
        'date_time_finish'       => 'datetime',
    ];
@antonkomarev

This comment has been minimized.

Copy link
Contributor

antonkomarev commented Oct 11, 2018

It could be extracted to trait then.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment