laravel / ideas

Issues board used for Laravel internals discussions.
938 stars 28 forks source link

Improve Typecasting #9

Closed AdenFraser closed 6 years ago

AdenFraser commented 8 years ago

This has been discussed before in different issues raised laravel/framework.

I think it would be rather handy if Eloquent's typecasting was extendable, so that as well as being able to automatically cast to the existing: integer, real, float, double, string, boolean, object, array, collection, date and datetime That we could also cast to custom cast types.

For instance when using value objects this would allow the automatic casting of a dec to a Money type object.

Thoughts?

sebastiandedeyne commented 8 years ago

Been pondering about the idea of casting to custom objects myself. Rough idea: classes that implement an interface with some sort of serialize & unserialize method, the class name could then be passed in the casts array.

interface DatabaseCastThing
{
    public function fromDatabase();
    public function toDatabase();
}
class Email implements DatabaseCastThing
{
    // ...
}
protected $casts = ['email' => Email::class];
valorin commented 8 years ago

Is there a reason you don't want to use Accessors & Mutators for this purpose?

I haven't seen any of the other discussions, so I don't know if it's been brought up before... but it's worth explaining why here, if it has.

AdenFraser commented 8 years ago

Well for starters, if you use accessors and mutators the result is an awful lot of code duplication.

If you have a Model with 10 castable values, that's 10 accessors and mutuators you'd have to setup, chances are this would be across multiple models. The only way I can see you'd avoid that is either through an abstract model (assuming each model shares similar values) or through a castable trait.

The other problem with accessors & mutators (and even the existing casting) is that nullable values required additional logic which again adds code duplication.

Imagine reproducing this:

    public function getOriginalPriceAttribute($value) 
    {
        if (is_null($value)) {
            return $value;
        }

        return new Money($value);
    }

    public function setOriginalPriceAttribute($value)
    {
        if (is_null($value)) {
            $this->attributes['money'] = $value;
        }

        $this->attributes['money'] = $value->toDecimal(); // or automatically to string via __toString(); etc.
    }

^That's an awful lot of code for what is essentially some incredibly simple typecasting.

Neither of these is a terribly elegant solution, compared to that of @sebastiandedeyne's idea above. Even there the toDatabase() method shouldn't be required in the interface... since an a value casted to an object with __toString(); would work just fine (assuming that is what the end-developer wants).

martinbean commented 8 years ago

I’m in the Accessors & Mutators camp for this.

alexbilbie commented 8 years ago

Casting to and from a JSON blob would be useful too

arcanedev-maroc commented 8 years ago

How about dynamic casting (like polymorphism) ? can be also sorted in a custom db column (castables).

valorin commented 8 years ago

Thanks for the detailed answer @AdenFraser - that's definitely a valid use case, would simplify a lot of repeated code for common things (common value objects come to mind!). Accessors and Mutators are great for simple one-off stuff, but it'd be nice to have the power of a class too without a lot of scaffolding. :-)

Casting to and from a JSON blob would be useful too @alexbilbie I thought you could already do this using object or array?

robclancy commented 8 years ago

Not sure why anyone would ever be against this. I actually expected it to just allow a custom type when I first used casting. It's just something one would naturally expect it to support and would be very easy to do so.

AdenFraser commented 8 years ago

@taylorotwell What are your thoughts in regards to this? Obviously there are quite a few thumbs ups above aswell but having your go ahead before spending some time on a PR would be great!

themsaid commented 8 years ago

@AdenFraser I think you need to make the PR man :) the idea is quite interesting, now it's time to discuss implementation.

taylorotwell commented 8 years ago

Agree could be cool!

AdenFraser commented 8 years ago

Going to start putting some code together today and will put a PR together as a WIP

AdenFraser commented 8 years ago

Thrown together an early draft https://github.com/laravel/framework/pull/13315

barryvdh commented 8 years ago

Casts are usually one-way right? Just for getting the data from the database, with the exception of json/datetime, which are also converted back. So do you want to account for the 'back', case as well?

Also, isn't it easier to register custom casts, like the Validation? https://laravel.com/docs/5.2/validation#custom-validation-rules

TypeCaster::extend('money', function($attribute, $value, $parameters, $caster) {
    $currency = isset($parameters[0]) ? $parameters[0] : '$';
    return new Money($value, $currency);
});

TypeCaster::extend('html', 'App\Html@convert');

Usage like this:

 protected $casts = [
        'amount' => 'money:£'
    'html_string' => 'html',
    ];

If you want to convert back, perhaps we could consider a second (optional) callback.

TypeCaster::extend('money', function($attribute, $value, $parameters, $caster) {
    return Money::fromString($value);
}, function($attribute, $value, $parameters, $caster) {
    return Money::toString($value);
});

TypeCaster::extend('html', 'App\Html@encode', 'App\Html@decode');

Which would essentially be mutators I guess, but with easier configuration.

robclancy commented 8 years ago

Just do it like mutators in the model. You are overthinking this a lot. It can be as simple as looking for a method called {castname}Cast. Or could just be a single method where you can insert your own catings just like the current ones work.

Models dont use a factory so doing it like the validator extending or blade extending just wont work. It should be in object scope not class scope.

On Tue, 26 Apr 2016 22:37 Barry vd. Heuvel notifications@github.com wrote:

Casts are usually one-way right? Just for getting the data from the database, with the exception of json/datetime, which are also converted back. So do you want to account for the 'back', case as well?

Also, isn't it easier to register custom casts, like the Validation? https://laravel.com/docs/5.2/validation#custom-validation-rules

TypeCaster::extend('money', function($attribute, $value, $parameters, $caster) { $currency = isset($parameters[0]) ? $parameters[0] : '$'; return new Money($value, $currency);});TypeCaster::extend('html', 'App\Html@convert');

Usage like this:

protected $casts = [ 'amount' => 'money:£' 'html_string' => 'html', ];

If you want to convert back, perhaps we could consider a second callback.

TypeCaster::extend('money', function($attribute, $value, $parameters, $caster) { return Money::fromString($value);}, function($attribute, $value, $parameters, $caster) { return Money::toString($value);});TypeCaster::extend('html', 'App\Html@encode', 'App\Html@decode');

Which would essentially be mutators I guess, but with easier configuration.

— You are receiving this because you commented. Reply to this email directly or view it on GitHub https://github.com/laravel/internals/issues/9#issuecomment-214725670

robclancy commented 8 years ago

Also why would this need converting back?

On Wed, 27 Apr 2016 07:35 Robbo robbo.clancy@gmail.com wrote:

Just do it like mutators in the model. You are overthinking this a lot. It can be as simple as looking for a method called {castname}Cast. Or could just be a single method where you can insert your own catings just like the current ones work.

Models dont use a factory so doing it like the validator extending or blade extending just wont work. It should be in object scope not class scope.

On Tue, 26 Apr 2016 22:37 Barry vd. Heuvel notifications@github.com wrote:

Casts are usually one-way right? Just for getting the data from the database, with the exception of json/datetime, which are also converted back. So do you want to account for the 'back', case as well?

Also, isn't it easier to register custom casts, like the Validation? https://laravel.com/docs/5.2/validation#custom-validation-rules

TypeCaster::extend('money', function($attribute, $value, $parameters, $caster) { $currency = isset($parameters[0]) ? $parameters[0] : '$'; return new Money($value, $currency);});TypeCaster::extend('html', 'App\Html@convert');

Usage like this:

protected $casts = [ 'amount' => 'money:£' 'html_string' => 'html', ];

If you want to convert back, perhaps we could consider a second callback.

TypeCaster::extend('money', function($attribute, $value, $parameters, $caster) { return Money::fromString($value);}, function($attribute, $value, $parameters, $caster) { return Money::toString($value);});TypeCaster::extend('html', 'App\Html@encode', 'App\Html@decode');

Which would essentially be mutators I guess, but with easier configuration.

— You are receiving this because you commented. Reply to this email directly or view it on GitHub https://github.com/laravel/internals/issues/9#issuecomment-214725670

barryvdh commented 8 years ago

If you cast it to an object and modify it, you want to return it to a value you can store in database, right?

tomschlick commented 8 years ago

Yeah I definitely think there should be a toDatabase method of some sort. By default it could just use __toString() but for more complicated implementations it would be useful to have, especially since __toString() is horrible for reporting errors.

robclancy commented 8 years ago

What, no? If you need to return it to the database you are using this wrong. That is what mutators are for. Casting has a single use, to cast for the JSON response. So for example JS doesn't do dumb things because you accidently sent it your int in string form. If you need to cast something and return it to the database at a later date that is the exact use case for attribute mutators.

On Wed, Apr 27, 2016 at 7:47 AM Tom Schlick notifications@github.com wrote:

Yeah I definitely think there should be a toDatabase method of some sort. By default it could just use toString() but for more complicated implementations it would be useful to have, especially since toString() is horrible for reporting errors.

— You are receiving this because you commented. Reply to this email directly or view it on GitHub https://github.com/laravel/internals/issues/9#issuecomment-214897983

tomschlick commented 8 years ago

If you modify something in the cast object and then save the model it has to find its way back to the database. This is how the current casts system works. Try it with the carbon dates, it uses __toString() on the carbon instance.

robclancy commented 8 years ago

https://github.com/laravel/framework/blob/5.2/src/Illuminate/Database/Eloquent/Model.php#L2473

I don't see how that is modifying the model at all? It just casts for the attributes it is returning?

tomschlick commented 8 years ago

https://github.com/laravel/framework/blob/5.2/src/Illuminate/Database/Eloquent/Model.php#L2484

Shows how the dates are formatted with the serializeDate() method

robclancy commented 8 years ago

Yes, for the toArray method which then goes to a JSON response. It doesn't touch the underlying attributes? Casting an object returns the casted attributes, it doesn't change anything you can save like normal (unless I am missing something, and if I am I would say that is a bug).

AdenFraser commented 8 years ago

If you cast to datetime and simply use Carbon's ->addDay () method before saving... the date stored in the database will be one day in the future than it was before.

For instance

    $product->released_at->addDay ();
    $product->save ();

On the save action the Carbon instance is converted __toString in the format the Eloquent date format is required in... with any modifications.

The same way if you modify a casted json array and subsequently save... it gets updated. Or an array or any ither cast for that matter. On 27 Apr 2016 00:16, "Robert Clancy (Robbo)" notifications@github.com wrote:

Yes, for the toArray method which then goes to a JSON response. It doesn't touch the underlying attributes? Casting an object returns the casted attributes, it doesn't change anything you can save like normal (unless I am missing something, and if I am I would say that is a bug).

On Wed, Apr 27, 2016 at 9:03 AM Tom Schlick notifications@github.com wrote:

https://github.com/laravel/framework/blob/5.2/src/Illuminate/Database/Eloquent/Model.php#L2484

Shows how the dates are formatted with the serializeDate() method

— You are receiving this because you commented. Reply to this email directly or view it on GitHub https://github.com/laravel/internals/issues/9#issuecomment-214913848

— You are receiving this because you were mentioned. Reply to this email directly or view it on GitHub https://github.com/laravel/internals/issues/9#issuecomment-214916124

robclancy commented 8 years ago

Then you are using casts wrong and shouldn't do that. Why would you ever cast something and then save it? Casting happens as the last thing to send a response. And even if you did need to save after casting it you have a cast method that changes the model attributes in some way that is a bug in your code because you are doing it wrong.

robclancy commented 8 years ago

The exact thing could happen with attribute mutators because it is simply not how you should do things.

AdenFraser commented 8 years ago

In the case of a Money cast, something like this could be very simple (and relatively useless) use case:

    $invoice->balance->reduce (2.50):
    $invoice->due_date->addYear ();
    $invoice->save ();

The resulting row update for the model would perform the Money reduction and the Date addition based on the casted objects __toString () return value... On 27 Apr 2016 00:21, "Aden Fraser" wrote:

If you cast to datetime and simply use Carbon's ->addDay () method before saving... the date stored in the database will be one day in the future than it was before.

For instance

    $product->released_at->addDay ();
    $product->save ();

On the save action the Carbon instance is converted __toString in the format the Eloquent date format is required in... with any modifications.

The same way if you modify a casted json array and subsequently save... it gets updated. Or an array or any ither cast for that matter. On 27 Apr 2016 00:16, "Robert Clancy (Robbo)" notifications@github.com wrote:

Yes, for the toArray method which then goes to a JSON response. It doesn't touch the underlying attributes? Casting an object returns the casted attributes, it doesn't change anything you can save like normal (unless I am missing something, and if I am I would say that is a bug).

On Wed, Apr 27, 2016 at 9:03 AM Tom Schlick notifications@github.com wrote:

https://github.com/laravel/framework/blob/5.2/src/Illuminate/Database/Eloquent/Model.php#L2484

Shows how the dates are formatted with the serializeDate() method

— You are receiving this because you commented. Reply to this email directly or view it on GitHub https://github.com/laravel/internals/issues/9#issuecomment-214913848

— You are receiving this because you were mentioned. Reply to this email directly or view it on GitHub https://github.com/laravel/internals/issues/9#issuecomment-214916124

robclancy commented 8 years ago

What? You are making zero sense. What has that code got to do with casting at all?

AdenFraser commented 8 years ago

This is achievable using mutators, you are correct. But i don't understand how you can claim that casting can only be the last thing that happens before we send a response.

If that's the case then why is Eloquent casting dates to Carbon objects? Presumably based on what you are saying, just for response purposes, rather than for providing an API for manipulating dates.

robclancy commented 8 years ago

It doesn't even matter if it is last or not. Casting doesn't change the attributes at all. Eloquent casts to Carbon completely separate to casting. Casting has a single use, to cast attributes to their types for use in a JSON response. Casts are done when toArray is called and that is then JSON encoded.

robclancy commented 8 years ago

Dates are mutated: https://github.com/laravel/framework/blob/5.2/src/Illuminate/Database/Eloquent/Model.php#L2867

Casts have nothing to do with this. Casts ONLY return different types for the JSON response. They do not modify attributes in any way (unless you do it wrong after the proposed PR is merged).

AdenFraser commented 8 years ago

No it's not separate at all, try it out for yourself. Add a few extra columns, date, datetime, collection and cast them. Look at the Eloquent Model API:

 public function getAttributeValue($key)
    {
        $value = $this->getAttributeFromArray($key);

        // If the attribute has a get mutator, we will call that then return what
        // it returns as the value, which is useful for transforming values on
        // retrieval from the model to a form that is more useful for usage.
        if ($this->hasGetMutator($key)) {
            return $this->mutateAttribute($key, $value);
        }

        // If the attribute exists within the cast array, we will convert it to
        // an appropriate native PHP type dependant upon the associated value
        // given with the key in the pair. Dayle made this comment line up.
        if ($this->hasCast($key)) {
            $value = $this->castAttribute($key, $value);
        }

        // If the attribute is listed as a date, we will convert it to a DateTime
        // instance on retrieval, which makes it quite convenient to work with
        // date fields without having to create a mutator for each property.
        elseif (in_array($key, $this->getDates())) {
            if (! is_null($value)) {
                return $this->asDateTime($value);
            }
        }

        return $value;
    }

Clearly when you call for an attributes value, if a mutator doesn't exist the next operation is to check for a cast AND PERFORM CASTING if a cast exists.

robclancy commented 8 years ago

Then I stand corrected. And disagree with the casting working like that altogether. And also then disagree with custom casting doing more than just small things because of reversing being needed and when you add that you have just made attribute mutators.

AdenFraser commented 8 years ago

But you have made attribute mutators which can readily be reused, on multiple models and multiple attributes without the duplicate code bloat required by using mutators.

robclancy commented 8 years ago

Then instead I think you should be adding the ability to do exactly as you just described. Not manipulating casts into a form of mutators that are a little more reusable. Just make current mutators reusable.

AdenFraser commented 8 years ago

@robclancy Now that is a new tanget of discussion entirely and one perhaps worth exploring. It all comes down to the expected functional difference between a cast and a mutator and making that clearly defined.

robclancy commented 8 years ago

Well the definitions are fairly clear. One is for casting types and one is mutating values. Once casts do more than change type they are mutating.

robclancy commented 8 years ago

This could be as simple as the base model having methods (or a trait) like usdGetMutator and usdSetMutator. Then a $mutate array working similar to the casts array.

But with better names for things.

AdenFraser commented 8 years ago

Sure. But if you are casting a monetary figure from a column into a monetery data type, then it's not exactly being mutated (because the underlying value hasn't immediately chnaged) it has just been cast to a different data type.

According to the eloquent docs: The $casts property on your model provides a convenient method of converting attributes to common data types.

The supported cast types are: integer, real, float, double, string, boolean, object, array, collection, date, datetime, and timestamp.

robclancy commented 8 years ago

If that can easily save back then sure casts can work for that. And I still think we need to be able to do our own castings but it should be for very simple things. Anything more should be a mutator.

robclancy commented 8 years ago

Also if you clone the object before modifying it in the custom cast then this reverse issue goes away for the most part.

AdenFraser commented 8 years ago

Obviously it depends largely on use scenario. The docs suggest accessors and mutators for lowercase, uppercase, titlecase etc for first_name and I completely agree with that.

Casting to a Laravel Collection is provided as one of the cast types but I don't see any reason why an extended instance of a Collection shouldn't be castable too.

AdenFraser commented 8 years ago

I don't understand how the reverse issue is an issue. For instance if I store a json_encoded array of data in a column and I cast it to a collection, I want to be able to do things such as:

     $casts = [
         'casted_collection' => 'collection'
     ];
     $model->castedCollection->prepend('new first value')->push('new end value');
     $model->save();

And when the Eloquent Model is saved, the casted Collection is converted back to JSON, thanks to:

    /**
     * Convert the collection to its string representation.
     *
     * @return string
     */
    public function __toString()
    {
        return $this->toJson();
    }

And thus my revised values are stored in the database.

This is far simpler than having to define this combo in the model, no?

    public function getCastedCollectionAttribute($value)
    {
          return new Collection(json_decode($value));
    }

    public function setCastedCollectionAttribute($value)
    {
          $this->attributes['casted_collection'] = $value->toJson();
    }
robclancy commented 8 years ago

That is a very weak way of attribute mutating restricted to just __toString().

AdenFraser commented 8 years ago

It's late and I don't quite follow, so I'll let someone chime in and await some feedback on the WIP PR.

But as far as I'm aware this can prove to be a useful piece of additional functionality and I'm sure others agree but I'll wait for some other voices to confirm one way or another.

JosephSilber commented 8 years ago

I for one agree with Aden: casts should definitely work both ways.

robclancy commented 8 years ago

Then they should just be more "global" mutators.

tomschlick commented 8 years ago

On another note, the approach by @barryvdh was very nice. I would only suggest one thing, if only one param is submitted it should just be the class name and we would assume there is a toDatabase() / fromDatabase() or some default method on the class from the interface.

TypeCaster::extend('html', App\Html::class);
tomschlick commented 8 years ago

That allows you to do the class imports and leaves less room for error as its no longer in a string.

michaeldyrynda commented 8 years ago

Mutating should go both ways, but I'd be inclined to leverage __toString in most (all?) cases, given you're going to be putting text representations of your data into the database most of the time, right? Even a binary field is a textual representation of that content.

Or is the intention to store the serialized object wholesale into the database and unserialize it? In @AdenFraser's earlier example, he's using a mutator (setOriginalPriceAttribute) to explicitly call a method on the Money value object. Would it not make sense to just leverage __toString? Then again, you still have issues in dealing with null.