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

Add support for ManyToMany Criteria #885

Merged
merged 15 commits into from
Mar 16, 2014
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@

namespace Doctrine\ORM\Cache\Persister;

use Doctrine\Common\Collections\Criteria;
use Doctrine\ORM\Cache\EntityCacheKey;
use Doctrine\ORM\Cache\CollectionCacheKey;
use Doctrine\ORM\Persisters\CollectionPersister;
Expand Down Expand Up @@ -272,4 +273,12 @@ public function slice(PersistentCollection $collection, $offset, $length = null)
{
return $this->persister->slice($collection, $offset, $length);
}

/**
* {@inheritDoc}
*/
public function loadCriteria(PersistentCollection $collection, Criteria $criteria)
{
return $this->persister->loadCriteria($collection, $criteria);
}
}
6 changes: 4 additions & 2 deletions lib/Doctrine/ORM/PersistentCollection.php
Original file line number Diff line number Diff line change
Expand Up @@ -869,8 +869,10 @@ public function matching(Criteria $criteria)
return $this->coll->matching($criteria);
}

if ($this->association['type'] !== ClassMetadata::ONE_TO_MANY) {
throw new \RuntimeException("Matching Criteria on PersistentCollection only works on OneToMany associations at the moment.");
if ($this->association['type'] === ClassMetadata::MANY_TO_MANY) {
$persister = $this->em->getUnitOfWork()->getCollectionPersister($this->association);

return new ArrayCollection($persister->loadCriteria($this, $criteria));
Copy link
Member

Choose a reason for hiding this comment

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

Add an empty newline before this line

}

$builder = Criteria::expr();
Expand Down
11 changes: 10 additions & 1 deletion lib/Doctrine/ORM/Persisters/AbstractCollectionPersister.php
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@

namespace Doctrine\ORM\Persisters;

use Doctrine\Common\Collections\Criteria;
use Doctrine\ORM\EntityManager;
use Doctrine\ORM\PersistentCollection;

Expand Down Expand Up @@ -51,7 +52,7 @@ abstract class AbstractCollectionPersister implements CollectionPersister
* @var \Doctrine\DBAL\Platforms\AbstractPlatform
*/
protected $platform;

/**
* The quote strategy.
*
Expand Down Expand Up @@ -203,6 +204,14 @@ public function get(PersistentCollection $coll, $index)
throw new \BadMethodCallException("Selecting a collection by index is not supported by this CollectionPersister.");
}

/**
* {@inheritdoc}
*/
public function loadCriteria(PersistentCollection $coll, Criteria $criteria)
{
throw new \BadMethodCallException("Filtering a collection by Criteria is not supported by this CollectionPersister.");
}

/**
* Gets the SQL statement used for deleting a row from the collection.
*
Expand Down
11 changes: 11 additions & 0 deletions lib/Doctrine/ORM/Persisters/CollectionPersister.php
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@

namespace Doctrine\ORM\Persisters;

use Doctrine\Common\Collections\Criteria;
use Doctrine\ORM\PersistentCollection;

/**
Expand Down Expand Up @@ -136,4 +137,14 @@ public function removeKey(PersistentCollection $collection, $key);
* @return mixed
*/
public function get(PersistentCollection $collection, $index);

/**
* Loads association entities matching the given Criteria object.
*
* @param \Doctrine\ORM\PersistentCollection $collection
* @param \Doctrine\Common\Collections\Criteria $criteria
*
* @return array
*/
public function loadCriteria(PersistentCollection $collection, Criteria $criteria);
Copy link
Member

Choose a reason for hiding this comment

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

@bakura10 Shouldn't the contract here be to throw an exception if the operation is unsupported by the implementing collection persister? IMO that should be documented here as you already do throw an exception in AbstractCollectionPersister. What do you think?

Copy link
Member Author

Choose a reason for hiding this comment

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

I mimic the other methods. All the other methods (slice, containsKey...) throw an exception in the AbstractCollectionPersister but it's not in the interface. I just followed the same pattern, if I add it here I should also add it for all other methods then :).

Copy link
Member

Choose a reason for hiding this comment

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

@bakura10 Okay I didn't know that. Then it's perfectly fine to me, no need to go a different way here then :)

}
135 changes: 111 additions & 24 deletions lib/Doctrine/ORM/Persisters/ManyToManyPersister.php
Original file line number Diff line number Diff line change
Expand Up @@ -19,8 +19,10 @@

namespace Doctrine\ORM\Persisters;

use Doctrine\Common\Collections\Criteria;
use Doctrine\ORM\Mapping\ClassMetadata;
use Doctrine\ORM\PersistentCollection;
use Doctrine\ORM\Query;
use Doctrine\ORM\UnitOfWork;

/**
Expand Down Expand Up @@ -54,7 +56,7 @@ protected function getDeleteRowSQL(PersistentCollection $coll)
}

return 'DELETE FROM ' . $tableName
. ' WHERE ' . implode(' = ? AND ', $columns) . ' = ?';
. ' WHERE ' . implode(' = ? AND ', $columns) . ' = ?';
}

/**
Expand Down Expand Up @@ -102,7 +104,7 @@ protected function getInsertRowSQL(PersistentCollection $coll)
}

return 'INSERT INTO ' . $joinTable . ' (' . implode(', ', $columns) . ')'
. ' VALUES (' . implode(', ', array_fill(0, count($columns), '?')) . ')';
. ' VALUES (' . implode(', ', array_fill(0, count($columns), '?')) . ')';
}

/**
Expand Down Expand Up @@ -178,7 +180,7 @@ protected function getDeleteSQL(PersistentCollection $coll)
}

return 'DELETE FROM ' . $joinTable
. ' WHERE ' . implode(' = ? AND ', $columns) . ' = ?';
. ' WHERE ' . implode(' = ? AND ', $columns) . ' = ?';
}

/**
Expand Down Expand Up @@ -257,7 +259,7 @@ public function count(PersistentCollection $coll)
}

/**
* {@inheritdoc}
* {@inheritDoc}
*/
public function slice(PersistentCollection $coll, $offset, $length = null)
{
Expand All @@ -276,7 +278,7 @@ public function containsKey(PersistentCollection $coll, $key)
}

/**
* {@inheritdoc}
* {@inheritDoc}
*/
public function contains(PersistentCollection $coll, $element)
{
Expand All @@ -302,7 +304,7 @@ public function contains(PersistentCollection $coll, $element)
}

/**
* {@inheritdoc}
* {@inheritDoc}
*/
public function removeElement(PersistentCollection $coll, $element)
{
Expand Down Expand Up @@ -485,52 +487,137 @@ public function getFilterSql($mapping)
return array('', '');
}

$conditions = array();
// A join is needed if there is filtering on the target entity
$tableName = $this->quoteStrategy->getTableName($rootClass, $this->platform);
$joinSql = ' JOIN ' . $tableName . ' te' . ' ON';
Copy link
Member

Choose a reason for hiding this comment

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

is te a convention throughout the class?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes it seems.

$onConditions = $this->getOnConditionSQL($mapping);

$joinSql .= implode(' AND ', $onConditions);

return array($joinSql, $filterSql);
}

/**
* Generates the filter SQL for a given entity and table alias.
*
* @param ClassMetadata $targetEntity Metadata of the target entity.
* @param string $targetTableAlias The table alias of the joined/selected table.
*
* @return string The SQL query part to add to a query.
*/
protected function generateFilterConditionSQL(ClassMetadata $targetEntity, $targetTableAlias)
{
$filterClauses = array();

foreach ($this->em->getFilters()->getEnabledFilters() as $filter) {
if ($filterExpr = $filter->addFilterConstraint($targetEntity, $targetTableAlias)) {
$filterClauses[] = '(' . $filterExpr . ')';
}
}

$sql = implode(' AND ', $filterClauses);
return $sql ? '(' . $sql . ')' : '';
}

/**
* Generate ON condition
*
* @param array $mapping
*
* @return array
Copy link
Member

Choose a reason for hiding this comment

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

picky: blank line missing before @return tag ;) (not really a law but is used everywhere else and reads better)

*/
protected function getOnConditionSQL($mapping)
{
$association = $mapping;

if ( ! $mapping['isOwningSide']) {
$class = $this->em->getClassMetadata($mapping['targetEntity']);
$association = $class->associationMappings[$mapping['mappedBy']];
}

// A join is needed if there is filtering on the target entity
$tableName = $this->quoteStrategy->getTableName($rootClass, $this->platform);
$joinSql = ' JOIN ' . $tableName . ' te' . ' ON';
$targetClass = $this->em->getClassMetadata($mapping['targetEntity']);

$joinColumns = $mapping['isOwningSide']
? $association['joinTable']['inverseJoinColumns']
: $association['joinTable']['joinColumns'];

$conditions = array();

foreach ($joinColumns as $joinColumn) {
$joinColumnName = $this->quoteStrategy->getJoinColumnName($joinColumn, $targetClass, $this->platform);
$refColumnName = $this->quoteStrategy->getReferencedJoinColumnName($joinColumn, $targetClass, $this->platform);

$conditions[] = ' t.' . $joinColumnName . ' = ' . 'te.' . $refColumnName;
}

$joinSql .= implode(' AND ', $conditions);
return $conditions;
}

return array($joinSql, $filterSql);
/**
* {@inheritDoc}
*/
public function loadCriteria(PersistentCollection $coll, Criteria $criteria)
{
$mapping = $coll->getMapping();
$owner = $coll->getOwner();
$ownerMetadata = $this->em->getClassMetadata(get_class($owner));
$whereClauses = $params = array();

foreach ($mapping['relationToSourceKeyColumns'] as $key => $value) {
$whereClauses[] = sprintf('t.%s = ?', $key);
$params[] = $ownerMetadata->getFieldValue($owner, $value);
}

$parameters = $this->expandCriteriaParameters($criteria);

foreach ($parameters as $parameter) {
list($name, $value) = $parameter;
$whereClauses[] = sprintf('te.%s = ?', $name);
$params[] = $value;
}

$mapping = $coll->getMapping();
$targetClass = $this->em->getClassMetadata($mapping['targetEntity']);
$tableName = $this->quoteStrategy->getTableName($targetClass, $this->platform);
$joinTable = $this->quoteStrategy->getJoinTableName($mapping, $ownerMetadata, $this->platform);
$onConditions = $this->getOnConditionSQL($mapping);

$rsm = new Query\ResultSetMappingBuilder($this->em);
$rsm->addRootEntityFromClassMetadata($mapping['targetEntity'], 'te');

$sql = 'SELECT ' . $rsm->generateSelectClause() . ' FROM ' . $tableName . ' te'
. ' JOIN ' . $joinTable . ' t ON'
. implode(' AND ', $onConditions)
. ' WHERE ' . implode(' AND ', $whereClauses);

$stmt = $this->conn->executeQuery($sql, $params);
$hydrator = $this->em->newHydrator(Query::HYDRATE_OBJECT);

return $hydrator->hydrateAll($stmt, $rsm);
}

/**
* Generates the filter SQL for a given entity and table alias.
* Expands Criteria Parameters by walking the expressions and grabbing all
* parameters and types from it.
*
* @param ClassMetadata $targetEntity Metadata of the target entity.
* @param string $targetTableAlias The table alias of the joined/selected table.
* @param \Doctrine\Common\Collections\Criteria $criteria
*
* @return string The SQL query part to add to a query.
* @return array
*/
protected function generateFilterConditionSQL(ClassMetadata $targetEntity, $targetTableAlias)
private function expandCriteriaParameters(Criteria $criteria)
{
$filterClauses = array();
$expression = $criteria->getWhereExpression();

foreach ($this->em->getFilters()->getEnabledFilters() as $filter) {
if ($filterExpr = $filter->addFilterConstraint($targetEntity, $targetTableAlias)) {
$filterClauses[] = '(' . $filterExpr . ')';
}
if ($expression === null) {
return array();
}

$sql = implode(' AND ', $filterClauses);
return $sql ? "(" . $sql . ")" : "";
$valueVisitor = new SqlValueVisitor();

$valueVisitor->dispatch($expression);

list($values, $types) = $valueVisitor->getParamsAndTypes();

return $types;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,7 @@ abstract class AbstractCollectionPersisterTest extends OrmTestCase
'removeElement',
'removeKey',
'get',
'loadCriteria'
);

/**
Expand Down Expand Up @@ -298,4 +299,4 @@ public function testInvokeGet()

$this->assertEquals($element, $persister->get($collection, 0));
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

namespace Doctrine\Tests\ORM\Functional;

use Doctrine\Common\Collections\Criteria;
use Doctrine\Tests\Models\CMS\CmsUser,
Doctrine\Tests\Models\CMS\CmsGroup,
Doctrine\Common\Collections\ArrayCollection;
Expand Down Expand Up @@ -377,4 +378,25 @@ public function testClearBeforeLazyLoad()
$user = $this->_em->find(get_class($user), $user->id);
$this->assertEquals(0, count($user->groups));
}

public function testMatching()
{
$user = $this->addCmsUserGblancoWithGroups(2);
$this->_em->clear();

$user = $this->_em->find(get_class($user), $user->id);

$groups = $user->groups;
$this->assertFalse($user->groups->isInitialized(), "Pre-condition: lazy collection");

$criteria = Criteria::create()->where(Criteria::expr()->eq('name', (string) 'Developers_0'));
$result = $groups->matching($criteria);

$this->assertCount(1, $result);

$firstGroup = $result->first();
$this->assertEquals('Developers_0', $firstGroup->name);

$this->assertFalse($user->groups->isInitialized(), "Post-condition: matching does not initialize collection");
}
}