Skip to content
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

Support value types in serialization #1569

Merged
merged 3 commits into from
Nov 24, 2024

Conversation

gregtyler
Copy link
Contributor

@gregtyler gregtyler commented Nov 11, 2024

Q A
Bug fix? yes
New feature? yes (kinda, but I think this is how you'd expect the library to work)
Doc updated no (not applicable)
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets #1568
License MIT

Allows serialization of value types (i.e. true and false, as opposed to bool), including in union types (e.g. string|false).

Otherwise, tries to create class with FQCN "true" and, for union types, filters out value types as non-primitive.

Fixes #1568

@gregtyler
Copy link
Contributor Author

I wasn't sure where best to add tests for SerializationGraphNavigator, but I did find an appropriate test suite for the union types inclusion.

Allows serialization of value types (i.e. `true` and `false`, as opposed to `bool`), including in union types (e.g. `string|false`).

Otherwise, tries to create class with FQCN "true" and, for union types, filters out value types as non-primitive.

Fixes schmittjoh#1568
@scyzoryck
Copy link
Collaborator

@gregtyler I added some end to end tests for serialisation. Unfortunately I was not able to push to your branch :(
ea7f7be

@goetas @gregtyler there is one edge that we need to cover - what if we try to deserialize value true into false type. Looking at other types it seems like NonCastableTypeException should be thrown - we can check type inside DeserializationVisitorInterface::visitBoolean() method. WDYT?

From `ea7f7be4cb6ca2d1ed5e156b57f1413be8936e82`, but I can't cherry-pick because it's cross-remote
@gregtyler
Copy link
Contributor Author

@scyzoryck I've copied your changes onto the PR (I couldn't figure out how to cherry-pick across forks unfortunately).

On the deserialization error, I think the PHP error message caused by type-breaking is already very effective:

Cannot assign true to property JMS\Serializer\Tests\Fixtures\DataFalse::$data of type false

You could test for that in visitBoolean instead, but I think a useful error message would basically say exactly the same thing.

I added that to the test (to expect a TypeError), but I didn't add an expectation on the message because it's PHP internals and varies between versions.

@scyzoryck
Copy link
Collaborator

Thanks! I will think a bit about exceptions in this case - but it should be merged this week :)

@scyzoryck scyzoryck requested a review from goetas November 18, 2024 20:48
@scyzoryck
Copy link
Collaborator

Soo, it looks like the handling of incorrect is inconsistent right now in deserialization - as sometimes \RuntimeException will be thrown, sometimes JMS\Serializer\Exception\NonVisitableTypeException depending on the type of data.
Current solution sounds good for now, but looks like we need to try to organize it a bit better in the future.

@scyzoryck scyzoryck merged commit 6b86447 into schmittjoh:master Nov 24, 2024
20 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Error serializing union types between class and boolean value types
2 participants