Skip to content

[DDC-2166] Refactor identity hash generation #522

Closed
wants to merge 1 commit into from

6 participants

@beberlei
Doctrine member

This work prepares for the merge of GH-232, allowing more complex and robust identifier hash generation.

@doctrinebot

Hello,

thank you for positing this Pull Request. I have automatically opened an issue on our Jira Bug Tracker for you with the details of this Pull-Request. See the Link:

http://doctrine-project.org/jira/browse/DDC-2167

@Ocramius Ocramius commented on the diff Nov 26, 2012
lib/Doctrine/ORM/UnitOfWork.php
+ *
+ * @param ClassMetadata $class
+ * @param mixed $id
+ * @return array
+ */
+ public function normalizeIdentifier($class, $id)
+ {
+ if (is_object($id) && $this->em->getMetadataFactory()->hasMetadataFor(ClassUtils::getClass($id))) {
+ $id = $this->getSingleIdentifierValue($id);
+
+ if ($id === null) {
+ throw ORMInvalidArgumentException::invalidIdentifierBindingEntity();
+ }
+ }
+
+ if ( ! is_array($id)) {
@Ocramius
Doctrine member
Ocramius added a note Nov 26, 2012

Identifiers are always arrays as of class metadata. We shouldn't introduce more inconsistencies on that one (already a mess in the various ODMs, where that interface is currently ignored).

@beberlei
Doctrine member
beberlei added a note Nov 26, 2012

$id is a scalar when people call $entityManager->find("Entity", 1); Thats why the method is about normalizing.

@Ocramius
Doctrine member
Ocramius added a note Nov 26, 2012

@beberlei yeah, but then we're adding this call everywhere (instead of just where the API states @param mixed $id). Nvm, guess it's an acceptable overhead.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@Ocramius Ocramius commented on the diff Nov 26, 2012
lib/Doctrine/ORM/UnitOfWork.php
@@ -3069,4 +3043,65 @@ public function isReadOnly($object)
return isset($this->readOnlyObjects[spl_object_hash($object)]);
}
+
+ /**
+ * Normalize identifier into a sorted array of identifier values.
+ *
+ * @param ClassMetadata $class
+ * @param mixed $id
+ * @return array
+ */
+ public function normalizeIdentifier($class, $id)
+ {
+ if (is_object($id) && $this->em->getMetadataFactory()->hasMetadataFor(ClassUtils::getClass($id))) {
@Ocramius
Doctrine member
Ocramius added a note Nov 26, 2012

Shouldn't this be moved at line 3074?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@Ocramius Ocramius commented on the diff Nov 26, 2012
lib/Doctrine/ORM/UnitOfWork.php
+ }
+
+ $sortedId = array();
+
+ foreach ($class->identifier as $identifier) {
+ if ( ! isset($id[$identifier])) {
+ throw ORMException::missingIdentifierField($class->name, $identifier);
+ }
+
+ $sortedId[$identifier] = $id[$identifier];
+ }
+
+ return $sortedId;
+ }
+
+ public function getHashForEntityIdentifier($class, $id)
@Ocramius
Doctrine member
Ocramius added a note Nov 26, 2012

Don't forget type hints on those :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@FabioBatSilva FabioBatSilva commented on the diff Nov 26, 2012
...e/ORM/Internal/IdentityMap/DerivedKeyHashStrategy.php
+ * @var \Doctrine\ORM\Mapping\ClassMetadata
+ */
+ private $class;
+
+ /**
+ * @var \Doctrine\ORM\EntityManager
+ */
+ private $entityManager;
+
+ public function __construct(ClassMetadata $class, EntityManager $entityManager)
+ {
+ $this->class = $class;
+ $this->entityManager = $entityManager;
+ }
+
+ public function getHash(array $identifier)
@FabioBatSilva
Doctrine member

Could you assign the callback to a variable and align the =

Something like :

<?php
/**
 * {@inheritdoc}
 */
public function getHash(array $identifier)
{
    $class      = $this->class;
    $em         = $this->entityManager;
    $callback   = function ($fieldName) use ($identifier, $class, $em)
    {
        if (is_object($identifier[$fieldName]) && ! $em->getMetadataFactory()->isTransient($identifier[$fieldName])) {
            $class  = $em->getClassMetadata(get_class($identifier[$fieldName]));
            $id     = $em->getUnitOfWork()->getEntityIdentifier($identifier[$fieldName]);

            return $em->getUnitOfWork()->getHashForEntityIdentifier($class, $id);
        }

        return $identifier[$fieldName];
    };

    return implode(' ', array_map($callback, $class->identifier));
}
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@FabioBatSilva FabioBatSilva commented on the diff Nov 26, 2012
lib/Doctrine/ORM/UnitOfWork.php
@@ -2773,7 +2746,8 @@ public function getSingleIdentifierValue($entity)
*/
public function tryGetById($id, $rootClassName)
{
- $idHash = implode(' ', (array) $id);
+ $classMetadata = $this->em->getClassMetadata($rootClassName);
@FabioBatSilva
Doctrine member

Please align = signs

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@TomasVotruba

This is almost 2 years old. Still relevant @beberlei ?

If so, it would be great to merge the change. As the time goes by, potential merge conflict will grow.

@Ocramius
Doctrine member

@guilhermeblanco can you allocate some of @FabioBatSilva's time to check this and #1113 PR?

@Ocramius Ocramius self-assigned this Nov 11, 2014
@Ocramius
Doctrine member

This has been partially handled in the IdentifierFlattener class now. Closing as incomplete.

@Ocramius Ocramius closed this Nov 11, 2014
@Ocramius Ocramius deleted the DDC-2166 branch Nov 11, 2014
@TomasVotruba

@Ocramius Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.