Skip to content
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

[8.x] Allow serializing custom casts when converting a model to an array #34702

Merged
merged 4 commits into from
Oct 6, 2020
Merged

[8.x] Allow serializing custom casts when converting a model to an array #34702

merged 4 commits into from
Oct 6, 2020

Conversation

ragulka
Copy link
Contributor

@ragulka ragulka commented Oct 6, 2020

This PR allows serializing custom value objects when a model is converted to an array even if the value object does not implement the Arrayable interface. There are no breaking change, new functionality is covered with tests.

Reasoning

Consider a custom caster that casts the value to a value object, say a BigDecimal instance. When calling $model->toArray(), the value is not serialized to a primitive type like a string - it remains a BigDecimal object instance:

$model->toArray(); // ["price" => Brick\Math\BigDecimal {#2254}]

This is inconsistent with dates, which are cast to Carbon instances, but serialized when converting the model to an array:

$model->toArray(); // ["created_at" => "2020-10-05T07:49:06.000000Z"]

A comment suggests that the value object should implement the Arrayable interface to serialize the value. However, there are cases where this may not be possible, feasible or would not make sense for the class itself.

  • If the value object is an instance of a 3rd party class that is declared final.
    Since final classes cannot be extended, the only way would be to use composition to create a custom wrapper around the final class. This might work fine for simpler classes, but for something like BigDecimal, the maintenance overhead and code duplication would make it non-feasible (some 30+ methods), especially for smaller projects and teams.
    This issue was brought up in laravel/deas as well: Add JSON serialization for cast attributes that implement Jsonable or JsonSerializable interface ideas#2152
  • Creating a toArray() method for a value object that represents basically a scalar value makes little sense. What would be the expected return value of (new Decimal(123.45))->toArray()?

Furthermore, dates already have special handling serializeDate in core (see below) - why not allow custom serialization for custom casts?

// If the attribute cast was a date or a datetime, we will serialize the date as
// a string. This allows the developers to customize how dates are serialized
// into an array without affecting how they are persisted into the storage.
if ($attributes[$key] &&
($value === 'date' || $value === 'datetime')) {
$attributes[$key] = $this->serializeDate($attributes[$key]);
}
if ($attributes[$key] && $this->isCustomDateTimeCast($value)) {
$attributes[$key] = $attributes[$key]->format(explode(':', $value, 2)[1]);
}
if ($attributes[$key] && $attributes[$key] instanceof DateTimeInterface &&
$this->isClassCastable($key)) {
$attributes[$key] = $this->serializeDate($attributes[$key]);
}

What's the use case? Why do we need the values to be serialized in the array at all?

  • Consistency with how dates are handled
  • Being able to produce a raw array of primitive model attributes with factories:
     Product::factory()->make(['price' => 12.345]);
     // ['price' => '12.345' ] instead of ["price" => Brick\Math\BigDecimal {#2254}]

Why not simply use Jsonable / JsonSerializable like suggested in laravel/ideas#2152 ?

This would probably work fine for most cases - but custom serialization adds more flexibility. If dates already allow custom serialization/formats why not make it possible for custom casts as well?

Why not cast the model to JSON and the decode it back to an array?

I thought about it and while it's possible to do a little encoding dance:

json_decode($model->toJson()); // ['price' => '123.45']

... it's far from elegant, adds extra overhead (including mental) and just feels ... meh.

Conclusion

I think custom casts is a really powerful feature, especially if it's possible to ease the adoption when casting to 3rd party library classes.

This is my first PR to Laravel, so please go easy on me :) I've tried to follow the core naming and code structure conventions, however feel free to make edits or suggestions there.

If you think this contribution would be worthwhile but needs a different implementation, just let me know. I'm open to all ideas and suggestions.

@taylorotwell taylorotwell merged commit 3722acd into laravel:8.x Oct 6, 2020
@ragulka
Copy link
Contributor Author

ragulka commented Oct 6, 2020

🙌

@shaffe-fr
Copy link
Contributor

Thanks @ragulka for this PR :)

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

Successfully merging this pull request may close these issues.

4 participants