[10.x] Support array syntax for casts

[innocenzi]

Currently, to pass arguments to casts, we use string concatenation:

protected $casts = [
    'secret' => Hash::class.':sha256',
];

This pull request implements the array syntax to perform the same thing, but in a more readable way:

protected $casts = [
    'secret' => [Hash::class, 'sha256']
];

protected $casts = [
    'songs' => [DataCollection::class, SongData::class],
];

[taylorotwell]

I'm a little concerned about the breaking change to getCasts with this PR. There could be a fair number of packages or apps that read the results of this method (since it is public) and expect the values to only be strings. I think if we can make this change, the getCasts method should format any array values back to strings, but that does potentially add a bit of overhead as we have spin the casts array each time getCasts is called.

[innocenzi]

In my very unscientific testing, the overhead for a model with 15 casts, 5 of which are arrays, is 0.001ms:

Benchmark::dd(function () use ($_casts) {
    array_map(static function (array|string $castType) {
        if (is_array($castType)) {
            [$castType, $arguments] = [array_shift($castType), $castType];
            $castType = $castType . ':' . implode(',', $arguments);
        }

        return $castType;
    }, $_casts);
}, 15_000);

I don't think this overhead will be noticeable, and we still have the solution to add a cache if it is. Your suggestion is also better, less code needs to be changed. I'll update the PR 👍

[innocenzi]

@johanrosenson This is not valid PHP, you can only use literals and constant expressions to initialize class properties... I also wanted that syntax myself but that's not a thing, unfortunately. :(

[johanrosenson]

@innocenzi

You are 100% correct, i didn't think about exactly where the casts are defined, too bad, it would have been neat :)

[taylorotwell]

@johanrosenson I agree and have often wanted that syntax myself but, as noted, it's not allowed by PHP. We would have to have a method (instead of a property) that returns an array of casts for the model.

[taylorotwell]

One concern I have with this PR is performance. We've now added a loop over all of the defined casts everytime getCasts is called, which can be quite often I think, especially if you are hydrating or interacting with a lot of models in a loop. Any thoughts or benchmarks there?

[innocenzi]

As I said in my previous comment, I did a basic benchmark — the loop wasn't noticeable. But if this is still a concern, we can add a static cache to only perform the loop once?

[innocenzi]

Well, tried to add cache, but tests fail because it's static and isn't reset between tests. I'm not sure if a static cache there could cause troubles in a real app though. Should I just fix the tests? Or should I remove the cache?

[donnysim]

The static property needs to be defined on each model itself to be cache per model instance, otherwise it's shared across all models and the key probably should have a model class prefix, but I don't think a static cache is a good option, because you can specify casts dynamically per model basis, as in with a query builder:

MyModel::query()->withCasts(['my_joined_prop' => 'bool'])->get();

which would break.

[innocenzi]

You're totally right that the cache is being shared, my bad. For the dynamic casts though, you're right but we could simply clear the cache when this happens.

EDIT: I've been trying to clear the cache where needed, but honestly I think the cache i overkill and adds too much complexity — I couldn't see any performance penalty in my tests

[taylorotwell]

@innocenzi Can you elaborate on your benchmark. I don't think it's 0.001ms for 15,000 iterations. 0.001 is the average amount of time the callback took across the 15,000 iterations - not the sum of all iterations. I would be more curious about the sum of 15,000 invocations of getCasts.

[innocenzi]

Yeah it was definitely the average, sorry if I wasn't clear. It should be a total of ~15ms overhead for 15k getCasts invocations. I'll try running a basic benchmark to see for myself.

[innocenzi]

So, here is another benchmark, the average time for 15k getCasts calls (1000 iterations, including the creation of the model itself) was about the same:

• Initial implementation of this PR (without cache): 194.3ms
• Prior to this PR: 193.2ms

Benchmark code

$durations = [];

for ($iteration = 0; $iteration < 1000; $iteration++) {
    $stopwatch = new Stopwatch(true);
    $stopwatch->start('default');

    for ($call = 0; $call < 15_000; $call++) {
        Listing::make()->getCasts();
    }

    $result = $stopwatch->stop('default');
    $durations[] = $result->getDuration();
}

dd(collect($durations)->average());

[taylorotwell]

I see you committed the cache? Can you remove it? I don't think it's totally bulletproof. Casts are an instance level concept and managing them at the static / global level could be problematic. For example, imagine you merge some casts into one model, cache is rebuilt... but the next model you interact with of that same type will use the rebuilt cache even though you never merged that specific cast into its cast definition.

In my own benchmarking of 150,000 invocations I see getCasts taking 40ms before this PR and 160ms after this PR (without cache). This is on a model with 11 cast attributes.

[innocenzi]

Sure, I removed the cache. Can I ask what was the code for your benchmark?

If the performance overhead is definitely an issue, I think the only solution would be to make a backwards incompatible implementation where getCasts can have array values, and these are values are evaluated when they need to be?

[taylorotwell]

$start = microtime(true);

$model = new Flight;

for ($i = 0; $i < 150000; $i++) {
    $model->getCasts();
}

dd(number_format((microtime(true) - $start) * 1000, 2));

Flight had casts like:

protected $casts = [
    'name' => 'string',
    'age' => 'int',
    'votes' => 'int',
    'admin' => 'bool',
    'cancelled' => 'bool',
    'revoked' => 'bool',
    'banned' => 'bool',
    'repairing' => 'bool',
    'options' => 'array',
    'metadata' => 'array',
    'final' => [AsCollection::class, 'something'],
];

[innocenzi]

Right, I have about the same results as you with this benchmark. What do you suggest? Is this acceptable, or should we make this more performant (but with a BC break on getCasts)?

[taylorotwell]

I personally would be interested in brainstorming a way to define casts via a method. That would allow for even better syntax such as invoking methods to build cast strings, etc. while avoiding any performance penalty since the method could return an array of simple strings that don't have to be iterated over.

public function someCastDefinitionFunction()
{
    return [
        'something' => AsCollection::using(CustomClass::class),
    ];
}

[Angel5a]

@taylorotwell Sorry, but, Isn't it simple one:

trait DynamicCasts
{
    protected function initializeDynamicCasts(): void
    {
        if (method_exists($this, 'someCastDefinitionFunction')) {
            $this->casts = array_merge($this->casts, $this->someCastDefinitionFunction());
        }
    }
}

>>> $user->getCasts()
=> [
     "id" => "int",
     "email_verified_at" => "datetime",
     "role" => "App\Enums\UserRole",
     "more_fields" => "Hello:My2iJHkK84bVm0p7",
   ]

Just add a builder, which also could be implemented as a trait to cast-class.

[innocenzi]

Personally I still think exploring the array syntax is worth, even if it means waiting for 11.x, because adding casts to a class property is easier and having to add a method is more cumbersome.

With that being said, the idea of having methods to build cast strings is really appealing, even if it means it has to be in a method.

@Angel5a If this feature has to be implemented using a method, I'd look for a way without traits, because that'd be even more cumbersome. I'm not sure what the overhead would be, but I guess calling the someCastDefinitionFunction could be done in getCasts directly?

As for the name, I like defineCasts, though it's not really a name pattern that's used in Laravel (it is used a lot in TypeScript-land though), but something simple like casts would have too much chances of conflicting with userland code...

[innocenzi]

Using the same benchmark as earlier, getCasts goes from ~30ms (current implementation) to ~40ms when defineCasts doesn't exist and ~60ms when it does exist and returns an array of casts.

Code

// HasAttributes.php
public function getCasts()
{
    if (method_exists($this, 'defineCasts')) {
        $this->casts = array_merge($this->defineCasts(), $this->casts);
    }

    if ($this->getIncrementing()) {
        return array_merge([$this->getKeyName() => $this->getKeyType()], $this->casts);
    }

    return $this->casts;
}

[WendellAdriel]

@taylorotwell @innocenzi

I'll be honest that IDK on how that could be applied here or if this is something that you want as the design for this. But in the DTO package that I created, the cast is defined like this:

protected function casts(): array
{
    return [
        'property' => new CarbonImmutableCast(),
    ];
}

And when needed any parameters we just send it to the constructor:

protected function casts(): array
{
    return [
        'property' => new CarbonImmutableCast('Europe/Lisbon', 'Y-m-d'),
    ];
}

Here's the source code of the CarbonImmutableCast: https://github.com/WendellAdriel/laravel-validated-dto/blob/main/src/Casting/CarbonImmutableCast.php

And the only thing to create custom casts is to implement this simple interface: https://github.com/WendellAdriel/laravel-validated-dto/blob/main/src/Casting/Castable.php

This works like a charm for the package but IDK if this design would be something to bring to the Eloquent casts.

[Angel5a]

@innocenzi My example is just to show, that this is possible without any intrusion onto framework right now.

A word on your sketch. I don't know if call to defineCasts() inside getCasts() should mutate $casts property. There could be multiple calls for getCasts(). If we wan't to make "live casts" available (like, dynamically determine if we need that cast), then we miss the ability to remove the cast. If we doesn't need such a feature, than we can call to defineCasts() inside a model's constructor to minimize the penalty. How many times we are creating models to not use them?

A word on the idea of array syntax. I like it. Nice readable syntax. It's worth to wait 11.x. All this string concatenations looks ugly.

[taylorotwell]

@Angel5a yes, you are correct that is one way to accomplish a method based approach now.

[innocenzi]

@taylorotwell I refactored the PR to use initializeHasAttributes. I think @Angel5a's suggestion was great, and it seems there is no breaking change with this new implementation since getCasts always return normalized casts now.

If you are interested, I can open a new PR in addition to this one for a defineCasts method that allows using static cast constructors? I personally think this is not much needed though. Static constructors would be cool but a new method would just be cumbersome I think.

[nunomaduro]

@innocenzi Let me know what do you think about this: #47237.

[innocenzi]

@nunomaduro LGTM and I think we can close this PR and track the feature in #47237 👍