liip / serializer

A PHP serializer that generates PHP code for maximum performance
MIT License
127 stars 10 forks source link

improvement(DateTime): handle multiple deserialization formats and timezones #39

Closed brutal-factories closed 9 months ago

brutal-factories commented 12 months ago

The deserialization generator does not handle timezones for PropertyDateTime, and will throw an exception if the metadata contains any. In addition, with the https://github.com/liip/metadata-parser/pull/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 commented 12 months ago

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

dbu commented 12 months ago

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

dbu commented 12 months ago

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

dbu commented 9 months ago

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 commented 9 months ago

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 :thinking: .

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 commented 9 months ago

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 commented 9 months ago

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

brutal-factories commented 9 months ago

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

:tada: 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 commented 9 months ago

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