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

Union types deserialisation #1504

Closed

Conversation

scyzoryck
Copy link
Collaborator

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

Lets add initial support for Union types from JSON. Limitations:

  • only for simple types for now
  • only from JSON

@idbentley
Copy link
Contributor

@scyzoryck I am using serializer pretty intensively for some SDK tasks, but I need union support to move forward.

I'm very happy to contribute some development effort to get this in place.

What can I do to help this PR get back on track?

Would you be open to me extending this functionality to more complex types?

Thanks,
Ian

@scyzoryck
Copy link
Collaborator Author

scyzoryck commented Jun 11, 2024

@goetas - could you take a look at it, please? :) If you agree with taking such approach for the union types, I will fix the phpstan issues.

@idbentley Thanks for bringing back this MR :) For no needed, I will try to make it work after approval for the approach from Goetas :)

@idbentley
Copy link
Contributor

@scyzoryck

I think adding these assertions would improve the tests as well:

diff --git a/tests/Serializer/BaseSerializationTestCase.php b/tests/Serializer/BaseSerializationTestCase.php
index 9ce0d5cd..8ddcd324 100644
--- a/tests/Serializer/BaseSerializationTestCase.php
+++ b/tests/Serializer/BaseSerializationTestCase.php
@@ -2009,6 +2009,9 @@ abstract class BaseSerializationTestCase extends TestCase
 
         $object = new UnionTypedDocBLockProperty(10000);
         self::assertEquals($object, $this->deserialize(static::getContent('data_integer'), UnionTypedDocBLockProperty::class));
+
+        $object = new UnionTypedDocBlockProperty('foo');
+        self::assertEquals($object, $this->deserialize(static::getContent('data_string'), UnionTypedDocBLockProperty::class));
     }
 
     public function testIterable(): void
diff --git a/tests/Serializer/JsonSerializationTest.php b/tests/Serializer/JsonSerializationTest.php
index 12cebabd..cbca7b5a 100644
--- a/tests/Serializer/JsonSerializationTest.php
+++ b/tests/Serializer/JsonSerializationTest.php
@@ -142,6 +142,7 @@ class JsonSerializationTest extends BaseSerializationTestCase
             $outputs['uninitialized_typed_props'] = '{"virtual_role":{},"id":1,"role":{},"tags":[]}';
             $outputs['custom_datetimeinterface'] = '{"custom":"2021-09-07"}';
             $outputs['data_integer'] = '{"data":10000}';
+            $outputs['data_string'] = '{"data":"foo"}';
             $outputs['uid'] = '"66b3177c-e03b-4a22-9dee-ddd7d37a04d5"';
             $outputs['object_with_enums'] = '{"ordinary":"Clubs","backed":"C","ordinary_array":["Clubs","Spades"],"backed_array":["C","H"],"ordinary_auto_detect":"Clubs","backed_auto_detect":"C","backed_int_auto_detect":3,"backed_int":3,"backed_name":"C","backed_int_forced_str":3}';
             $outputs['object_with_autodetect_enums'] = '{"ordinary_array_auto_detect":["Clubs","Spades"],"backed_array_auto_detect":["C","H"],"mixed_array_auto_detect":["Clubs","H"]}';
@@ -446,6 +447,8 @@ class JsonSerializationTest extends BaseSerializationTestCase
 
         $object = new UnionTypedProperties(10000);
         self::assertEquals($object, $this->deserialize(static::getContent('data_integer'), UnionTypedProperties::class));
+        $object = new UnionTypedProperties('foo');
+        self::assertEquals($object, $this->deserialize(static::getContent('data_string'), UnionTypedProperties::class));
     }

case 'string':
break;
default:
throw new RuntimeException();
Copy link
Collaborator

Choose a reason for hiding this comment

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

shouldn't here be a better message?

or be a map (static property in the class) with alternative names for types?

static $aliases = [
 'boolean' => 'bool',
 'integer' => 'int',
 //...
];

public static function getSubscribingMethods()
{
return [
[
Copy link
Collaborator

Choose a reason for hiding this comment

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

can this be shortened as in

public static function getSubscribingMethods()
?


private function matchSimpleType($data, array $type, Context $context)
{
$dataType = gettype($data);
Copy link
Collaborator

Choose a reason for hiding this comment

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

i guess this works for json only... in XML all the data are always strings... so I do not see the point of this handler for xml

@@ -423,6 +425,18 @@ public static function getTypeHintedArraysAndStdClass()
];
}

public function testDeserializingUnionProperties()
Copy link
Collaborator

Choose a reason for hiding this comment

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

this seems doing the test with primitives... what about union of objects? is that supported?

@idbentley
Copy link
Contributor

Hi @goetas please checkout #1546 where I have addressed your feedback and made some minor improvements.

I'd really like to get this merged, and then also add handling for Unions of Complex types.

@scyzoryck scyzoryck closed this Jul 9, 2024
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.

3 participants