Handle array format for dateHandler #1108

VincentLanglet · 2019-07-14T23:14:15Z

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

goetas · 2019-07-28T06:16:05Z

Hi!
First of all, thanks for your contribution! I apologize, but somehow missed this PR.

Can you add some tests for this? I'm really not able to understand how this works...

VincentLanglet · 2019-07-28T09:30:33Z

I made a mistake. All is good now and I added tests.
Does this helps you ?

params can still be ['Y-m-d'] but now can be [['Y-m-d'], ['Y/m/d'], ...].
In this case we try to serialize with the format 'Y-m-d', then, in case of failure, 'Y/m/d', and so on.

goetas · 2019-07-28T14:30:05Z

I think here is missing the update on the type parser. How to provide this new type definition via @Type annotation as example?

VincentLanglet · 2019-07-28T16:17:11Z

@goetas I wanted to be used this way

@JMS\Type("DateTime<'Y-m-d\TH:i:s'>")
@JMS\Type("DateTime<null, null, 'Y-m-d\TH:i:s'>")
@JMS\Type("DateTime<['Y-m-d\TH:i:s', 'Y-m-d\TH:i:sP']>")
@JMS\Type("DateTime<null, null, ['Y-m-d\TH:i:s', 'Y-m-d\TH:i:sP']>")

Are you saying ['Y-m-d\TH:i:s', 'Y-m-d\TH:i:sP'] is not actually parsed as a php array ?

goetas · 2019-07-28T16:47:45Z

Are you saying ['Y-m-d\TH:i:s', 'Y-m-d\TH:i:sP'] is not actually parsed as a php array ?

Exactly. I think this has to be implemented.

VincentLanglet · 2019-07-28T17:40:06Z

Exactly. I think this has to be implemented.

Do you know how it works and what is needed to be added/changed.

I see in both Xml and Json deserialisation visitor the function.

public function visitArray($data, array $type): array

So it seems like it's already implemented.
If it's not, I really don't know if I'll be able to add this feature...

goetas · 2019-07-28T19:57:37Z

In https://github.com/schmittjoh/serializer/tree/master/src/Type you have the classes responsible for the type parsing. They convert the type definition syntax in the array of parameters passed to the visitors.

The type parsing is implemented using the hoa lexer and parser (see https://github.com/hoaproject/Compiler)

VincentLanglet · 2019-07-28T22:34:33Z

In https://github.com/schmittjoh/serializer/tree/master/src/Type you have the classes responsible for the type parsing. They convert the type definition syntax in the array of parameters passed to the visitors.

Yes, the type visitor call visitCompoundType which returns

[
    'name' => $nameToken->getValueValue(),
    'params' => array_map(
        function (TreeNode $node) use ($handle, $eldnah) {
            return $node->accept($this, $handle, $eldnah);
        },
        $parameters
    ),
];

When you look at the accept function of the hoa project, it calls the visit of the Visitor.
And in the DeserializationVisitorInterface of you project, there is already a function visitArray. So it's seems already implemented.

Can you check again ?
Or at least give a failing test with array params I need to fix ?

VincentLanglet · 2019-08-11T20:56:00Z

@goetas You maybe missed my previous message

goetas · 2019-08-11T20:59:28Z

So it's seems already implemented.

Most probably is not. You can see here the tests for it https://github.com/schmittjoh/serializer/blob/master/tests/Serializer/Type/ParserTest.php#L36

You can try to add here strings following your syntax and check if they pass.

VincentLanglet · 2019-08-11T21:06:57Z

It seems good to me this way 0ff06b7

It's the same syntax than

'array<Foo\Bar, Baz\Boo>'

goetas · 2019-08-12T04:47:55Z

Yeah, but the foundation of your proposal is based on this syntax: DateTime<null, null, ['Y-m-d\TH:i:s', 'Y-m-d\TH:i:sP']>, that is not tested and most probably not working.

Can you test this?

VincentLanglet · 2019-08-12T07:07:36Z

Yeah, but the foundation of your proposal is based on this syntax: DateTime<null, null, ['Y-m-d\TH:i:s', 'Y-m-d\TH:i:sP']>, that is not tested and most probably not working.

Can you test this?

Damned ! I forgot what I wanted to test...
I fixed it : 72f15d1

Still working

VincentLanglet · 2019-08-18T12:38:47Z

@goetas I'm sorry, how should I understand your emoji ?
Why are you confused ?

goetas · 2019-08-18T14:27:59Z

I'm confused because the grammar file has no instructions on how to handle []... so will have to check it on my own when will have some time

VincentLanglet · 2019-08-18T15:12:00Z

I'm confused because the grammar file has no instructions on how to handle []... so will have to check it on my own when will have some time

I understand. Indeed, [] is surely not handled.
But no need to handle [] if you handle array<>.

DateTime<null, null, array<'Y-m-d\TH:i:s', 'Y-m-d\TH:i:sP'>> is good enough.

VincentLanglet · 2019-09-17T12:44:16Z

Hi @goetas, do you have time to check it on your own ?

goetas · 2019-09-17T12:54:20Z

I had a look, and as said in #1108 (comment)
DateTime<null, null, ['Y-m-d\TH:i:s', 'Y-m-d\TH:i:sP']> is not handled.

DateTime<null, null, array<'Y-m-d\TH:i:s', 'Y-m-d\TH:i:sP'>>, with array has a different semantic meaning.

@Majkl578 Could you help us to have the parser supporting [] array definitions?

goetas · 2019-09-17T12:57:13Z

Ideally the tests case to pass should be

        yield [
            'DateTime<null, null, [\'Y-m-d\TH:i:s\', \'Y-m-d\TH:i:sP\']>',
            $type('DateTime', [null, null, ['Y-m-d\TH:i:s', 'Y-m-d\TH:i:sP']]),
        ];

VincentLanglet · 2019-09-17T13:04:00Z

DateTime<null, null, array<'Y-m-d\TH:i:s', 'Y-m-d\TH:i:sP'>>, with array has a different semantic meaning.

What are the semantic meaning differences between array and [] ? This is surprising me

goetas · 2019-09-17T13:05:07Z

array is a type. used for the handlers. while [] are just parameters for the current type.

Majkl578 · 2019-09-19T01:26:11Z

@goetas Is this what you had in mind? #1125

goetas · 2019-09-19T06:48:52Z

@VincentLanglet If you look at #1125 , @Majkl578 did exactly what you need!

VincentLanglet · 2019-09-19T08:03:34Z

Great ! Thanks @Majkl578

VincentLanglet · 2019-11-03T22:01:43Z

HI @goetas What are the step now ?

Merging Accept arrays as type attribute values #1125
Merging this PR

Am I missing something ?

goetas · 2019-11-08T15:26:52Z

tests/Serializer/Type/ParserTest.php

@@ -79,6 +79,10 @@ public function validTypesProvider(): iterable
            'array<Foo\Bar, Baz\Boo>',
            $type('array', [['name' => 'Foo\Bar', 'params' => []], ['name' => 'Baz\Boo', 'params' => []]]),
        ];
+        yield [
+            'DateTime<null, null, array<\'Y-m-d\TH:i:s\', \'Y-m-d\TH:i:sP\'>>',
+            $type('DateTime', [null, null, ['name' => 'array', 'params' => ['Y-m-d\TH:i:s', 'Y-m-d\TH:i:sP']]]),


They should be tested together to make sure it works as expected.

Your current branch is not compatible with #1125.
If you checkout #1125 and and test it with your code, it will fail.

DateTime<null, null, ['Y-m-d\TH:i:s', 'Y-m-d\TH:i:sP'], will produce an array format that your getDeserializationFormatsAndTimeZones will not be able to understand.

Wouldn't be easier if the #1125 branch was merge into master ?

VincentLanglet · 2019-12-09T23:38:12Z

@goetas Rebased, Fixed, And tested with #1125

And I tried to update the doc

VincentLanglet · 2020-01-08T09:39:15Z

Hi @goetas , happy new year.
I think everything is ready in this PR. Do you have some time to look at ?

goetas · 2020-01-12T08:22:38Z

doc/reference/annotations.rst

@@ -463,6 +466,11 @@ Examples:
         */
        private $endAt;

+        /**
+         * @Type("DateTime<['Y-m-d', 'Y/m/d']>")


This is not correct. Serialization does not accept more than one format. The feature you are building makes sense only for de-serialization.
I think we should throw some error if this is used for serialization.

DateTime<'formats'>

is a shortcut for

DateTime<'formats', null, 'formats'>

that's why I allowed the syntax.

But for serialization I used the first valid format.

If I only allow the third parameter to be an array, it means that someone who only care bout de-serialization, will write

@Type("DateTime<'Y-m-d'>")

To deserialize only one format, and

@Type("DateTime<'Foo', 'Bar', ['Y-m-d', 'Y/m/d']>")

To handle multiple syntax. I found this a little sad...

I can understand that, but DateTime<['Y-m-d', 'Y/m/d']> is wrong since the serialization does not accept multiple formats. it will always pick the first.

But to be hones, thinking it twice, I start to like this feature less and less.

I had a similar problem in the past and i solved it with a custom date handler (see https://github.com/goetas-webservices/xsd2php-runtime/blob/master/src/Jms/Handler/XmlSchemaDateHandler.php#L86)

I can only allow the syntax DateTime<'Foo', 'Bar', ['Y-m-d', 'Y/m/d']> if preferred.

But to be hones, thinking it twice, I start to like this feature less and less.

It would be nice to make your choice, I took time working on it and I prefer to know now if I taking more time on it will be useless.

I had a similar problem in the past and i solved it with a custom date handler

I feel like, it's really hard to write a custom date handler for someone starting with the library.
When only using the annotation (like we do in our company), it's so much easier to use an array that writing a whole custom dateHandler.

goetas · 2020-01-12T08:29:26Z

See #1108 (comment) and I think an integration test would be a good thing to do, something like

serializer/tests/Serializer/BaseSerializationTest.php

Line 675 in 1eeb406

public function testDateTimeImmutable($key, $value, $type)

Providing two date formats, the first should fail, and the second should succeed.

VincentLanglet · 2020-01-19T21:45:39Z

@goetas

@Type("DateTime<['Y-m-d', 'Y/m/d']>")

Is now not supported

And there is an integration test

goetas · 2020-01-24T05:23:38Z

thank you @VincentLanglet for your work and your patience!

…te formats

…ormats are supported here as well

VincentLanglet mentioned this pull request Jul 14, 2019

Supporting multiple format with (Datetime or others) handlers schmittjoh/JMSSerializerBundle#758

Closed

VincentLanglet marked this pull request as ready for review July 27, 2019 11:44

goetas added enhancement Missing Tests labels Jul 28, 2019

goetas reviewed Nov 8, 2019

View reviewed changes

VincentLanglet and others added 4 commits December 10, 2019 00:00

Handle array format for dateHandler

c5e3986

Add tests for array of DateTime

76526d1

Accept arrays as type attribute values

7d81f6a

Update with new syntax

8f17c4e

Update doc

0aefabc

VincentLanglet requested a review from goetas December 10, 2019 00:21

goetas reviewed Jan 12, 2020

View reviewed changes

Allow feature only for deserialization

a2c5a59

goetas removed the Missing Tests label Jan 24, 2020

goetas merged commit 5286388 into schmittjoh:master Jan 24, 2020

VincentLanglet deleted the handleArray branch January 27, 2020 13:41

mgrundkoetter added a commit to ujamii/openimmo that referenced this pull request Feb 4, 2020

[FEATURE] with schmittjoh/serializer#1108, we now support multiple da…

7646dc8

…te formats

goetas mentioned this pull request Mar 7, 2020

Accept arrays as type attribute values #1125

Closed

mgrundkoetter added a commit to ujamii/openimmo that referenced this pull request Mar 10, 2020

[FEATURE] with schmittjoh/serializer#1108 merged now, multiple date f…

cc88162

…ormats are supported here as well

Handle array format for dateHandler #1108

Handle array format for dateHandler #1108

Conversation

VincentLanglet commented Jul 14, 2019

goetas commented Jul 28, 2019 • edited Loading

VincentLanglet commented Jul 28, 2019

goetas commented Jul 28, 2019

VincentLanglet commented Jul 28, 2019

goetas commented Jul 28, 2019

VincentLanglet commented Jul 28, 2019 • edited Loading

goetas commented Jul 28, 2019

VincentLanglet commented Jul 28, 2019

VincentLanglet commented Aug 11, 2019

goetas commented Aug 11, 2019

VincentLanglet commented Aug 11, 2019 • edited Loading

goetas commented Aug 12, 2019 • edited Loading

VincentLanglet commented Aug 12, 2019 • edited Loading

VincentLanglet commented Aug 18, 2019

goetas commented Aug 18, 2019

VincentLanglet commented Aug 18, 2019

VincentLanglet commented Sep 17, 2019

goetas commented Sep 17, 2019

goetas commented Sep 17, 2019

VincentLanglet commented Sep 17, 2019

goetas commented Sep 17, 2019

Majkl578 commented Sep 19, 2019

goetas commented Sep 19, 2019

VincentLanglet commented Sep 19, 2019

VincentLanglet commented Nov 3, 2019

goetas Nov 8, 2019

Choose a reason for hiding this comment

VincentLanglet Nov 11, 2019

Choose a reason for hiding this comment

VincentLanglet commented Dec 9, 2019 • edited Loading

VincentLanglet commented Jan 8, 2020

goetas Jan 12, 2020 • edited Loading

Choose a reason for hiding this comment

VincentLanglet Jan 13, 2020

Choose a reason for hiding this comment

goetas Jan 13, 2020

Choose a reason for hiding this comment

VincentLanglet Jan 14, 2020

Choose a reason for hiding this comment

goetas commented Jan 12, 2020 • edited Loading

VincentLanglet commented Jan 19, 2020

goetas commented Jan 24, 2020

goetas commented Jul 28, 2019 •

edited

Loading

VincentLanglet commented Jul 28, 2019 •

edited

Loading

VincentLanglet commented Aug 11, 2019 •

edited

Loading

goetas commented Aug 12, 2019 •

edited

Loading

VincentLanglet commented Aug 12, 2019 •

edited

Loading

VincentLanglet commented Dec 9, 2019 •

edited

Loading

goetas Jan 12, 2020 •

edited

Loading

goetas commented Jan 12, 2020 •

edited

Loading