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

Handle union types for typed properties and docblocks #1330

Merged
merged 11 commits into from
May 11, 2022

Conversation

dgafka
Copy link
Contributor

@dgafka dgafka commented Jul 23, 2021

This handle union types for typed properties and DocBlock type resolver.

The solution allows for defining union types for above, which was previously failing during cache building phase.
As union type is non implicit in what we are actually serializing / deserializing, we are not trying to recognize which type user want to use in specific context.

How does serialization works?
Serializing knows which type to use, as it actually holds the value that is going to be serialized. It use that value to infer the type and do the serialization.

How does deserialization works?
The deserialization will throw an exception as we can't guess, which type user would like to pick.
If developer will define Type Attribute, it will be chosen as default one.
This is a place for improvement. Possible we could introduce custom union deserializing handler, that would decide on his own what class he would like to deserialize too.

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

@dgafka
Copy link
Contributor Author

dgafka commented Jul 23, 2021

ping @yyaremenko @gam6itko :)

@goetas
Copy link
Collaborator

goetas commented Nov 13, 2021

i'm sorry for the delay, @dgafka could you please rebase this?

@dgafka
Copy link
Contributor Author

dgafka commented Feb 20, 2022

@goetas branch is up to date with master now :)
I am not sure, how to fix phpstan or phpcs for lower PHP versions, can you do it?

@gnat42
Copy link
Contributor

gnat42 commented Feb 26, 2022

Hello, trying to upgrade a project to 8.1 and running into failed tests because of this. How can I help to get this merged/tested etc?

@davidwindell
Copy link

Just tested this on our app and can confirm it seems to work well for us.

@dgafka
Copy link
Contributor Author

dgafka commented Apr 25, 2022

Hey @goetas,
Can this be merged? :)

Copy link
Collaborator

@scyzoryck scyzoryck left a comment

Choose a reason for hiding this comment

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

@dgafka - Could you take a look at the code style issues that has been reported by the coding standard, please? I believe they can be fixed automatically by the tool. Also you need to exclude your fixture from PHPStan analysis - as in CI it is running with PHP7.4 that do not support union types.

@dgafka
Copy link
Contributor Author

dgafka commented Apr 27, 2022

@scyzoryck how to fix that:

  Line   tests/Metadata/Driver/TypedPropertiesDriverTest.php                                                                                                                            
 ------ ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- 
  15     ParseError (syntax error, unexpected '|', expecting variable (T_VARIABLE)) thrown while looking for class JMS\Serializer\Tests\Fixtures\TypedProperties\UnionTypedProperties.  
         💡 Learn more at https://phpstan.org/user-guide/discovering-symbols                                                                                                             
  53     ParseError (syntax error, unexpected '|', expecting variable (T_VARIABLE)) thrown while looking for class JMS\Serializer\Tests\Fixtures\TypedProperties\UnionTypedProperties.  
         💡 Learn more at https://phpstan.org/user-guide/discovering-symbols 

as excluding it from phpstan analyse does not change a thing:

    excludePaths:
        - %currentWorkingDirectory%/src/Annotation/ReadOnly.php
        - %currentWorkingDirectory%/tests/Fixtures/TypedProperties/UnionTypedProperties.php

@dgafka
Copy link
Contributor Author

dgafka commented Apr 27, 2022

I've put the test under phpstan ignore too. Tests are passing now.

tests/Metadata/Driver/UnionTypedPropertiesDriverTest.php Outdated Show resolved Hide resolved
phpstan.neon.dist Outdated Show resolved Hide resolved
@dgafka
Copy link
Contributor Author

dgafka commented May 5, 2022

Extracted separate phpstan analysis for PHP 7.4.
The rest of the versions are running against those files :)

Copy link
Collaborator

@scyzoryck scyzoryck left a comment

Choose a reason for hiding this comment

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

Thanks! Looks good for me! @goetas - Could you take a look, please?

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.

5 participants