fix(test): replace Collection|iterable with Collection and add appropriate PHPDoc tags

[vinceAmstoutz]

Q	A
Branch?	main
Tickets	-
License	MIT

Fixes several phpstan errors ignored in the config

[soyuka]

ci is all red probably due to this change

[vinceAmstoutz]

ci is all red probably due to this change

@VincentLanglet @soyuka At some points both Collection and iterable types are used from different places for the same property, so I found a solution to prevent typing error from PHPStan, here an example.
WDYT?

Otherwise I have this strange failure in Behat checks but I only edited fixtures files (entities & documents), any idea?

[VincentLanglet]

@VincentLanglet @soyuka At some points both Collection and iterable types are used from different places for the same property, so I found a solution to prevent typing error from PHPStan, here an example.

WDYT?

I feel like it's really hacky and kinda a code smell to have an "addFoo" method which does nothing when the property is an iterable.

Ok it's "only for tests files" but it could still lead to some weird error in the futur, and adding such hack just to fix a phpstan baseline error might not be worth it.

Otherwise I have this strange failure in Behat checks but I only edited fixtures files (entities & documents), any idea?

It could be more insteresting to use the first fix you use (Collection everywhere) and understand the behat failure. I can only take a look starting next week since i'm currently one vacation without a computer.

[vinceAmstoutz]

@VincentLanglet @soyuka At some points both Collection and iterable types are used from different places for the same property, so I found a solution to prevent typing error from PHPStan, here an example.

WDYT?

I feel like it's really hacky and kinda a code smell to have an "addFoo" method which does nothing when the property is an iterable.

Ok it's "only for tests files" but it could still lead to some weird error in the futur, and adding such hack just to fix a phpstan baseline error might not be worth it.

Otherwise I have this strange failure in Behat checks but I only edited fixtures files (entities & documents), any idea?

It could be more insteresting to use the first fix you use (Collection everywhere) and understand the behat failure. I can only take a look starting next week since i'm currently one vacation without a computer.

Ok thanks @VincentLanglet , I will revert to Collection everywhere, but probably we'll need to adapt some parts which uses iterables at some parts.

[VincentLanglet]

Ok thanks @VincentLanglet , I will revert to Collection everywhere, but probably we'll need to adapt some parts which uses iterables at some parts.

Sure, ping me when it's done ; I'll try to help

[vinceAmstoutz]

Ok thanks @VincentLanglet , I will revert to Collection everywhere, but probably we'll need to adapt some parts which uses iterables at some parts.

Sure, ping me when it's done ; I'll try to help

Yes, thanks, I will tomorrow

[vinceAmstoutz]

@VincentLanglet it's ready and PHPStan is green

[VincentLanglet]

@vinceAmstoutz with such commit it might be easier
6e15dc7

You get logs like

{"@context":"\/contexts\/Error","@id":"\/errors\/400","@type":"hydra:Error","detail":"Failed to denormalize attribute \u0022relatedDummies\u0022 value for class \u0022ApiPlatform\\Tests\\Fixtures\\TestBundle\\Document\\Dummy\u0022: Expected argument of type \u0022Doctrine\\Common\\Collections\\Collection\u0022, \u0022array\u0022 given at property path \u0022relatedDummies\u0022.","status":400,"type":"\/errors\/400","trace":[{"file":"\/home\/runner\/work\/core\/core\/vendor\/symfony\/serializer\/Normalizer\/AbstractObjectNormalizer.php","line":400,"function":"createForUnexpectedDataType","class":"Symfony\\Component\\Serializer\\Exception\\NotNormalizableValueException","type":"::"},{"file":"\/home\/runner\/work\/core\/core\/src\/Serializer\/AbstractItemNormalizer.php","line":260,"function":"denormalize","class":"Symfony\\Component\\Serializer\\Normalizer\\AbstractObjectNormalizer","type":"-\u003E"},{"file":"\/home\/runner\/work\/core\/core\/src\/Serializer\/ItemNormalizer.php","line"...

but I didn't fully solve this yet...

maybe you'll get a better understanding than me ?

[vinceAmstoutz]

@vinceAmstoutz with such commit it might be easier 6e15dc7

You get logs like

{"@context":"\/contexts\/Error","@id":"\/errors\/400","@type":"hydra:Error","detail":"Failed to denormalize attribute \u0022relatedDummies\u0022 value for class \u0022ApiPlatform\\Tests\\Fixtures\\TestBundle\\Document\\Dummy\u0022: Expected argument of type \u0022Doctrine\\Common\\Collections\\Collection\u0022, \u0022array\u0022 given at property path \u0022relatedDummies\u0022.","status":400,"type":"\/errors\/400","trace":[{"file":"\/home\/runner\/work\/core\/core\/vendor\/symfony\/serializer\/Normalizer\/AbstractObjectNormalizer.php","line":400,"function":"createForUnexpectedDataType","class":"Symfony\\Component\\Serializer\\Exception\\NotNormalizableValueException","type":"::"},{"file":"\/home\/runner\/work\/core\/core\/src\/Serializer\/AbstractItemNormalizer.php","line":260,"function":"denormalize","class":"Symfony\\Component\\Serializer\\Normalizer\\AbstractObjectNormalizer","type":"-\u003E"},{"file":"\/home\/runner\/work\/core\/core\/src\/Serializer\/ItemNormalizer.php","line"...

but I didn't fully solve this yet...

maybe you'll get a better understanding than me ?

@VincentLanglet Thanks for your help!
Yes, that's because in some tests, for example here, we using an IRI array instead of a collection. That's why it works with the array type.
In other places, we using the ->add() method and a collection, which is why I think we inherit from a Collection|iterable.

What do you think, should we modify the tests to better comply with code quality rules and so separate usage from Collection and iterable using various independent fixtures?

[VincentLanglet]

@VincentLanglet Thanks for your help! Yes, that's because in some tests, for example here, we using an IRI array instead of a collection. That's why it works with the array type. In other places, we using the ->add() method and a collection, which is why I think we inherit from a Collection|iterable.

What I don't understand is why the conversion is not done from array to Collection here but when I use something like

$response = $client->request('POST', '/invitations', [
            'json' => [
                'email' => '[email protected]',
                'workspace' => '/workspaces/'.AppFixtures::DEFAULT_WORKSPACE_UUID,
                'projectInvitations' => [
                    [
                        'projectUuid' => '9f8b4f32-e845-4c0b-9a5d-a8df2e3b7c12',
                        'role' => Role::EDITOR->value,
                        'restrictedLanguages' => ['en'],
                    ],
                ],
            ],
        ]);

in my tests, projectInvitations are correctly transformed to a Collection of ProjectInvitation...

Maybe it's because it lacks of methods and we should have both get/set/add/remove methods ?

What do you think, should we modify the tests to better comply with code quality rules and so separate usage from Collection and iterable using various independent fixtures?

If we don't find how to always work with a Collection, maybe we could modify the few fixtures where array is used to something like

public Collection|array $property

public function getProperty(): Collection
{
    if (\is_array($this->property) {
         $this->property = new ArrayCollection($this->property);
    }
    
    return $this->property;
}

public function addProperty($foo): void
{
    $this->getProperty()->add($foo);
}