Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ReferenceRepository::getIdentifier() return type not compatible with all supported object managers #504

Closed
mbabker opened this issue Dec 2, 2024 · 4 comments

Comments

@mbabker
Copy link
Contributor

mbabker commented Dec 2, 2024

Bug Report

Q A
Version 2.0.0
Previous Version if the bug is a regression 1.x

Summary

In 2.0, the ReferenceRepository::getIdentifier() method has an array return type (compared to 1.x where this was in the doc block only), however, this return type is not compatible with the returns from the MongoDB ODM's UnitOfWork::getDocumentIdentifier() (mixed) or the PHPCR ODM's UnitOfWork::getDocumentId() (string or null).

Current behavior

The ReferenceRepository::getIdentifier() method only works correctly when an object is registered to any unit of work or can fetch the identifier from the ORM's unit of work.

Expected behavior

The ReferenceRepository::getIdentifier() method should work regardless of the object manager and its unit of work.

How to reproduce

I haven't yet taken the time to extract this out to a standalone test case, but working on liip/LiipTestFixturesBundle#326 the CI fails in that PR point to this type error:

Config Mongodb (Liip\Acme\Tests\Test\ConfigMongodb)
 ✘ Load fixtures mongodb
   │
   │ TypeError: Doctrine\Common\DataFixtures\ReferenceRepository::getIdentifier(): Return value must be of type array, string returned
   │
   │ /app/vendor/doctrine/data-fixtures/src/ReferenceRepository.php:77
   │ /app/vendor/doctrine/data-fixtures/src/ReferenceRepository.php:97
   │ /app/vendor/doctrine/data-fixtures/src/ReferenceRepository.php:136
   │ /app/vendor/doctrine/data-fixtures/src/AbstractFixture.php:63
   │ /app/tests/AppConfigMongodb/DataFixtures/MongoDB/LoadUserDataFixture.php:43
   │ /app/vendor/doctrine/data-fixtures/src/Executor/AbstractExecutor.php:86
   │ /app/vendor/doctrine/data-fixtures/src/Executor/MongoDBExecutor.php:65
   │ /app/src/Services/DatabaseTools/MongoDBDatabaseTool.php:85
   │ /app/tests/Test/ConfigMongodbTest.php:80
   │
@greg0ire
Copy link
Member

greg0ire commented Dec 2, 2024

After giving this a cursory glance, I think the type could be relaxed. I also saw an inconsistency:

* @phpstan-var array<class-string, array<string, mixed>>
*/
private array $identitiesByClass = [];

is not consistent with

* @phpstan-return array<class-string, array<string, object>>
*/
public function getIdentitiesByClass(): array

I might work on this later, but feel free to beat me to it.

@greg0ire
Copy link
Member

greg0ire commented Dec 2, 2024

PR for 1.8: #506

@greg0ire
Copy link
Member

@mbabker
Copy link
Contributor Author

mbabker commented Dec 10, 2024

Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants