tobyzerner / json-api-server

A JSON:API server implementation in PHP.
https://tobyzerner.github.io/json-api-server/
MIT License
63 stars 21 forks source link

Serialization broken since refactor to Types #91

Closed bertramakers closed 7 months ago

bertramakers commented 7 months ago

Hi @tobyzerner . I was processing your comments on my PR/issues, which mentioned quite a lot of breaking changes. While refactoring our app to be compatible with the latest version again, I ran into the following issue.

Before the refactor to Types, I had a field like this:

Str::make('nzcEmissionDomain')
  ->writable()
  ->nullable()
  ->default(null)
  ->enum(array_column(NzcEmissionDomain::cases(), 'value')
  ->serialize(fn (?NzcEmissionDomain $emissionDomain): ?string => $emissionDomain?->value)

Note that NzcEmissionDomain is a string-backed enum in our application.

I refactored this particular field to this:

Attribute::make('nzcEmissionDomain')
  ->type(Str::make()->enum(array_column(NzcEmissionDomain::cases(), 'value')))
  ->writable()
  ->nullable()
  ->default(null)
  ->serialize(fn (?NzcEmissionDomain $emissionDomain): ?string => $emissionDomain?->value)

However, when I now include a value for it in a create/update request, I get the following Error:

Object of class App\\V2\\Domain\\Measures\\NzcEmissionDomain could not be converted to string

This is caused by Attribute calling the serialize() method on Str first before calling the custom serialization function (here). And the Str::serialize() method tries to cast the value to a string (here), which will fail if it's a type that cannot be cast automatically. The purpose of the custom serialize method seemed to me to prevent this, so I would assume that it should be called first instead?

tobyzerner commented 7 months ago

Yes you're right! Will fix soon. Sorry for all the breaking changes!

bertramakers commented 7 months ago

Thanks for the quick response! No worries, I knew at least one breaking change was coming for the field names. Extracting the type definitions to separate classes is a bit more work to refactor it on our end now, but I understand your logic and agree it makes sense.