Better merging of collection types

[brutal-factories]

When a model has a collection type-hinted with Collection, and the JMS serializer annotations has the actual information about the collections contents (@Serializer\Type("ArrayCollection<...>")), the model parsing fails with the error Can't merge type Liip\MetadataParser\Metadata\PropertyTypeClass with Liip\MetadataParser\Metadata\PropertyTypeArray, they must be the same or unknown.

This happens when the ReflectionParser passes before the JMSParser, due to the two property type metadatas PropertyTypeClass<Collection> and PropertyTypeArray<ArrayCollection<...>> not being compatible. Using JMS alone, this does work, and logically speaking, the PropertyTypeArray in the example can be understood to be a specialisation of the PropertyTypeClass.

This Pull Request proposes a specialized case to merge Collections from PropertyTypeClass into PropertyTypeArrays that are Collections too, using their most derived common base class (hopefully, both sides have the same collection type).

Here is a test script to explain my idea

<?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\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;

    /**
     * @Serializer\Type("DateTimeImmutable<'Y-m-d'>")
     */
    public ?DateTimeImmutable $createdAt = null;
    /**
     * @Serializer\Type("DateTime<'Y-m-d'>")
     */
    public ?DateTime $updatedAt = null;
}

class Course
{
    public ?string $name = null;
    /**
     * @Serializer\Type("ArrayCollection<Student>")
     */
    public ?Collection $students = 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());
$metadata = $builder->build(IndirectAccess::class, [new TakeBestReducer()]);
$classes = [Student::class, Course::class];
$configuration = GeneratorConfiguration::createFomArray([
    'classes' => array_fill_keys($classes, []),
]);
@mkdir('./cached');
@mkdir('./cached/liip');
$cacheDirectory = 'cached/liip';
$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'],
    ]
], Course::class));

P.S.: I'll be adding proper auomated tests shortly Tests have been added to cover as many cases as I could think of.

[dbu]

thanks for this improvement. it makes a lot of sense to me.

i realize that the PropertyTypeArray is a bit of a misnomer, we should have called it PropertyTypeIterable to go with PHP iterable type... would that make sense to you?

as we do a BC break anyways, we could maybe also cleanup the name.

can you please add some tests for this? and a changelog entry?

i created the 2.x branch. can you please target that version.

[dbu]

thanks a lot! i think this is about ready. i suggest to be forward compatible with our version 2, at least for the new PropertyTypeIterable.

please also add a note to the changelog. i think in the readme doc there is no explanation needed.

i created the 2.x milestone, please add to it.

[dbu]

we should make this class final. if inverse what extends what we can do that, and have the deprecated class no longer be technically final.

[brutal-factories]

It seems that force-pushing on my branch closed down my PR, I'll soon reopen it when my git directory is in order

[dbu]

it looks like what you did recreated every single commit since the very fist commit :-O

i think what i'd do is to create a fresh branch from 1.x and then cherry-pick the relevant commits.

git checkout 1.x
git pull
git checkout -b improvement/collection-types-take-2
git cherry-pick 57214bfe9f2d741ec5ae512df9fecfb50eb2d375
... and so on from your commits as per https://github.com/liip/metadata-parser/pull/40/commits (starting from old to new in order, otherwise you get conflicts)
git push origin HEAD -u

and create a new merge request.

[brutal-factories]

This is reopened in #42