glad to see this come back to life

This is going to be awesome!

Relevant discussion: laravel/ideas#9

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?

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?

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.

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

@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.

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.

@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). ?

@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.

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.

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.

@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).

@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 😀

@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,
    ];
 }

@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.

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

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

It's just an example.

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

@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.

@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.

@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.

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.

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.

@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

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

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

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',
    ];

It could be extracted to trait then.