improvement(DateTime): handle multiple deserialization formats and timezones

[brutal-factories]

The deserialization generator does not handle timezones for PropertyDateTime, and will throw an exception if the metadata contains any. In addition, with the liip/metadata-parser#44 PR , I've also implemented the handling of multiple deserialization formats.

Here's a small script I've put together for this feature, from the same PR, just with the timezones added

<?php

require_once 'vendor/autoload.php';

use Doctrine\Common\Collections\ArrayCollection;
use Doctrine\Common\Collections\Collection;

use Doctrine\Common\Annotations\AnnotationReader;
use JMS\Serializer\Annotation as Serializer;
use Liip\MetadataParser\Builder;
use Liip\MetadataParser\ModelParser\VisibilityAwarePropertyAccessGuesser;
use Liip\MetadataParser\Parser;
use Liip\MetadataParser\RecursionChecker;
use Liip\MetadataParser\ModelParser\JMSParser;
use Liip\MetadataParser\ModelParser\LiipMetadataAnnotationParser;
use Liip\MetadataParser\ModelParser\PhpDocParser;
use Liip\MetadataParser\ModelParser\ReflectionParser;
use Liip\MetadataParser\Reducer\TakeBestReducer;
use Liip\Serializer\Configuration\GeneratorConfiguration;


class Student {
    public ?string $name = null;

    /**
     * Simple case
     * @Serializer\Type("DateTimeImmutable<'Y-m-d'>")
     */
    public ?DateTimeImmutable $createdAt = null;
    /**
      * Type-hint doesn't match JMS type information (probably not the best practices, but works)
     * @Serializer\Type("DateTimeInterface<'Y-m-d'>")
     */
    public ?Datetime $updatedAt = null;
}

class Course
{
    public ?string $name = null;
    /**
     * @Serializer\Type("ArrayCollection<Student>")
     */
    public ?Collection $students = null;
     /**
      * Multiple deserialization formats
      * Type-hint doesn't exactly match JMS type information
      * Timezone explicitly specified
      * @Serializer\Type("DateTime<'Y-m-d\TH:i:sP', 'UTC', ['Y-m-d', 'Y-m-d\TH:i:sO']>")
      */
     public ?DateTimeInterface $startDate = null;
     /**
      * Multiple deserialization formats
      * Type-hint doesn't exactly match JMS type information
      * Timezone explicitly specified
      * @Serializer\Type("DateTimeImmutable<'Y-m-d\TH:i:sP', 'UTC', ['Y-m-d', 'Y-m-d\TH:i:sO']>")
      */
     public ?DateTimeInterface $endDate = null;

    public function __construct()
    {
        $this->students = new ArrayCollection();
    }
}

$parser = new Parser([
    new ReflectionParser(),
    new JMSParser(new AnnotationReader()),
    new PhpDocParser(),
    new LiipMetadataAnnotationParser(new AnnotationReader()),
]);

$builder = new Builder($parser, new RecursionChecker());
$classes = [Student::class, Course::class];
$configuration = GeneratorConfiguration::createFomArray([
    'classes' => array_fill_keys($classes, []),
]);

@mkdir($cacheDirectory = './cached/liip', 0777, true);
$deserializerGenerator = new \Liip\Serializer\DeserializerGenerator(new \Liip\Serializer\Template\Deserialization(), $classes, $cacheDirectory);
$deserializerGenerator->generate($builder);

$serializer = new \Liip\Serializer\Serializer($cacheDirectory);
var_dump($serializer->fromArray([
    'name' => 'Advanced php - II',
    'students' => [
        ['name' => 'Bob', 'created_at' => '2020-01-01', 'updated_at' => '2023-05-09'],
        ['name' => 'Alice', 'created_at' => '2021-02-12', 'updated_at' => '2023-05-09'],
    ],
    'startDate' => '2021-05-07T23:52:45+0000',
    'endDate' => '2023-06-15',
], Course::class));

P.S. : I'll be adding more tests later More tests have been added for those cases

[brutal-factories]

I seem to have trouble with both Rector and PhpCS on files I haven't edited here, am I missing something?

[dbu]

seems to be changes in cs fixer and rectorphp. i fixed the errors in #40

[dbu]

instead of checking for method existence, we can change composer.json to require ^1.2.0 of metadata-parser.

[brutal-factories]

Looking at this place, it does feel weird: JMS made the choice of making the serialization format the deserialization format as well by default, which is understandable. But the way liip/* is structured, it should be possible to accept datetimes from a variety of formats by default (new DateTime($data)), and output them in a single format. Do you think it would be too bad of a BC break to do that?

[dbu]

i think there would not be a BC break, because until now we would have errored if there were multiple formats, no? or i misunderstand, it would still be ok if we accept more valid but differently formatted dates than before, i don't see a problem there.

as long as we don't accidentally change the format when we serialize a date, i see no risks in adding more flexibility.

[dbu]

i would prefer to do that at the top of this method, immediately sanitize the $formats. additionally, we could trigger a deprecation warning when a string is passed instead of an array.

[brutal-factories]

trigger a deprecation? should that be done with doctrine/deprecations or with a simple trigger_error($message, E_USER_DEPRECATED) ?

[dbu]

i would do it the same as symfony does: https://github.com/symfony/symfony/blob/6c9107334da9e443a4f64f782a93182ac453bf78/src/Symfony/Component/ErrorHandler/DebugClassLoader.php#L337C18-L337C18

@trigger_error('...', \E_USER_DEPRECATED);

[dbu]

thanks a lot for this work! i commented a couple of questions.

[dbu]

great! can you please rebase on the 2.x branch so that cs and rector errors not related to your changes go away? then we see if all is fine with the changes.

and can you please add an entry to the CHANGELOG.md file that explains the new functionality? we use this for the release notes on github, and users can read the changelog to discover new features.

[brutal-factories]

great! can you please rebase on the 2.x branch so that cs and rector errors not related to your changes go away? then we see if all is fine with the changes.

and can you please add an entry to the CHANGELOG.md file that explains the new functionality? we use this for the release notes on github, and users can read the changelog to discover new features.

Changelog is updated

I remember indeed rebasing on the 2.x branch when I picked back up this PR, due to Rector/CSFixer issues, but the issues persisted. I just tried re-pulling the branch just to make sure, but the git history still says my branch is indeed based on the tip of the current 2.x branch.

One thing to note is, for src/Context.php which CSFixer picks up, its latest commit does contain the formatting issues CSFixer wants to change, and the github web interface still shows the commit with failed CI as the latest change to the file, even though the branch history says otherwise 🤔 .

edit: could it be that I have a different php-cs-fixer configuration? I don't see where it was configured to place empty braces pairs on the same line in my php-cs-fixer.dist.php

[dbu]

the thing is that we do not lock a specific cs fixer version, and the latest minor version had some new rules. i am updating in #43

[dbu]

great, thanks a lot for this contribution! and thanks for the patience to get this mergeable.

[dbu]

https://github.com/liip/serializer/releases/tag/2.6.0

[brutal-factories]

https://github.com/liip/serializer/releases/tag/2.6.0

🎉
On that note, could you also make a new tag for liip/metadata-parser ? It currently only has a branch, and I was thinking of making a PR to liip/serializer to make it compatible with liip/metadata-parser:^2.0

[dbu]

there are a couple things in metadata-parser that should be done before releasing. i tried to be minimal to only have the things that need BC breaks, but supporting doctrine attributes instead of annotations would be quite useful for the future too: https://github.com/liip/metadata-parser/issues