Expose `EntityPersister::count()` through `EntityRepository::count()` #6003

phansys · 2016-09-02T18:44:39Z

Q	A
Branch	master
Bug fix?	no
New feature?	yes
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets
License	MIT
Doc PR

TODO:

Add tests.
Add documentation.

Ocramius · 2016-09-07T20:19:21Z

I personally don't like overloading the repositories with even more functionality, but this might actually be useful.

Thoughts, @guilhermeblanco @beberlei @deeky666 @kimhemsoe?

Ocramius · 2016-09-07T20:19:55Z

docs/en/reference/working-with-objects.rst

@@ -41,7 +41,7 @@ headline "Hello World" with the ID 1234:
 <?php
 $article = $entityManager->find('CMS\Article', 1234);
 $article->setHeadline('Hello World dude!');
- 
+


Please don't trim whitespace: it is in place due to RST here.

Ok @Ocramius, I'll revert these changes.

carnage · 2016-09-07T21:12:19Z

I would probably have assumed this feature already existed if asked. From that perspective I think it makes for a good addition.

Perhaps use a rebase --interactive to clean up the change then revert of whitespace changes to the docs.

phansys · 2016-09-07T21:13:41Z

Sure @carnage, when the PR is ready to merge I'll perform a rebase.

mnapoli · 2016-09-07T21:24:36Z

👍 that would avoid having to write a DQL query for a simple count (unless I'm completely wrong?)

phansys · 2016-09-07T21:26:33Z

Yes @mnapoli, this is the intended goal.

phansys · 2016-09-07T21:46:16Z

lib/Doctrine/ORM/EntityRepository.php

+ case 4:
+ return $this->$method(array($fieldName => $arguments[0]), $arguments[1], $arguments[2], $arguments[3]);
+
+ default:


Do you know if this statement is here intentionally (just for the comment) or if this can be removed?

This is a bug.

asgrim · 2016-09-07T22:00:09Z

LGTM 👍 (would use a +1 thing but I'm on mobile, sorry)

Ocramius · 2016-09-07T22:24:37Z

lib/Doctrine/ORM/EntityRepository.php

+ case 3:
+ return $this->$method(array($fieldName => $arguments[0]), $arguments[1], $arguments[2]);
+
+ case 4:


If we support only PHP 5.6+, then we can simply do return $this->$method([$fieldName, $arguments[0]), ...array_slice($arguments, 1))

And yep, we only support 5.6+

I'll apply this on merge: merging and moving forward from master myself :-)

Should I squash now then?

Nope: already checked out locally and took ownership of the branch. Relax, sit back and have a 🍺

Ocramius · 2016-09-07T22:25:56Z

lib/Doctrine/ORM/EntityRepository.php

+
+ $fieldName = lcfirst(\Doctrine\Common\Util\Inflector::classify($by));
+
+ if ($this->_class->hasField($fieldName) || $this->_class->hasAssociation($fieldName)) {


Should throw an exception when this is not true: move the exception throwing up :-)

Ocramius · 2016-09-07T22:57:09Z

@phansys merged, thanks!

…tion

teohhanhui · 2017-02-17T09:55:56Z

@phansys @Ocramius The documentation needs to state New in version 2.6.

I was surprised when I saw it, because I remember there was no such function. 😂

Ocramius · 2017-02-17T12:05:57Z

@teohhanhui sorry, our docs are indeed terribly versioned, as it stands (website publishes current master). Please consider helping us in fixing that at https://github.com/doctrine/doctrine-website-sphinx

Feature added in doctrine#6003

teohhanhui · 2017-02-17T15:28:37Z

@Ocramius Is #6294 going to render fine?

vudaltsov · 2017-12-21T19:54:11Z

This is such an unexpected BC-break for those who implemented this method on their own like I did...

Majkl578 · 2017-12-21T20:07:20Z

Adding new public method is not a BC break per se. BC break is an unexpected change of behavior or existing API. Even Symfony, known for its BC promise, won't give you guarantee when it comes to extending a class and adding new methods: https://symfony.com/doc/current/contributing/code/bc.html

vudaltsov · 2017-12-21T20:11:09Z

@Majkl578 , okay. But extending EntityRepository is a really popular practice. Naming method count() also makes a lot of sense. See above, there are already two PRs fixing this and I am sure, we'll see more tomorrow.

Ocramius · 2017-12-22T20:56:05Z

I don't see how this is a BC break: yes, it raises a warning if you subclassed and had different signature, but that makes it easy to migrate whilst maintaining it working.

And yes, that basically clarifies my initial doubts: no more EntityRepository features ever.

phansys force-pushed the entity_repo_count branch 4 times, most recently from 584dc0c to 3a41091 Compare September 4, 2016 20:06

Expose EntityPersister::count() through EntityRepository::count()

f384bb4

phansys force-pushed the entity_repo_count branch from 3a41091 to f384bb4 Compare September 5, 2016 14:39

Ocramius added the New Feature label Sep 7, 2016

Ocramius reviewed Sep 7, 2016
View reviewed changes

Updated docs

1a1f4cf

Swap logic from EntityRespository::__call()

69e4abe

phansys reviewed Sep 7, 2016
View reviewed changes

Ocramius reviewed Sep 7, 2016
View reviewed changes

Remove default clause at EntityRepository::resolveMagicCall()

fe0b405

Ocramius reviewed Sep 7, 2016
View reviewed changes

Ocramius self-assigned this Sep 7, 2016

Ocramius added this to the 2.6.0 milestone Sep 7, 2016

Ocramius added a commit that referenced this pull request Sep 7, 2016

#6003 removed useless method parameter count checking duplication

6f79a37

Ocramius added a commit that referenced this pull request Sep 7, 2016

#6003 no default parameter needed

5e51a98

Ocramius added a commit that referenced this pull request Sep 7, 2016

#6003 removed useless count() call

de4c854

Ocramius added a commit that referenced this pull request Sep 7, 2016

#6003 imported used symbol

7bf4a65

Ocramius added a commit that referenced this pull request Sep 7, 2016

#6003 corrected return type definition

e2cba87

Ocramius added a commit that referenced this pull request Sep 7, 2016

#6003 inlined persister retrieval

36e9904

Ocramius added a commit that referenced this pull request Sep 7, 2016

#6003 clarifying thrown exception

c5c56a9

Ocramius closed this in 8a87fa2 Sep 7, 2016

phansys deleted the entity_repo_count branch September 8, 2016 13:15

phansys mentioned this pull request Sep 8, 2016

Update exception message at EntityRepository::__call() #6010

Merged

Ocramius mentioned this pull request Oct 25, 2016

Add getCount(), getCountBy($criteria), and getCountBy<Field>($fieldValue) methods to Doctrine\ORM\EntityRepository #6098

Closed

yvoyer pushed a commit to yvoyer/doctrine2 that referenced this pull request Nov 21, 2016

doctrine#6003 removed useless method parameter count checking duplica…

13894fb

…tion

yvoyer pushed a commit to yvoyer/doctrine2 that referenced this pull request Nov 21, 2016

doctrine#6003 no default parameter needed

9f2cd45

yvoyer pushed a commit to yvoyer/doctrine2 that referenced this pull request Nov 21, 2016

doctrine#6003 removed useless count() call

ae115f5

yvoyer pushed a commit to yvoyer/doctrine2 that referenced this pull request Nov 21, 2016

doctrine#6003 imported used symbol

46f4aaa

yvoyer pushed a commit to yvoyer/doctrine2 that referenced this pull request Nov 21, 2016

doctrine#6003 corrected return type definition

5ad4fc7

yvoyer pushed a commit to yvoyer/doctrine2 that referenced this pull request Nov 21, 2016

doctrine#6003 inlined persister retrieval

327df84

yvoyer pushed a commit to yvoyer/doctrine2 that referenced this pull request Nov 21, 2016

doctrine#6003 clarifying thrown exception

b445a72

teohhanhui added a commit to teohhanhui/doctrine2 that referenced this pull request Feb 17, 2017

Add versionadded note for EntityRepository::count

47b5034

Feature added in doctrine#6003

teohhanhui mentioned this pull request Feb 17, 2017

Add versionadded note for EntityRepository::count #6294

Closed

mbabker mentioned this pull request Sep 10, 2017

Incompatible CustomerRepository count() method declaration Sylius/Sylius#8566

Closed

danrot mentioned this pull request Dec 20, 2017

fix BC break introduced in doctrine 2.6 sulu/sulu#3694

Closed

Majkl578 mentioned this pull request Dec 21, 2017

Better document addition of EntityRepository::count() #6933

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Expose `EntityPersister::count()` through `EntityRepository::count()` #6003

Expose `EntityPersister::count()` through `EntityRepository::count()` #6003

phansys commented Sep 2, 2016 •

edited

Loading

Ocramius commented Sep 7, 2016

Ocramius Sep 7, 2016

phansys Sep 7, 2016

carnage commented Sep 7, 2016

phansys commented Sep 7, 2016

mnapoli commented Sep 7, 2016

phansys commented Sep 7, 2016

phansys Sep 7, 2016

Ocramius Sep 7, 2016

phansys Sep 7, 2016

asgrim commented Sep 7, 2016

Ocramius Sep 7, 2016

Ocramius Sep 7, 2016

Ocramius Sep 7, 2016

phansys Sep 7, 2016

Ocramius Sep 7, 2016

Ocramius Sep 7, 2016

Ocramius commented Sep 7, 2016

teohhanhui commented Feb 17, 2017

Ocramius commented Feb 17, 2017

teohhanhui commented Feb 17, 2017

vudaltsov commented Dec 21, 2017

Majkl578 commented Dec 21, 2017

vudaltsov commented Dec 21, 2017

Ocramius commented Dec 22, 2017


		$fieldName = lcfirst(\Doctrine\Common\Util\Inflector::classify($by));

		if ($this->_class->hasField($fieldName) \|\| $this->_class->hasAssociation($fieldName)) {

Expose EntityPersister::count() through EntityRepository::count() #6003

Expose EntityPersister::count() through EntityRepository::count() #6003

Conversation

phansys commented Sep 2, 2016 • edited Loading

Ocramius commented Sep 7, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carnage commented Sep 7, 2016

phansys commented Sep 7, 2016

mnapoli commented Sep 7, 2016

phansys commented Sep 7, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

asgrim commented Sep 7, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ocramius commented Sep 7, 2016

teohhanhui commented Feb 17, 2017

Ocramius commented Feb 17, 2017

teohhanhui commented Feb 17, 2017

vudaltsov commented Dec 21, 2017

Majkl578 commented Dec 21, 2017

vudaltsov commented Dec 21, 2017

Ocramius commented Dec 22, 2017

Expose `EntityPersister::count()` through `EntityRepository::count()` #6003

Expose `EntityPersister::count()` through `EntityRepository::count()` #6003

phansys commented Sep 2, 2016 •

edited

Loading