jessarcher / laravel-castable-data-transfer-object

Automatically cast JSON columns to rich PHP objects in Laravel using Spatie's data-transfer-object class
https://jessarcher.com/blog/casting-json-columns-to-value-objects/
MIT License
328 stars 23 forks source link

Add support for passing flags to json_encode() #8

Closed jrmajor closed 3 years ago

jrmajor commented 3 years ago

This PR introduces support for passing flags to be used when serializing DTO to JSON.

class User extends Model
{
    protected $casts = [
        'json_column' => SomeDTO::class.':preserveZeroFraction,prettyPrint,invalidUtf8Ignore',
    ];
}

This example would result in following flags being used:

json_encode($dto->toArray(), JSON_PRESERVE_ZERO_FRACTION | JSON_PRETTY_PRINT | JSON_INVALID_UTF8_IGNORE);

The PR is based on top of changes in #7 and resolves #5.

jessarcher commented 3 years ago

Thanks once again for your awesome work!

I am a little hesitant to use cast arguments for this - mostly due to the stringiness of them, but also because I'm worried there might be a better use for it in the future.

I was originally thinking we'd just hardcode JSON_PRESERVE_ZERO_FRACTION on the json_encode() call, but I do like the configurability you've created here, especially with how it ties into $options from the Jsonable interface.

What are your thoughts on adding them as a private property on the DTO class instead?

E.g.

class SomeDTO extends CastableDataTransferObject
{
    private int $jsonEncodeFlags = JSON_PRESERVE_ZERO_FRACTION | JSON_PRETTY_PRINT | JSON_INVALID_UTF8_IGNORE;
}

In addition to being able to use the constants, it also centralises the configuration for that DTO, because I imagine it would always want the same flags.

jrmajor commented 3 years ago

I am a little hesitant to use cast arguments for this - mostly due to the stringiness of them, but also because I'm worried there might be a better use for it in the future.

Strings are how Laravel decided to do it, and they still can be used in the future for some other functionality. Only arguments for existing JSON flags are accepted, others will throw an InvalidArgumentException, so we might add more, unrelated to serialization, if we ever need to.

What are your thoughts on adding them as a private property on the DTO class instead?

I'm not sure whether I like the idea of polluting DTOs with JSON flags. Cast arguments sound like a good place to configure casters.

In addition to being able to use the constants, it also centralizes the configuration for that DTO, because I imagine it would always want the same flags.

That is a good point. If you prefer this solution, I can try to implement it.

jessarcher commented 3 years ago

I'm not sure whether I like the idea of polluting DTOs with JSON flags. Cast arguments sound like a good place to configure casters.

Yeah, that's fair. What do you think about this alternative:

$casts = [
    'preserve_zero_fraction' => TestDataTransferObject::class.':'.(JSON_PRESERVE_ZERO_FRACTION | JSON_PRETTY_PRINT),
];

The syntax is a little gross with the parenthesis, but it does get us back to constants. Plus it makes the cast logic a lot simpler because it just needs to inval() the argument. The whole int->string->int thing doesn't smell great though.

jrmajor commented 3 years ago

The syntax is a little gross with the parenthesis

Yeah, to be honest, I don't like this syntax, also it would make adding additional cast arguments in the future harder. Camel cased string parameters seemed like the most Laravel way of doing this.

However, since we're dropping support for PHP 7.4 anyway, I have one more idea:

#[CastUsingJsonFlags(encode: JSON_PRESERVE_ZERO_FRACTION | JSON_PRETTY_PRINT)]
class SomeDTO extends CastableDataTransferObject {}

It would also centralize configuration for cases where one DTO is used in multiple models.

jessarcher commented 3 years ago

This is really nice! :heart_eyes:

Do you think that covers everything for the next major release?

jrmajor commented 3 years ago

@jessarcher Looks good to me!