Skip to content

CollectionLinkHydrator should extend from different AbstractCollectionStrategy#14

Open
zluiten wants to merge 2 commits into
laminas-api-tools:1.9.xfrom
zluiten:fix/extend-base-class
Open

CollectionLinkHydrator should extend from different AbstractCollectionStrategy#14
zluiten wants to merge 2 commits into
laminas-api-tools:1.9.xfrom
zluiten:fix/extend-base-class

Conversation

@zluiten

@zluiten zluiten commented Jan 15, 2021

Copy link
Copy Markdown
Contributor

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.

…gy base class.

Signed-off-by: Zacharias Luiten <zach@xzachly.com>
@zluiten zluiten changed the title Should extend from doctrine-laminas-hydrator AbstractCollectionStrate… CollectionLinkHydrator should extend from different AbstractCollectionStrategy Jan 15, 2021
@zluiten

zluiten commented Aug 16, 2021

Copy link
Copy Markdown
Contributor Author

Can someone have a look this please?

@froschdesign

froschdesign commented Aug 16, 2021

Copy link
Copy Markdown
Member

@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.

"doctrine/doctrine-module": "^2.1.8 || ^3.0.1 || ^4.1.0",

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

@zluiten

zluiten commented Aug 16, 2021

Copy link
Copy Markdown
Contributor Author

@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

Copy link
Copy Markdown
Member

@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

Copy link
Copy Markdown
Member

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

use laminas\apitools\doctrine\querybuilder\hydrator\strategy\collectionlink;

Thanks in advance! 👍

@zluiten

zluiten commented Aug 16, 2021

Copy link
Copy Markdown
Contributor Author

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

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

use laminas\apitools\doctrine\querybuilder\hydrator\strategy\collectionlink;

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 ?

Signed-off-by: Zacharias Luiten <zach@xzachly.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

CollectionLinkHydrator extends wrong base class when DoctrineModule > v3 is used

2 participants