-
Notifications
You must be signed in to change notification settings - Fork 11.1k
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.3] [WIP] Custom Type Casting #13315
Conversation
I've noticed this is breaking casting to 'datetime' (a Carbon instance) - fixing now. |
I think there needs to be a way for the cast class to access the model, or at least multiple fields. For instance if you had a payments table utilizing the |
I agree that would be useful, I'm not sure how best to implement that without making the process of creating a Casting class more conveluted. Thoughts? |
maybe something like this? /**
* The attributes that should be cast to native types.
*
* @var array
*/
protected $casts = [
'amount' => [Money::class, 'currency'],
'amount' => '\App\Money,currency',
'amount' => '\App\Money,£',
]; the thought is if the parameter == a field name then inject that field, if not then just inject that string. |
See my comment here: laravel/ideas#9 (comment) This seems a bit confusing and not in line with other Laravel declarations (eg. routes/validators are |
Also, probably better to use the Laravel App container to build your classes, instead of reflection, if you go this way. |
@barryvdh Agree with your comments laravel/ideas#9 (comment) @tomschlick I think to avoid unexpected conflicts of field values as parameters what you've proposed would need to be more inline with the way Routing and Middleware parameters work - where {field} is injected with it's dynamic value, for instance protected $casts = [
'amount' => '\App\Money,{currency}',
]; Extended to @barryvdh's suggestion would look like: protected $casts = [
'amount' => 'money,{currency}',
]; Which I think is clean and inline with Routing, Middleware, Validation, etc. |
I do agree that building the class through the App container is the way to go! |
Yeah 👍 for that. |
Note that we cannot do that in an eloquent model. It cannot be coupled like that. |
What would be the reason for resolving it out of the container? You should only ever cast to value objects, and those shouldn't ever have dependencies. For example, if you want the ability to convert between different currencies at the current rate, you should have a separate service that handles that; your |
@JosephSilber I largely agree, although I wonder if there would be a use case where someone would like to resolve an interface implementation of 'Money'. I'm not to sure of the practicality of that though. |
71c12e0
to
3d891e7
Compare
*/ | ||
protected function castExists($key) | ||
{ | ||
if (! $this->castClass($key)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
return $this->castClass($key);
I'll be attempting to wrap this up later this week, I'm currently transitioning between machines so a few things have had to go on the backburner. |
If it is OK to use PHP serialization, It will be possible to support inheritance as well. Please take a look at #13511 |
Where are we at with this? :) |
Sorry been hectic with a change in jobs. Will sink my teeth into this Happy to open up write access to my fork (if that's possible?) If others Definitely need to find time for this as I hope it can make it into 5.3 ;)
|
I have an alternative implementation of this that I think addresses all concerns fairly elegantly. (Resolving stuff from IoC, mutating multiple fields into one aggregate value object, etc.) I'll try to get it pushed up as a PR today so y'all can take a look and see what you think. |
While I have an implementation, I am still curious why people would prefer this approach over just defining |
Using accessors and mutators means you have to create a set of those for every single attribute that you want it in, in every model.
Using casting means you define the functionality once, then simply add the attribute names to the casts array.
Much less boilerplate this way.
|
@taylorotwell Awesome, I look forward to seeing what you decided to put together! |
I have a question. If we change the casted object, will the model get affected as well? e.g: $model->money = '10$';
$model->money->add(10)
echo $model->money; is |
When the save action is used, it would be expected that your casted option Presumably most people would want the new value to be stored in the
|
@AdenFraser sorry don't find my answer. I wasn't talking about the saved value in the database, I am considering the in memory values. Let me put it this way, if you read assert($model->money === $model->money); |
Once the value is casted, you will receive an object. So you'd have to In memory it would behave the same as accessory and mutations- the object
|
Yeah I think that value object casting would need to be cached and only done once per model like Hamid is saying.
|
Let's take up discussion here. |
This is an early draft of some custom type casting functionality, discussed here:
laravel/ideas#9
The proposed implementation would allow Model Attributes to be cast to custom classes, with optional parameters.
For instance:
Where the cast class is instantiated with the attribute value as the first parameter, followed by any additional parameters provided.
Assuming a casting class has a __toString() method, this will be used when inserting the attribute back into the database.
A simple custom class example could be automatic casting to HtmlString (useful for WYSIWYG stored content). Creating additional cast classes for Value Objects, such as Money, Currency, Url etc would be as straightforward as creating a class with a __construct and __toString().
This could certainly be made stricter, perhaps with a Castable interface to be implemented on castable objects but it works nicely as above.
This is an early draft to gather some feedback, thoughts?
:)