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

Fixed ObjectHydrator when namespace alias is given. #554

Merged
merged 5 commits into from
Feb 2, 2013

Conversation

beregond
Copy link
Contributor

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
Copy link

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
Copy link
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.

@@ -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);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@beregond
Copy link
Contributor Author

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
Copy link
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
Copy link
Contributor Author

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.


$translate = function (&$alias) use ($em, $fqcn)
{
if (strpos($alias, ':') !== false && !isset($fqcn[$alias])) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can completely be simplified into:

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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();

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

$fqcn = array();

$translate = function (&$alias) use ($em, $fqcn)
{
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this curly brace should be on the previous line

guilhermeblanco added a commit that referenced this pull request Feb 2, 2013
Fixed ObjectHydrator when namespace alias is given.
@guilhermeblanco guilhermeblanco merged commit dea37ed into doctrine:master Feb 2, 2013
@beregond beregond deleted the hydrator-fix branch February 5, 2013 09:35
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

Successfully merging this pull request may close these issues.

7 participants