Skip to content
This repository has been archived by the owner on Jul 16, 2021. It is now read-only.

Improve Typecasting #9

Closed
AdenFraser opened this issue Mar 14, 2016 · 72 comments
Closed

Improve Typecasting #9

AdenFraser opened this issue Mar 14, 2016 · 72 comments
Labels

Comments

@AdenFraser
Copy link

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

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

valorin commented Mar 15, 2016

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

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

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

@alexbilbie
Copy link

Casting to and from a JSON blob would be useful too

@arcanedev-maroc
Copy link

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

@valorin
Copy link

valorin commented Mar 17, 2016

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

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

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

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

@taylorotwell
Copy link
Member

Agree could be cool!

@AdenFraser
Copy link
Author

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

@AdenFraser
Copy link
Author

Thrown together an early draft laravel/framework#13315

@barryvdh
Copy link

barryvdh commented Apr 26, 2016

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

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 [email protected] 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
#9 (comment)

@robclancy
Copy link

Also why would this need converting back?

On Wed, 27 Apr 2016 07:35 Robbo [email protected] 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 [email protected]
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
#9 (comment)

@barryvdh
Copy link

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

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

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 [email protected]
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
#9 (comment)

@tomschlick
Copy link

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

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

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

robclancy commented Apr 26, 2016

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

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)" [email protected]
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 [email protected]
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
#9 (comment)


You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#9 (comment)

@robclancy
Copy link

robclancy commented Apr 26, 2016

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

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

@AdenFraser
Copy link
Author

AdenFraser commented Apr 26, 2016

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)" [email protected]
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 [email protected]
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
#9 (comment)


You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#9 (comment)

@robclancy
Copy link

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

@AdenFraser
Copy link
Author

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.

@sebastiandedeyne
Copy link

sebastiandedeyne commented Jan 9, 2017

What if we'd have the caster mutate the model instead of returning a serialized value? Another example implementation, similar to @barryvdh's:

class MyModel extends Model
{
    protected $casts = [
        'price' => 'money',
    ];
}
class CastsMoney
{
    public function get(Model $model)
    {
        return new Money($model->price_amount, $model->price_currency);
    }
    
    public function set(Model $model, Money $value)
    {
        $model->price_amount = $value->getAmount();
        $model->price_currency = $value->getCurrency();
    }
}

Casts::extend('price', CastsMoney::class);

Advanced: You could pass values as parameters, for example the field names, or a hard coded currency like in the previous example.

class MyModel extends Model
{
    protected $casts = [
        'total' => 'money:total_amount,total_currency',
        'shipping_costs' => 'money:shipping_costs_amount,shipping_costs_currency',
    ];
}
class CastsMoney
{
    public function get(Model $model, $amountField, $currencyField)
    {
        return new Money($model->$amountField, $model->$currencyField);
    }
    
    public function set(Model $model, Money $value, $amountField, $currencyField)
    {
        $model->$amountField = $value->getAmount();
        $model->$currencyField = $value->getCurrency();
    }
}

Casts::extend('price', CastsMoney::class);

I think this would cover everything we want:

  • Define reusable custom accessors and mutators in $casts
  • Getters and setters
  • Create value objects from multiple fields

@tomschlick
Copy link

FYI, a new PR has been opened by @tillkruss to further the work that was started on this.

laravel/framework#18106

For now discussion it is locked as it is a work in progress.

@tillkruss
Copy link

tillkruss commented Feb 25, 2017

That was actually a typo, hence the [WIP] in the title. I fixed it earlier today.

$user->token = 'larav3l-secr3t'; // does not work
$user->save();

@robclancy
Copy link

robclancy commented Feb 25, 2017 via email

@tillkruss
Copy link

Because I don't want people to comment, yet. If you have valuable feedback, ping me in Slack.

@robclancy
Copy link

robclancy commented Feb 25, 2017 via email

@tillkruss
Copy link

tillkruss commented Feb 25, 2017

Valueless, off-topic, thread-hijacking comments like that. That why it's locked.

@robclancy
Copy link

robclancy commented Feb 25, 2017 via email

@franzliedke
Copy link

Ignoring Robbo's comments for a while, I too agree that it would be helpful to allow feedback directly on the PR, especially since it is work in progress. ;)

@robclancy
Copy link

robclancy commented Feb 25, 2017 via email

@franzliedke
Copy link

Well, I don't agree with your tone.

@robclancy
Copy link

robclancy commented Feb 25, 2017 via email

@sisve
Copy link

sisve commented Feb 26, 2017

The current implementation in laravel/framework#18106 has a large flaw; it requires ownership of the objects you cast to. That is often not the case; an example are people using https://github.com/nicolopignatelli/valueobjects which would require an intermediate casting layer. Something like a TokenConverter that has a convertToPHPValue($value) and convertToDatabaseValue($value)

@sisve
Copy link

sisve commented Feb 26, 2017

Can someone explain why this issue is so secretive that I need to talk to @tillkruss in a hidden thread in #internals on Slack instead of commenting in the PR?

Additional things I've told @tillkruss in previously mentioned secretive thread;

  1. The intermediate casting object (hereby known as ConverterThingie) should have an interface, and all methods should be instance methods.
  2. The ConverterThingie should be instantiated using the container so it can have dependencies.
  3. The current implementation will not ask the ConverterThingie about null values. This sounds wrong, the ConverterThingie's sole purpose in life is to convert things, it should be asked how to convert null values.

@barryvdh
Copy link

I don't get it either. If you're not sure about the architecture, discuss that privately before creating a PR.
If you're not ready for feedback, work on it locally before creating a PR.

If you do create a PR, accept feedback on it. These discussions are very valuable to read back later, to understand why something is done this way. Redirecting to Slack just means it's lost after a few days.

I really hope this isn't a trend.. IMHO locking should be reserved for out of control threads with just spam, not reasonable discussions..

@lagbox
Copy link

lagbox commented Feb 26, 2017

Can we please stop this secretive nonsense. The point of this repository is so that [expletive deleted] isn't just on slack which disappears. This creates paper trails so other people can actually see the conversations and IDK maybe not waste their [expletive deleted] time on things that have already been discussed that they couldn't possibly [expletive deleted] know because they weren't on slack. There has to be somewhere this information lives that is accessible.
How many times does this obvious issue have to be pointed out?

@barryvdh agreed pre-emptive heavy-handed moderating is not good

@tillkruss
Copy link

tillkruss commented Feb 26, 2017

Unlocked the thread and closed the PR. Will open new PR when it's ready.

@sisve
Copy link

sisve commented Mar 8, 2017

For those that missed it; a new PR was opened.

Every single point I made was ignored.

  1. There is no "casting object" that can declare constructor dependencies needed for conversion.
  2. No interfaces for this new functionality to conform to, instead it's specially named methods that need to implemented in every model.
  3. Null values are still ignored by the casting layer.

A quick reminder from that hidden-in-history Slack thread.

image

Did any of that happen?

@sisve
Copy link

sisve commented Mar 8, 2017

It's way early in the morning and I misread the PR. It's not yet merged, and I updated my previous comment to remove that part.

I know I am bitter and filled of hatred, but could there at least be some notes about the different suggested implementations that show why those were declined? It's almost a year since the most thumbed-up (which I think means some kind of approval) comment suggested a way to do this, why wasn't that implemented? #9 (comment)

@robclancy
Copy link

I mean... his first PR showed he doesn't care what other people think. Instead of closing he will just ignore everyone because he is better than them (unless one of the Laravel "inner circle twitter gods").

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

No branches or pull requests