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

Nullable type definitions using php 7.4 typed properties style #1081

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

goetas
Copy link
Collaborator

@goetas goetas commented May 3, 2019

Q A
Bug fix? no
New feature? yes
Doc updated no
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets -
License MIT
  • feature and tests
  • documentation
  • usecase

@goetas goetas changed the base branch from master to metadata-check May 3, 2019 21:37
@goetas goetas changed the base branch from metadata-check to master May 3, 2019 22:12
@simPod
Copy link
Contributor

simPod commented Oct 6, 2020

This should also handle the case when

  1. I serialize object with uninitialized nullable property
  2. Deserialize
  3. The property is uninitialized

Personally, I'd somehow get rid of $context->setSerializeNull(true); setting and made it default.

Unintialized properties would then be not part of serialized content while initialized with null would.

@Tobion
Copy link
Contributor

Tobion commented Dec 7, 2020

If this is meant to change the behavior of #1252 and #1081 (comment) then I don't think it's the right direction. So a property that is not serialized should not initialize the property after deserialization to null. This is because we don't know if the initial property was missing from the serialization because it was unitialized or because of serialize_null = false.

So I think what we need is a different option like serialize_typed_null which defaults to true. Then typed nullable properties are serialized by default which solves #1252. And people can still configure the nullable behavior of non-typed properties independently like before.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants