search

laminas-api-tools / api-tools-doctrine-querybuilder

Laminas API Tools Doctrine QueryBuilder module
https://api-tools.getlaminas.org/documentation
BSD 3-Clause "New" or "Revised" License
9 stars 17 forks source link

CollectionLinkHydrator should extend from different AbstractCollectionStrategy #14

Open zluiten opened 3 years ago

zluiten commented 3 years ago

Fixes #13.

Class Laminas\ApiTools\Doctrine\QueryBuilder\Hydrator\Strategy\CollectionLinkHydratorV3 should extend class AbstractCollectionStrategy of doctrine/doctrine-laminas-hydrator when DoctrineModule >v3 is installed instead of the then removed AbstractCollectionStrategy of DoctrineModule v2.

zluiten commented 3 years ago

Can someone have a look this please?

froschdesign commented 3 years ago

@netiul

…when DoctrineModule >v3 is installed instead of the then removed AbstractCollectionStrategy of DoctrineModule v2.

Your pull request does not fix the problem at all because this package still allow the installation of DoctrineModule with version 2.

https://github.com/laminas-api-tools/api-tools-doctrine-querybuilder/blob/002038f798e54fd2a4b9dd18fbe7c9958688827d/composer.json#L33

https://github.com/doctrine/DoctrineModule/blob/c8ce08e9fa67de928bad306b3fe798fb6683bfea/src/DoctrineModule/Stdlib/Hydrator/Strategy/AbstractCollectionStrategy.php#L17

zluiten commented 3 years ago

@froschdesign

This package support both v2 and v3 of DoctrineModule.

The class DoctrineModule\Stdlib\Hydrator\Strategy\AbstractCollectionStrategy you are linking to only exists in v2. V3 of DoctrineModule uses the Laminas\Hydrator package. To support both there is a V2 and V3 version of the CollectionLinkHydrator. In src/_autoload.php the appropiate version is aliased to Hydrator\Strategy\CollectionLink based on the existence of the Laminas\Hydrator\HydratorPluginManagerInterface (which is only the case when v3 of DoctrineModule is installed).

But here is the problem, v3 of the CollectionLinkHydrator still extends the AbstractCollectonStrategy from DoctrineModule v2! This PR fixes that.

froschdesign commented 3 years ago

@netiul

To support both there is a V2 and V3 in src/_autoload.php the appropiate versions is aliased to Hydrator\Strategy\CollectionLink based on the existence of the Laminas\Hydrator\HydratorPluginManagerInterface (which is only the case when v3 of DoctrineModule is installed).

Thanks, I missed this hint in the bug report and in the pull request description.

froschdesign commented 3 years ago

@netiul Can you also check the related test class because name looks strange:

https://github.com/laminas-api-tools/api-tools-doctrine-querybuilder/blob/002038f798e54fd2a4b9dd18fbe7c9958688827d/test/Hydrator/Strategy/CollectionLinkTest.php#L11

Thanks in advance! 👍

zluiten commented 3 years ago

@froschdesign No problem! I guess I could have been more elaborative initially.

Can you also check the related test class because name looks strange:

https://github.com/laminas-api-tools/api-tools-doctrine-querybuilder/blob/002038f798e54fd2a4b9dd18fbe7c9958688827d/test/Hydrator/Strategy/CollectionLinkTest.php#L11

Thanks in advance! +1

That looks strange indeed. I can fix the casing in the test but I see now that the word Collectionlink in Hydrator\Strategy\Collectionlink in _autoload.php is aliased with lowercased letter l of link. That might be a breaking change though. PHP's namespacing is case insensitive, but I think composer's autoloading is not. Can you advise on that @froschdesign ?