Data v4

[rubenvanassche]

#407

[sebastiaanluca]

Wanted to give a heads up on fa428ce#diff-ae2f481eb5439ef575f8131c7b806281fcdafe257235badb387a90da175fea7aR40. It's not really mentioned in the upgrade guide, but it was a big breaking change that took a bit of work to get straightened out. Maybe it's worth mentioning, but I'd also like to get some feedback on it.

Basically we have a lot of models with extra custom attributes (methods). This new functionality triggers/uses ALL of the optional attributes (not included in $appends) on all models, some which perform queries or complex logic, while the data is not really requested. ->only() is only evaluated after, $appends is not checked, and it doesn't take into account the target properties on the data object.

E.g. in one case, we have a data object with only a name and a slug:

<?php

declare(strict_types=1);

namespace Shared;

use Spatie\LaravelData\Data;
use Spatie\TypeScriptTransformer\Attributes\TypeScript;

#[TypeScript]
final class ExampleData extends Data
{
    public function __construct(
        public readonly string $name,
        public readonly string $slug,
    ) {}
}

And we create it from some models:

ExampleData::collect($models)

In some cases, we can fix it by parsing the Eloquent collection to an array instead:

ExampleData::collect($models->toArray())

But that sometimes introduces other problems because of them being parsed to arrays. This isn't always viable since ->toArray() transforms Carbon dates to strings first, which are in the 2024-02-11T21:36:43.000000Z format, while the package uses DATE_ATOM and expects 2024-02-11T21:36:43Z without the microseconds (no idea why Laravel outputs it that way). Plus it takes a really long time because we have a lot of these.

Is this still the correct way of creating collections though? Followed the upgrade guide and new documentation. From an external point of view, you'd expect it to just do $model->property to get the value, instead of parsing all relations, every custom attribute, …

Another small thing is that passing null to the collect() method now fails because it can't normalize the data, while this worked fine before.

[rubenvanassche]

That's because we try to normalize the data structure to an array before doing anything on it. Since it takes quite a while and a lot of checks before we know what properties are required. Additionally, as you mentioned, based upon the kind of structure provided additional things should happen like with model dates.

I once had an idea how we could build it so that we don't require normalizers anymore, but that is such a big change that we need a v5.

Take a look at the ModelNormalizer, you can make your own version where the attributes aren't included and then have a base data object where you use that normalizer instead of the package one. Then extend all your data objects from that object.

I wouldn't mind a PR which allows users to enable or disable certain parts of the normalizer.

As for collecting null, since we don't know the required output type we cannot return anything so that's why you'll now will have to manually check this first. I've added an option in the next release, when you manually define the output type and null is provided that an empty version of that output type will be returned.

[sebastiaanluca]

Thanks for the explanation, makes sense of course. For now we've reverted the upgrade because we found a few bugs or changes that were too much to handle (i.e. the above, nested data isn't parsed to data objects anymore if you use PHPDoc annotations for collections, objects are now arrays when transforming to TypeScript), but we'll look into it later and see if we can reproduce it to perhaps open issues.

[mgjgid]

We are having the same difficulties with the changes to collections in v4, which is why we are staying on v3.