Skip to content

Serialization broken since refactor to Types #91

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

Closed
bertramakers opened this issue Dec 6, 2023 · 2 comments
Closed

Serialization broken since refactor to Types #91

bertramakers opened this issue Dec 6, 2023 · 2 comments
Labels
bug Something isn't working

Comments

@bertramakers
Copy link
Contributor

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
Copy link
Owner

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

@tobyzerner tobyzerner added the bug Something isn't working label Dec 6, 2023
@bertramakers
Copy link
Contributor Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants