Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

Add EntityRepository->countBy() method to complement findBy() #560

Closed
wants to merge 1 commit into from

5 participants

@alex-signature-it

It would be reasonable to add the method to ObjectRepository interface, but I might not be aware of all its implementations that will become broken

@alex-signature-it alex-signature-it Add EntityRepository->countBy() method to complement findBy()
It would be reasonable to add the method to ObjectRepository interface, but I might not be aware of all its implementations that will become broken
717fd3d
@doctrinebot
Collaborator

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-2263

@stof stof commented on the diff
...e/Tests/ORM/Functional/SingleTableInheritanceTest.php
@@ -5,6 +5,8 @@
use Doctrine\ORM\Mapping\ClassMetadata;
use Doctrine\Common\Collections\Criteria;
+require_once __DIR__ . '/../../TestInit.php';
@stof
stof added a note

this must be removed. This file is already the phpunit bootstrap file so it does not need to be included again (except if you copied an outdated phpunit.xml)

I totally agree, that it's better to include it from bootstrap only, but that's just not my call to make such a change.
I must point out, that I made my phpunit.xml file from dbproperties.xml.dev according to the instructions provided in its comment, and it doesn't mention any bootstrap. Without this line I wasn't able to run tests for this particular class only, and if you'll look at almost any other functional test class, you'll notice that most of them explicitly require_once this file.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@stof stof commented on the diff
tests/Doctrine/Tests/ORM/Functional/TypeTest.php
@@ -127,7 +127,9 @@ public function testDateTime()
$this->assertEquals('2009-10-02 20:10:52', $dateTimeDb->datetime->format('Y-m-d H:i:s'));
$articles = $this->_em->getRepository( 'Doctrine\Tests\Models\Generic\DateTimeModel' )->findBy( array( 'datetime' => new \DateTime( "now" ) ) );
- $this->assertEquals( 0, count( $articles ) );
+ $this->assertEquals(0, count($articles));
@stof
stof added a note

you should use assertCount

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@stof stof commented on the diff
lib/Doctrine/ORM/Persisters/BasicEntityPersister.php
@@ -1127,6 +1141,40 @@ protected function getSelectSQL($criteria, $assoc = null, $lockMode = 0, $limit
}
/**
+ * Gets the SELECT SQL to count entities by a set of field criteria.
+ *
+ * @param array|\Doctrine\Common\Collections\Criteria $criteria
+ * @return string
+ */
+ protected function _getCountSQL($criteria)
+ {
+ $conditionSql = ($criteria instanceof Criteria)
@stof
stof added a note

you are allowing a Criteria object bere but not in countAll

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@stof stof commented on the diff
lib/Doctrine/ORM/Persisters/BasicEntityPersister.php
((17 lines not shown))
+ $tableName = $this->quoteStrategy->getTableName($this->class, $this->platform);
+
+ if ('' !== $filterSql) {
+ $conditionSql = $conditionSql
+ ? $conditionSql . ' AND ' . $filterSql
+ : $filterSql;
+ }
+
+ $select = 'SELECT COUNT(1)';
+ $from = ' FROM ' . $tableName . ' ' . $tableAlias;
+ $join = $this->selectJoinSql;
+ $where = ($conditionSql ? ' WHERE ' . $conditionSql : '');
+ $query = $select
+ . $from
+ . $join
+ . $where;
@stof
stof added a note

you should keep this concatenation on 1 line

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@stof stof commented on the diff
lib/Doctrine/ORM/Persisters/BasicEntityPersister.php
((13 lines not shown))
+ : $this->getSelectConditionSQL($criteria);
+
+ $tableAlias = $this->getSQLTableAlias($this->class->name);
+ $filterSql = $this->generateFilterConditionSQL($this->class, $tableAlias);
+ $tableName = $this->quoteStrategy->getTableName($this->class, $this->platform);
+
+ if ('' !== $filterSql) {
+ $conditionSql = $conditionSql
+ ? $conditionSql . ' AND ' . $filterSql
+ : $filterSql;
+ }
+
+ $select = 'SELECT COUNT(1)';
+ $from = ' FROM ' . $tableName . ' ' . $tableAlias;
+ $join = $this->selectJoinSql;
+ $where = ($conditionSql ? ' WHERE ' . $conditionSql : '');
@stof
stof added a note

the braces are useless

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

Thank you for this PR @alex-signature-it, however this is what DQL is for, we cannot add everything to the repository just to avoid having to write DQL. This would add a duplicate feature and i am not sure it provides additional value.

Furthermore, this essentially expands the ObjectManager interface and increases incompatibility accross implementations even more.

@beberlei beberlei closed this
@alex-signature-it

First of all, I'd like to thank @stof for the feedback. Although most of the comments are related to the code and interfaces that were there before I made any changes, all of them are reasonable and I'll attend them.

@alex-signature-it

Now I'd like to appeal to @beberlei to reconsider decision to reject this pull request by the following reasoning:
countBy() method naturally complements findBy(), if you allow one of them, chances are high that you'll need the other as well.

Consider the following use case: you need to provide some kind of a view for a list of entities with option to filter it by entity attributes. It's quite natural to use findBy() method to load relevant data. Now consider that you also would like to provide any kind of incremental loading feature for this list (paging, AJAX incremental loading etc.). While being organic extension of the previous feature, you'll have to drastically change the way you work with ORM.

@Ocramius
Owner

@alex-signature-it we have a paginator written especially for this. What can eventually be done in your case is extending the criteria API so that it allows counting items in a lazy collection in a manner that doesn't load all the items in the collection itself.

And that's an implementation detail of an already existing API :)

$criteria = new Criteria();
// configure criteria here
$collection = $repo->matching($criteria);
var_dump($collection->count());
@stof

@Ocramius I opened DDC-2217 1 month ago for this.

@Ocramius
Owner

@stof that's awesome =)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Jan 27, 2013
  1. @alex-signature-it

    Add EntityRepository->countBy() method to complement findBy()

    alex-signature-it authored
    It would be reasonable to add the method to ObjectRepository interface, but I might not be aware of all its implementations that will become broken
This page is out of date. Refresh to see the latest.
View
18 lib/Doctrine/ORM/EntityRepository.php
@@ -182,6 +182,24 @@ public function findBy(array $criteria, array $orderBy = null, $limit = null, $o
}
/**
+ * Counts objects that satisfy a set of criteria.
+ *
+ * An implementation may throw an UnexpectedValueException if certain values
+ * of the limiting details are not supported.
+ *
+ * @throws \UnexpectedValueException
+ * @param array $criteria
+ * @return int
+ */
+ public function countBy(array $criteria)
+ {
+ $persister = $this->_em->getUnitOfWork()->getEntityPersister($this->_entityName);
+
+ return $persister->countAll($criteria);
+ }
+
+
+ /**
* Finds a single entity by a set of criteria.
*
* @param array $criteria
View
48 lib/Doctrine/ORM/Persisters/BasicEntityPersister.php
@@ -912,6 +912,20 @@ public function loadAll(array $criteria = array(), array $orderBy = null, $limit
}
/**
+ * Counts entities by a list of field criteria.
+ *
+ * @param array $criteria
+ * @return int
+ */
+ public function countAll(array $criteria = array())
+ {
+ $sql = $this->_getCountSQL($criteria);
+ list($params, $types) = $this->expandParameters($criteria);
+ $stmt = $this->conn->executeQuery($sql, $params, $types);
+ return intval($stmt->fetchColumn());
+ }
+
+ /**
* Gets (sliced or full) elements of the given collection.
*
* @param array $assoc
@@ -1127,6 +1141,40 @@ protected function getSelectSQL($criteria, $assoc = null, $lockMode = 0, $limit
}
/**
+ * Gets the SELECT SQL to count entities by a set of field criteria.
+ *
+ * @param array|\Doctrine\Common\Collections\Criteria $criteria
+ * @return string
+ */
+ protected function _getCountSQL($criteria)
+ {
+ $conditionSql = ($criteria instanceof Criteria)
@stof
stof added a note

you are allowing a Criteria object bere but not in countAll

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+ ? $this->getSelectConditionCriteriaSQL($criteria)
+ : $this->getSelectConditionSQL($criteria);
+
+ $tableAlias = $this->getSQLTableAlias($this->class->name);
+ $filterSql = $this->generateFilterConditionSQL($this->class, $tableAlias);
+ $tableName = $this->quoteStrategy->getTableName($this->class, $this->platform);
+
+ if ('' !== $filterSql) {
+ $conditionSql = $conditionSql
+ ? $conditionSql . ' AND ' . $filterSql
+ : $filterSql;
+ }
+
+ $select = 'SELECT COUNT(1)';
+ $from = ' FROM ' . $tableName . ' ' . $tableAlias;
+ $join = $this->selectJoinSql;
+ $where = ($conditionSql ? ' WHERE ' . $conditionSql : '');
@stof
stof added a note

the braces are useless

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+ $query = $select
+ . $from
+ . $join
+ . $where;
@stof
stof added a note

you should keep this concatenation on 1 line

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+
+ return $query;
+ }
+
+ /**
* Gets the ORDER BY SQL snippet for ordered collections.
*
* @param array $orderBy
View
83 tests/Doctrine/Tests/ORM/Functional/EntityRepositoryTest.php
@@ -185,6 +185,15 @@ public function testFindByField()
$this->assertEquals('dev', $users[0]->status);
}
+ public function testCountByField()
+ {
+ $user1Id = $this->loadFixture();
+ $repos = $this->_em->getRepository('Doctrine\Tests\Models\CMS\CmsUser');
+
+ $userCount = $repos->countBy(array('status' => 'dev'));
+ $this->assertEquals(2, $userCount);
+ }
+
public function testFindByAssociationWithIntegerAsParameter()
{
$address1 = $this->buildAddress('Germany', 'Berlim', 'Foo st.', '123456');
@@ -209,6 +218,29 @@ public function testFindByAssociationWithIntegerAsParameter()
$this->assertInstanceOf('Doctrine\Tests\Models\CMS\CmsAddress',$addresses[0]);
}
+ public function testCountByAssociationWithIntegerAsParameter()
+ {
+ $address1 = $this->buildAddress('Germany', 'Berlim', 'Foo st.', '123456');
+ $user1 = $this->buildUser('Benjamin', 'beberlei', 'dev', $address1);
+
+ $address2 = $this->buildAddress('Brazil', 'São Paulo', 'Bar st.', '654321');
+ $user2 = $this->buildUser('Guilherme', 'guilhermeblanco', 'freak', $address2);
+
+ $address3 = $this->buildAddress('USA', 'Nashville', 'Woo st.', '321654');
+ $user3 = $this->buildUser('Jonathan', 'jwage', 'dev', $address3);
+
+ unset($address1);
+ unset($address2);
+ unset($address3);
+
+ $this->_em->clear();
+
+ $repository = $this->_em->getRepository('Doctrine\Tests\Models\CMS\CmsAddress');
+ $addressCount = $repository->countBy(array('user' => array($user1->getId(), $user2->getId())));
+
+ $this->assertEquals(2, $addressCount);
+ }
+
public function testFindByAssociationWithObjectAsParameter()
{
$address1 = $this->buildAddress('Germany', 'Berlim', 'Foo st.', '123456');
@@ -233,6 +265,29 @@ public function testFindByAssociationWithObjectAsParameter()
$this->assertInstanceOf('Doctrine\Tests\Models\CMS\CmsAddress',$addresses[0]);
}
+ public function testCountByAssociationWithObjectAsParameter()
+ {
+ $address1 = $this->buildAddress('Germany', 'Berlim', 'Foo st.', '123456');
+ $user1 = $this->buildUser('Benjamin', 'beberlei', 'dev', $address1);
+
+ $address2 = $this->buildAddress('Brazil', 'São Paulo', 'Bar st.', '654321');
+ $user2 = $this->buildUser('Guilherme', 'guilhermeblanco', 'freak', $address2);
+
+ $address3 = $this->buildAddress('USA', 'Nashville', 'Woo st.', '321654');
+ $user3 = $this->buildUser('Jonathan', 'jwage', 'dev', $address3);
+
+ unset($address1);
+ unset($address2);
+ unset($address3);
+
+ $this->_em->clear();
+
+ $repository = $this->_em->getRepository('Doctrine\Tests\Models\CMS\CmsAddress');
+ $addressCount = $repository->countBy(array('user' => array($user1, $user2)));
+
+ $this->assertEquals(2, $addressCount);
+ }
+
public function testFindFieldByMagicCall()
{
$user1Id = $this->loadFixture();
@@ -376,6 +431,15 @@ public function testFindByAssociationKey_ExceptionOnInverseSide()
$user = $repos->findBy(array('address' => $addressId));
}
+ public function testCountByAssociationKey_ExceptionOnInverseSide()
+ {
+ list($userId, $addressId) = $this->loadAssociatedFixture();
+ $repos = $this->_em->getRepository('Doctrine\Tests\Models\CMS\CmsUser');
+
+ $this->setExpectedException('Doctrine\ORM\ORMException', "You cannot search for the association field 'Doctrine\Tests\Models\CMS\CmsUser#address', because it is the inverse side of an association. Find methods only work on owning side associations.");
+ $userCount = $repos->countBy(array('address' => $addressId));
+ }
+
/**
* @group DDC-817
*/
@@ -417,6 +481,15 @@ public function testFindByAssociationKey()
$this->assertEquals($addressId, $addresses[0]->id);
}
+ public function testCountByAssociationKey()
+ {
+ list($userId, $addressId) = $this->loadAssociatedFixture();
+ $repos = $this->_em->getRepository('Doctrine\Tests\Models\CMS\CmsAddress');
+ $addressCount = $repos->countBy(array('user' => $userId));
+
+ $this->assertEquals(1, $addressCount);
+ }
+
/**
* @group DDC-817
*/
@@ -486,6 +559,16 @@ public function testIsNullCriteria()
$this->assertEquals(1, count($users));
}
+ public function testCountByNullCriteria()
+ {
+ $this->loadFixture();
+
+ $repos = $this->_em->getRepository('Doctrine\Tests\Models\CMS\CmsUser');
+
+ $userCount = $repos->countBy(array('status' => null));
+ $this->assertEquals(1, $userCount);
+ }
+
/**
* @group DDC-1094
*/
View
12 tests/Doctrine/Tests/ORM/Functional/SQLFilterTest.php
@@ -402,6 +402,18 @@ public function testRepositoryFindBy()
$this->assertCount(0, $this->_em->getRepository('Doctrine\Tests\Models\CMS\CmsGroup')->findBy(array('id' => $this->groupId2)));
}
+ public function testRepositoryCountBy()
+ {
+ $this->loadFixtureData();
+
+ $this->assertSame(1, $this->_em->getRepository('Doctrine\Tests\Models\CMS\CmsGroup')->countBy(array('id' => $this->groupId2)));
+
+ $this->useCMSGroupPrefixFilter();
+ $this->_em->clear();
+
+ $this->assertSame(0, $this->_em->getRepository('Doctrine\Tests\Models\CMS\CmsGroup')->countBy(array('id' => $this->groupId2)));
+ }
+
public function testRepositoryFindByX()
{
$this->loadFixtureData();
View
23 tests/Doctrine/Tests/ORM/Functional/SingleTableInheritanceTest.php
@@ -5,6 +5,8 @@
use Doctrine\ORM\Mapping\ClassMetadata;
use Doctrine\Common\Collections\Criteria;
+require_once __DIR__ . '/../../TestInit.php';
@stof
stof added a note

this must be removed. This file is already the phpunit bootstrap file so it does not need to be included again (except if you copied an outdated phpunit.xml)

I totally agree, that it's better to include it from bootstrap only, but that's just not my call to make such a change.
I must point out, that I made my phpunit.xml file from dbproperties.xml.dev according to the instructions provided in its comment, and it doesn't mention any bootstrap. Without this line I wasn't able to run tests for this particular class only, and if you'll look at almost any other functional test class, you'll notice that most of them explicitly require_once this file.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+
class SingleTableInheritanceTest extends \Doctrine\Tests\OrmFunctionalTestCase
{
private $salesPerson;
@@ -334,6 +336,27 @@ public function testFindByAssociation()
$this->assertEquals(1, count($contracts), "There should be 1 entities related to " . $this->salesPerson->getId() . " for 'Doctrine\Tests\Models\Company\CompanyFlexUltraContract'");
}
+ public function testCountByAssociation()
+ {
+ $this->loadFullFixture();
+
+ $repos = $this->_em->getRepository("Doctrine\Tests\Models\Company\CompanyContract");
+ $contractCount = $repos->countBy(array('salesPerson' => $this->salesPerson->getId()));
+ $this->assertEquals(3, $contractCount, "There should be 3 entities related to " . $this->salesPerson->getId() . " for 'Doctrine\Tests\Models\Company\CompanyContract'");
+
+ $repos = $this->_em->getRepository("Doctrine\Tests\Models\Company\CompanyFixContract");
+ $contractCount = $repos->countBy(array('salesPerson' => $this->salesPerson->getId()));
+ $this->assertEquals(1, $contractCount, "There should be 1 entities related to " . $this->salesPerson->getId() . " for 'Doctrine\Tests\Models\Company\CompanyFixContract'");
+
+ $repos = $this->_em->getRepository("Doctrine\Tests\Models\Company\CompanyFlexContract");
+ $contractCount = $repos->countBy(array('salesPerson' => $this->salesPerson->getId()));
+ $this->assertEquals(2, $contractCount, "There should be 2 entities related to " . $this->salesPerson->getId() . " for 'Doctrine\Tests\Models\Company\CompanyFlexContract'");
+
+ $repos = $this->_em->getRepository("Doctrine\Tests\Models\Company\CompanyFlexUltraContract");
+ $contractCount = $repos->countBy(array('salesPerson' => $this->salesPerson->getId()));
+ $this->assertEquals(1, $contractCount, "There should be 1 entities related to " . $this->salesPerson->getId() . " for 'Doctrine\Tests\Models\Company\CompanyFlexUltraContract'");
+ }
+
/**
* @group DDC-1637
*/
View
4 tests/Doctrine/Tests/ORM/Functional/TypeTest.php
@@ -127,7 +127,9 @@ public function testDateTime()
$this->assertEquals('2009-10-02 20:10:52', $dateTimeDb->datetime->format('Y-m-d H:i:s'));
$articles = $this->_em->getRepository( 'Doctrine\Tests\Models\Generic\DateTimeModel' )->findBy( array( 'datetime' => new \DateTime( "now" ) ) );
- $this->assertEquals( 0, count( $articles ) );
+ $this->assertEquals(0, count($articles));
@stof
stof added a note

you should use assertCount

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+ $articleCount = $this->_em->getRepository('Doctrine\Tests\Models\Generic\DateTimeModel')->countBy(array('datetime' => new \DateTime("now")));
+ $this->assertEquals(0, $articleCount);
}
public function testDqlQueryBindDateTimeInstance()
Something went wrong with that request. Please try again.