search

doctrine / data-fixtures

Doctrine2 ORM Data Fixtures Extensions
http://www.doctrine-project.org
MIT License
2.78k stars 224 forks source link

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

Open mbabker opened 20 hours ago

mbabker commented 20 hours ago

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 https://github.com/liip/LiipTestFixturesBundle/pull/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 commented 10 hours ago

After giving this a cursory glance, I think the type could be relaxed. I also saw an inconsistency: https://github.com/doctrine/data-fixtures/blob/26536cc8a47f29432bfa4e1f117b23cff145cd1f/src/ReferenceRepository.php#L37-L39

is not consistent with

https://github.com/doctrine/data-fixtures/blob/26536cc8a47f29432bfa4e1f117b23cff145cd1f/src/ReferenceRepository.php#L211-L213

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

greg0ire commented 8 hours ago

PR for 1.8: https://github.com/doctrine/data-fixtures/pull/506