Union types deserialisation

[idbentley]

Q	A
Bug fix?	no
New feature?	yes
Doc updated	no
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets	#1502
License	MIT

This is just a minor improvement on #1504 - addressing @goetas feedback, improving type detection and tests.

[scyzoryck]

Thanks for taking up this topic. Unfortunately I'm super short in time recently :(

It looks like there was few issues found by the CI pipeline like:

Error: Class 'JMS\Serializer\Tests\Fixtures\DocBlockType\UnionTypedDocBlockProperty' not found

Also, please use vendor/bin/phpcbf to format the code :) I will take a deeper look tomorrow.

[idbentley]

@scyzoryck I completely understand. I need union support for a project I'm working on, so I'm happy to push this forward. Thanks for taking the time to work with me.

If you can kickoff the workflows again (I don't seem to be able to), I think I've fixed those issues.

[idbentley]

I will look into these phpstan issues tomorrow.

[idbentley]

Hey @scyzoryck I addressed the phpstan issues (sorry, I'm embarrassed I didn't pay more attention to the contributor guide).

However, there were a number of phpstan related issues on a file that is not touched in this pull request: src/Metadata/Driver/DoctrineTypeDriver.php. I can address those too, but I left them out because they seem unrelated to the other code changes. Let me know if you'd like me to apply that patch before we run through CI again.

[idbentley]

Hiya!

I added a commit to only register the UnionHandler on PHPVersion > 8.

Not sure how to get the PHPStan to ignore the | usage in UnionHandler though. It's only used in the return type of the determineType function - as a test I tried to use ?string instead of string|null?

It's not clear to me what the Error: Ignored error pattern #Constructor of class JMS\\Serializer\\Annotation\\.*? has an unused parameter# was not matched in reported errors. error reported by PHPStan means (that error does appear for me locally in master).

[scyzoryck]

Hi! In general it looks good for me as a basic setup. I left one comment to improve performance.

No need to fix src/Metadata/Driver/DoctrineTypeDriver.php in this PR - it requires separate PR. :)

[scyzoryck]

Let's move it into TypedPropertiesDriver. In that way it will be reorder once & cached, not for each deserialisation.

[idbentley]

Good idea.

[idbentley]

Seems like the remaining CI issues are in the DoctrineTypeDriver.php. Let me know if you need anything else from me!

[idbentley]

@scyzoryck Yep, I think we've addressed all issues. I'm happy for you to merge when ready!

[scyzoryck]

@idbentley thanks for contribution! You are welcome with the next Pull Requests! :) Could you test it with your projects & dev version of the package, please? :)

Best, Marcin

[goetas]

it would be great to have this handler integrated in the bundle as well

[idbentley]

@goetas I'm currently working on a PR to expand this functionality to support Objects, arrays, non primitive data types. I thought it would make sense to get it into documentation and any build targets once that PR is ready as well.

[goetas]

@idbentley I have talked a little with @scyzoryck about it, deserializing objects used in unions is certainly a good thing to have but I do not have any idea on how to make it work.... how to decide to which class to map an arbitrary data structure? do you have any idea on how to do so?

[simPod]

@idbentley this crashes here with Class "union" does not exist:

It calls MetadataFactory::getClassHierarchy() with 'union' input.

calling serialize with instance of

final class Foo
{
    public function __construct(
        public A|B $union,
    ) {

[idbentley]

@simPod that's definitely a bad way of handling the issue, and I'm gonna look into more graceful failure. However, I think your main problem is that there is currently only support for unions of primitives at this time. See #1549 to follow progress on my attempt at unions of objects.

[simPod]

Yup, it should ignore unsupported cases. Good luck!

…

On Fri, Jul 19, 2024, 17:35 Ian Bentley ***@***.***> wrote: @simPod <https://github.com/simPod> that's definitely a bad way of handling the issue, and I'm gonna look into more graceful failure. However, I think your main problem is that there is currently only support for unions of primitives at this time. See #1549 <#1549> to follow progress on my attempt at unions of objects. — Reply to this email directly, view it on GitHub <#1546 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AACQAJMTPSFZVE3TR5EV3STZNEW43AVCNFSM6AAAAABKJYMCYGVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDEMZZGQ3TIMBRGM> . You are receiving this because you were mentioned.Message ID: ***@***.***>