Skip to content

Conversation

@Spea
Copy link
Contributor

@Spea Spea commented Nov 28, 2025

Took a bit longer than initially anticipated to get this done, but with this change (union) discriminators are now parsed into proper meta data which can then be used to create the corresponding (de-)serialize classes in liip-serializer.

Its probably worth to mention that I tried to mimic the behaviour of what JMS is doing for certain cases (like having multiple primitive union types).

@Spea Spea force-pushed the suppport-discriminator branch from 20c457b to 0e34b15 Compare November 28, 2025 13:42
@Spea Spea marked this pull request as ready for review November 28, 2025 14:02
Copy link
Member

@dbu dbu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice, thanks a lot for implementing this feature!

i have one small detail, apart from that this looks good to merge to me. as we don't have extensive documentation, i don't think there is a place where this should be documented. however, please add a note to the changelog.

Spea added 6 commits December 1, 2025 11:10
With this addition, the class metadata now also holds the discriminator information (if any exists).
This class was only used in the `Parser` class, but even in there nothing was really done with it, so we can remove it.
With this change we also allow the following additional primitive types: `false`, `true`, `null`.
With this change one can now use primitive union types which will be properly (de-)serialized when using the `ReflectionParser`.
One can now use this attribute to define more complex union types.
@Spea Spea force-pushed the suppport-discriminator branch from 0e34b15 to 9b7f781 Compare December 1, 2025 10:13
@Spea Spea force-pushed the suppport-discriminator branch from 9b7f781 to 15aa305 Compare December 1, 2025 10:14
@Spea
Copy link
Contributor Author

Spea commented Dec 1, 2025

i have one small detail, apart from that this looks good to merge to me. as we don't have extensive documentation, i don't think there is a place where this should be documented. however, please add a note to the changelog.

Added a changelog entry now, I somehow keep forgetting this, sorry 😞

$type = $type->getLeafType();
}
if ($type instanceof PropertyTypeClass) {
$this->parseModel($type->getClassName(), $context->push($property), $registry);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

from the context class, it looks like it was made to provide the reference to where the error happened in case something does not work. but it is completely unused, so i agree we can drop it.

@dbu dbu merged commit 2501a62 into liip:2.x Dec 1, 2025
7 checks passed
@dbu
Copy link
Member

dbu commented Dec 1, 2025

do you need further tests or shall i tag a release?

@Spea
Copy link
Contributor Author

Spea commented Dec 1, 2025

do you need further tests or shall i tag a release?

Fine for me if you tag a new release now, thanks 🙇

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.

2 participants