Fixed ObjectHydrator when namespace alias is given. #554

Merged
merged 5 commits into from Feb 2, 2013

5 participants

@beregond

Fixes problem when executing native query:

$rsm = new ResultSetMapping();

$rsm->addEntityResult('DbBundle:Player', 'p');
$rsm->addFieldResult('p', 'id', 'id');
$rsm->addFieldResult('p', 'name', 'name');

$rsm->addJoinedEntityResult('DbBundle:User', 'u', 'p', 'user');
$rsm->addFieldResult('u', 'user_id', 'id');

$em->createNativeQuery('...', $rsm);

Hydrator couldn't find cached metadata, when "joined entity result" class name wasn't fully qualified name (as in the example above).

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

@Ocramius
Doctrine member

@beregond please add the failing test for this problem.
Also, I think this should be simply added in addEntityResult and addJoinedEntityResult to avoid performance drawbacks.

@norzechowicz norzechowicz and 1 other commented on an outdated diff Jan 24, 2013
lib/Doctrine/ORM/Internal/Hydration/ObjectHydrator.php
@@ -102,6 +102,12 @@ protected function prepare()
$this->identifierMap[$dqlAlias] = array();
$this->idTemplate[$dqlAlias] = '';
+ // Check for namespace alias.
+ if (strpos($className, ':') !== false) {
+ $metadata = $this->_em->getClassMetadata($className);

I'm not sure about this line.
If $className have ":" then _ce cache is useless because Entity Manager will be called every single time when this line is executed.

Well it's pain, but don't see other way now to make sure we have fully qualified name of class.

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

I wrote test and found out other place that need to fix. Best way is to translate namespace alias in ResultSetMapping, but is that somehow possible?

@beberlei
Doctrine member

We actually didn't support this for performance reasons (it affects the normal DQL queries too), but I see value in adding this for user convience. However i think the location is wrong. Rather than doing it in the hydration process, the Query object should actually fix a RSM when its set. This way we keep the transformation at the source.

@beregond

I've fixed test and added case for ResultSetMappingBuilder which has same problem.

This way it's now allows to translate namespaces whithout breaking BC.

@Ocramius Ocramius and 1 other commented on an outdated diff Jan 25, 2013
lib/Doctrine/ORM/Query/ResultSetMapping.php
@@ -543,4 +545,33 @@ public function addMetaResult($alias, $columnName, $fieldName, $isIdentifierColu
return $this;
}
+
+ /**
+ * Allows to translate entity namespaces to full qualified names.
+ *
+ * @param EntityManager $em
+ *
+ * @return void
+ */
+ public function translateNamespaces(EntityManager $em)
+ {
+ $fqcn = array();
+
+ $translate = function (&$alias) use ($em, $fqcn)
+ {
+ if (strpos($alias, ':') !== false && !isset($fqcn[$alias])) {
@Ocramius
Doctrine member

Can completely be simplified into:

$em->getClassMetadata($alias)->getName()

The metadata factory already handles namespaces. No need for additional function calls

Well I think it's important to check if it's namespace alias for sure, so we can have temporary mapping in $fqcn like:

$fqcn = array(
    'AcmeBundle:User' => 'Acme\SomeBundle\Entity\User',
    'AcmeBundle:Group' => 'Acme\SomeBundle\Entity\Group',
);

so we won't have to get metadata each time 'AcmeBundle:User' occurs. In fact I could remake three lines below to form:

$fqcn[$alias] = $em->getClassMetadata($alias)->getName();
@Ocramius
Doctrine member

You can just keep the map (not the strpos check)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@Ocramius Ocramius commented on an outdated diff Jan 25, 2013
lib/Doctrine/ORM/Query/ResultSetMapping.php
+
+ /**
+ * Allows to translate entity namespaces to full qualified names.
+ *
+ * @param EntityManager $em
+ *
+ * @return void
+ */
+ public function translateNamespaces(EntityManager $em)
+ {
+ $fqcn = array();
+
+ $translate = function (&$alias) use ($em, $fqcn)
+ {
+ if (strpos($alias, ':') !== false && !isset($fqcn[$alias])) {
+ if ($metadata = $em->getClassMetadata($alias)) {
@Ocramius
Doctrine member

This check is not needed. It will cause an exception on no retrieved metadata

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@stof stof commented on an outdated diff Jan 26, 2013
lib/Doctrine/ORM/Query/ResultSetMapping.php
@@ -543,4 +545,28 @@ public function addMetaResult($alias, $columnName, $fieldName, $isIdentifierColu
return $this;
}
+
+ /**
+ * Allows to translate entity namespaces to full qualified names.
+ *
+ * @param EntityManager $em
+ *
+ * @return void
+ */
+ public function translateNamespaces(EntityManager $em)
+ {
+ $fqcn = array();
+
+ $translate = function (&$alias) use ($em, $fqcn)
+ {
@stof
Doctrine member
stof added a note Jan 26, 2013

this curly brace should be on the previous line

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@stof stof commented on an outdated diff Jan 26, 2013
lib/Doctrine/ORM/Query/ResultSetMapping.php
@@ -543,4 +545,28 @@ public function addMetaResult($alias, $columnName, $fieldName, $isIdentifierColu
return $this;
}
+
+ /**
+ * Allows to translate entity namespaces to full qualified names.
+ *
+ * @param EntityManager $em
+ *
+ * @return void
+ */
+ public function translateNamespaces(EntityManager $em)
+ {
+ $fqcn = array();
+
+ $translate = function (&$alias) use ($em, $fqcn)
+ {
+ if ( ! isset($fqcn[$alias])) {
@stof
Doctrine member
stof added a note Jan 26, 2013

This will never be set as you don't pass $fqcn by reference

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@stof stof commented on an outdated diff Jan 26, 2013
.../Doctrine/Tests/ORM/Functional/Ticket/DDC2256Test.php
@@ -0,0 +1,117 @@
+<?php
+
+namespace Doctrine\Tests\ORM\Functional\Ticket;
+
+require_once __DIR__ . '/../../../TestInit.php';
@stof
Doctrine member
stof added a note Jan 26, 2013

This line is not needed as it is already the phpunit bootstrap script

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@guilhermeblanco guilhermeblanco merged commit dea37ed into doctrine:master Feb 2, 2013

1 check was pending

Details default The Travis build is in progress
@beregond beregond deleted the beregond:hydrator-fix branch Feb 5, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment