Skip to content

Criteria filtering doesn't work with DateTime instance #234

Closed
wants to merge 1 commit into from

5 participants

@ludekstepan

Filtering associations doesn't work with DateTime instance as a comparison value because filtering uses === operator. It works with SQL backed filtering, but not on PHP collection level.

$criteria = Criteria::create()
    ->where(Criteria::expr()->eq("birthday", new \DateTime("1982-02-17")))
    ->orderBy(array("username" => "ASC"))
    ->setFirstResult(0)
    ->setMaxResults(20)
;
@doctrinebot

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/DCOM-152

@Ocramius Ocramius commented on the diff Jan 10, 2013
.../Common/Collections/Expr/ClosureExpressionVisitor.php
@@ -92,6 +92,10 @@ public function walkComparison(Comparison $comparison)
switch ($comparison->getOperator()) {
case Comparison::EQ:
+ return function ($object) use ($field, $value) {
@Ocramius
Doctrine member
Ocramius added a note Jan 10, 2013

This is not a valid object comparison

@ludekstepan
ludekstepan added a note Jan 12, 2013

That's right, this is a loose equality comparison, not a strict identity comparison.

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

@ludekstepan comparing object instances doesn't work like that. An instance is equal to the other only if they reference the same object, otherwise a closure that handles the comparison is needed.

@ludekstepan

@Ocramius patch above implements loose comparison, see difference between Comparison::EQ and Comparison::IS

Consider following:

$a = new DateTime('2013-01-01');
$b = new DateTime('2013-01-01');

var_dump($a === $b);
var_dump($a == $b);
@Ocramius
Doctrine member

@ludekstepan those two objects are not the same though.

@ludekstepan

@Ocramius that's what Comparison::EQ is for

@Ocramius
Doctrine member

@ludekstepan and then comes:

class A 
{
    public function __construct()
    {
        $this->b = $this;
    }
}

to ruin the party (any circular reference will do)

@ludekstepan

@Ocramius please see the use case in the description of pull request and review patched source code, not the patch only. You are more than welcome to implement a better solution.

@beberlei
Doctrine member

This is tricky, because sometimes you want to compare objects by values sometimes by reference and php offers no way to decide here. We could introduce a DateTime special case, but this seems dangerous.

@Ocramius
Doctrine member

@beberlei what about (eventually) using "strict" as of http://php.net/manual/en/function.in-array.php ?

@ludekstepan I'm fine with this if you can get a test passing with the example I've pasted as a test asset to compare. You will also need to add corresponding tests to the test suite anyway before this is (eventually) merged

@ludekstepan

@beberlei Selecting the right comparison strategy is up to the programmer. I'm not proposing any special case.

The source code already provides sufficient options:

switch ($comparison->getOperator()) {
  case Comparison::EQ:
    return function ($object) use ($field, $value) {
      return ClosureExpressionVisitor::getObjectFieldValue($object, $field) == $value;
    };
  case Comparison::IS:
    return function ($object) use ($field, $value) {
      return ClosureExpressionVisitor::getObjectFieldValue($object, $field) === $value;
    };

A programmer should use Comparison::EQ for equality comparison, like:

$criteria = Criteria::create()
    ->where(Criteria::expr()->eq("birthday", new \DateTime("1982-02-17")))
;
// or
$criteria = Criteria::create()
    ->where(Criteria::expr()->eq("bar", false)) // handles all false values, including null
;

Whereas Comparison::IS should be used for identity comparison, like:

$criteria = Criteria::create()
    ->where(Criteria::expr()->is("parent", $entityObject))
;
// or
$criteria = Criteria::create()
    ->where(Criteria::expr()->is("bar", null) // same as isNull("bar")
;

@Ocramius You are free to implement the is() method and write tests. Have a look at ExpressionBuilder::isNull() for better understanding.

@Ocramius
Doctrine member

@ludekstepan well, then NEQ should probably be handled too (need a comparison operator also for a strict version of it) and I'm not really sure about IN and NIN

@ludekstepan

@Ocramius good point! :-) I missed that one.

@texdc
texdc commented Apr 30, 2013

If I may jump in with a suggestion:

case Comparison::NEQ:
    return function ($object) use ($field, $value) {
        return ClosureExpressionVisitor::getObjectFieldValue($object, $field) != $value;
    };

case Comparison::NIS:
    return function ($object) use ($field, $value) {
        return ClosureExpressionVisitor::getObjectFieldValue($object, $field) !== $value;
    };
// ...
case Comparison::IN:
    return function ($object) use ($field, $value) {
        return in_array(ClosureExpressionVisitor::getObjectFieldValue($object, $field), $value);
    };

case Comparison::INS:
    return function ($object) use ($field, $value) {
        return in_array(ClosureExpressionVisitor::getObjectFieldValue($object, $field), $value, true);
    };

case Comparison::NIN:
    return function ($object) use ($field, $value) {
        return ! in_array(ClosureExpressionVisitor::getObjectFieldValue($object, $field), $value);
    };

case Comparison::NINS:
    return function ($object) use ($field, $value) {
        return ! in_array(ClosureExpressionVisitor::getObjectFieldValue($object, $field), $value, true);
    };

Obviously, simply appending S to each type is not explicit. However, I've seen _STRICT used before, but it's a bit verbose.

@Ocramius
Doctrine member

@texdc is there actually any advantage in having all the negated comparisons? I was wondering if we actually need all that...

@texdc
texdc commented Apr 30, 2013

Yes, and no. I considered use-cases and would guess the strict versions would see very little use, comparatively. But, it would be nice to have the option. If they're not added, should we just drop the strict comparisons altogether?

@Ocramius
Doctrine member

I was talking about negated comparisons ;)

@texdc
texdc commented Apr 30, 2013

Gotcha. I don't think I could answer that. I didn't start using Doctrine until a few months ago. @beberlei care to comment?

@beberlei
Doctrine member
beberlei commented Feb 3, 2014

This is a no fix. The possibilities are endless here, we need to document that comparisons have to be in scalar values only, otherwise it will not work. We cannot offer another solution here, because you can have ANY object type using custom types and no way to compare it.

@beberlei beberlei closed this Feb 3, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.