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

DateTime - Deserialize from more formats (default) #1559

Merged
merged 3 commits into from
Oct 16, 2024

Conversation

iKsSs
Copy link

@iKsSs iKsSs commented Sep 24, 2024

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

This change allows the setting of multiple default deserialization formats of Date.

@goetas
Copy link
Collaborator

goetas commented Sep 25, 2024

Does this addition need to be documented?

@iKsSs
Copy link
Author

iKsSs commented Sep 26, 2024

Does this addition need to be documented?

I can put it somewhere but please tell me where.
Certainly, I will add this to JMSSerializerBundle config - there is a place for it

@iKsSs
Copy link
Author

iKsSs commented Oct 10, 2024

Who can take it further?

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.

@iKsSs In general - great work! It looks like we do not have a documentation for the Handlers - So creating docs in bundle sounds ok for me.
I left 2 small comments - can you take a look at them please? :)

Best, Marcin

Comment on lines 83 to 87
bool $xmlCData = true,
array $defaultDeserializationFormats = [\DateTime::ATOM]
) {
$this->defaultFormat = $defaultFormat;
$this->defaultDeserializationFormats = $defaultDeserializationFormats;
Copy link
Collaborator

Choose a reason for hiding this comment

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

If I understand correctly in the past $defaultFormat was working for both - serialisation & deserialisation. With the changes that you proposed I can see small BC break - when somebody was using $defaultFormat with different format after updating the package, they will get DateTime::ATOM as a deserialisation format what might be small BC. I guess something like might help to keep it consistent with old behaviour when no defaultDeserializationFormats are passed.

Suggested change
bool $xmlCData = true,
array $defaultDeserializationFormats = [\DateTime::ATOM]
) {
$this->defaultFormat = $defaultFormat;
$this->defaultDeserializationFormats = $defaultDeserializationFormats;
bool $xmlCData = true,
array $defaultDeserializationFormats = []
) {
$this->defaultFormat = $defaultFormat;
$this->defaultDeserializationFormats = $defaultDeserializationFormats === [] ? [$defaultFormat] : $defaultDeserializationFormats;

Copy link
Author

@iKsSs iKsSs Oct 12, 2024

Choose a reason for hiding this comment

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

Ou, you are very right. In my case, it would be BC. Thanks for providing me with a solution to this problem.
I was thinking about renaming defaultFormat private property to defaultSerializationFormat, what do you think?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Renaming sounds like a good solution 👍

Copy link
Author

Choose a reason for hiding this comment

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

OK, done

@@ -285,10 +298,14 @@ private function getDeserializationFormats(array $type): array
return is_array($type['params'][2]) ? $type['params'][2] : [$type['params'][2]];
}

return [$this->getFormat($type)];
if (isset($type['params'][0])) {
return is_array($type['params'][0]) ? $type['params'][0] : [$type['params'][0]];
Copy link
Collaborator

Choose a reason for hiding this comment

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

Looking at the old code:

    private function getFormat(array $type): string
    {
        return $type['params'][0] ?? $this->defaultFormat;
    }

$type['params'][0] will be always string.

Copy link
Author

Choose a reason for hiding this comment

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

Yeah, that's true => I removed redundant code, thanks

@iKsSs iKsSs requested a review from scyzoryck October 14, 2024 05:24
@scyzoryck scyzoryck merged commit 43404be into schmittjoh:master Oct 16, 2024
17 of 18 checks passed
@scyzoryck
Copy link
Collaborator

I will release it on Monday! Thanks for the contribution!

@iKsSs
Copy link
Author

iKsSs commented Oct 16, 2024

OK, you are welcome, thanks for your cooperation

@iKsSs
Copy link
Author

iKsSs commented Oct 22, 2024

I will release it on Monday! Thanks for the contribution!

@scyzoryck What Monday? 😄

@scyzoryck
Copy link
Collaborator

@iKsSs Sorry, I got some urgent stuff last week. Released today! 🎉

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.

4 participants