doctrine / data-fixtures

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

Use UnitOfWork->contains() instead of isInIdentityMap when Phpcr document #201

Closed aramalipoor closed 9 years ago

aramalipoor commented 9 years ago

Unfortunately isInIdentityMap does not exist in Phpcr's DocumentManager which is raising issues when for example we're trying to setReference a document.

I'm not an expert in Doctrine stuff so I'm not sure if this is the right way to do it.

Related issue https://github.com/doctrine/phpcr-odm/issues/492

stof commented 9 years ago

@lsmith77 @dbu can you check this ? Will it work properly when setting a reference before flushing ?

lsmith77 commented 9 years ago

maybe it makes more sense to use contains() on the ObjectManager: https://github.com/doctrine/common/blob/master/lib/Doctrine/Common/Persistence/ObjectManager.php#L168

https://github.com/doctrine/doctrine2/blob/master/lib/Doctrine/ORM/EntityManager.php#L708

stof commented 9 years ago

@lsmith77 won't work on the ORM, because the manager can contain the object without knowing its id if the object is scheduled for insertion and the id is an auto-increment in the DB, which will be known only after the flush

lsmith77 commented 9 years ago

so you need this logic?

so this is the logic you need? https://github.com/doctrine/doctrine2/blob/master/lib/Doctrine/ORM/UnitOfWork.php#L1594

then probably this method is better: https://github.com/doctrine/phpcr-odm/blob/master/lib/Doctrine/ODM/PHPCR/UnitOfWork.php#L3091

stof commented 9 years ago

@lsmith77 this looks weird. the method you are suggesting expects an id as argument, while what we are trying to do is precisely to retrieve the id of the object (but only when it is available)

stof commented 9 years ago

There are a bunch of other usages of isInIdentityMap in this class, so the PR is incomplete

aramalipoor commented 9 years ago

@stof I've created a helper method to use everywhere we need to check for the identifier. What do you think? Is hasIdentifier() a good name for it?

aramalipoor commented 9 years ago

@stof Done.