Support NULL in EntityRepository's magic findBy and findOneBy methods #110

Merged
merged 3 commits into from Oct 17, 2011

2 participants

@shesek

The magic findBy and findOneBy methods don't support passing NULL as the value, because "we cannot (yet) transform it into IS NULL".

However, BasicEntityPersister::_getSelectConditionSQL() does support that. It seems like leftovers from when there was no support for it. I tried it locally (after applying this change) and it does seem to work well.

@stof stof commented on an outdated diff Aug 25, 2011
lib/Doctrine/ORM/EntityRepository.php
- if ( !isset($arguments[0])) {
- // we dont even want to allow null at this point, because we cannot (yet) transform it into IS NULL.
+ if (count($arguments) === 0) {
@stof
Doctrine member
stof added a line comment Aug 25, 2011

empty($arguments) would probably be better

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 Aug 25, 2011
lib/Doctrine/ORM/EntityRepository.php
@@ -203,9 +203,9 @@ class EntityRepository implements ObjectRepository
"either findBy or findOneBy!"
);
}
+
@stof
Doctrine member
stof added a line comment Aug 25, 2011

you should remove this added line, especially as it contains trailing whitespaces.

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

Changed. For some reason I always thought count($arr) === 0 should be faster, but its the other way around - empty() performs much better! I should probably start egreping around for count\(\$\w+\)\s*===\s*0....

nadav@shesek:~$ time php -r '$arr = array(); for ($i=1; $i<=500000; $i++) { count($arr) === 0; }'
real    0m7.723s
user    0m3.324s
sys 0m2.736s

nadav@shesek:~$ time php -r '$arr = array(); for ($i=1; $i<=500000; $i++) { empty($arr); }'
real    0m0.429s
user    0m0.396s
sys 0m0.024s
@beberlei
Doctrine member

Can you just use the isset() again please and just have this remove the comment? It worked before and this changed line looks very strange to me since its missing the [0].

@beberlei
Doctrine member

Please add a test that verifies you can now select by null. I thought there was one in tests/Doctrine/Tests/ORM/Functional/EntityRepositoryTest.php though.

@shesek

Should I add more assertions to testFindFieldByMagicCall or create a new test method for it?

@beberlei beberlei merged commit 2e389e0 into doctrine:master Oct 17, 2011
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment