-
-
Notifications
You must be signed in to change notification settings - Fork 587
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
Conversation
I wasn't sure where best to add tests for |
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
360825d
to
62f7bb4
Compare
@gregtyler I added some end to end tests for serialisation. Unfortunately I was not able to push to your branch :( @goetas @gregtyler there is one edge that we need to cover - what if we try to deserialize value |
From `ea7f7be4cb6ca2d1ed5e156b57f1413be8936e82`, but I can't cherry-pick because it's cross-remote
8a93991
to
9538cce
Compare
@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:
You could test for that in 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. |
Thanks! I will think a bit about exceptions in this case - but it should be merged this week :) |
Soo, it looks like the handling of incorrect is inconsistent right now in deserialization - as sometimes |
Allows serialization of value types (i.e.
true
andfalse
, as opposed tobool
), 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