Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

DDC-2003 - DateTime conditions in Criteria objects result in errors #433

Merged
merged 5 commits into from Sep 5, 2012

Conversation

Projects
None yet
4 participants
Contributor

Josiah commented Aug 31, 2012

No description provided.

This pull request fails (merged 783c53d into bc2476f).

@Josiah Josiah Fixed DDC-2003 using closures to reference the functionality of the c…
…alling entity persister from the SQL value visitor.
c7f5d9d

This pull request fails (merged c7f5d9d into bc2476f).

This pull request fails (merged a6b6b25 into bc2476f).

Contributor

Josiah commented Aug 31, 2012

@beberlei it looks like implementing this using closures isn't going to work in PHP 5.3 as the methods needed are private in the BasicEntityPersister.

How would you like me to resolve this?

  1. Inject the persister in to the SqlValueVisitor and make the required methods public,
  2. Duplicate the functionality for these methods present in the BasicEntityPersister to the SqlValueVisitor, or
  3. Extract the functionality into a new helper class, then inject that into the value visitor.

@stof stof commented on an outdated diff Aug 31, 2012

...Tests/ORM/Functional/EntityRepositoryCriteriaTest.php
@@ -0,0 +1,71 @@
+<?php
+
+namespace Doctrine\Tests\ORM\Functional;
+
+use Doctrine\Tests\Models\Generic\DateTimeModel;
+use Doctrine\Common\Collections\Criteria;
+
+require_once __DIR__ . '/../../TestInit.php';
@stof

stof Aug 31, 2012

Member

this line is useless since the refactoring of the phpunit setup months ago so you should remove it

@stof stof commented on the diff Aug 31, 2012

...Tests/ORM/Functional/EntityRepositoryCriteriaTest.php
@@ -0,0 +1,71 @@
+<?php
+
@stof

stof Aug 31, 2012

Member

the license header is missing

@stof stof commented on an outdated diff Aug 31, 2012

...Tests/ORM/Functional/EntityRepositoryCriteriaTest.php
@@ -0,0 +1,71 @@
+<?php
+
+namespace Doctrine\Tests\ORM\Functional;
+
+use Doctrine\Tests\Models\Generic\DateTimeModel;
+use Doctrine\Common\Collections\Criteria;
+
+require_once __DIR__ . '/../../TestInit.php';
+
+/**
+ * @author Josiah <josiah@jjs.id.au>
+ */
+class EntityRepositoryCriteriaTest extends \Doctrine\Tests\OrmFunctionalTestCase
+{
+ protected function setUp() {
@stof

stof Aug 31, 2012

Member

the curly brace should be on its own line

This pull request fails (merged e0d1633 into bc2476f).

@stof stof and 1 other commented on an outdated diff Aug 31, 2012

lib/Doctrine/ORM/Persisters/SqlValueVisitor.php
$value = $comparison->getValue()->getValue();
$field = $comparison->getField();
-
- $this->values[] = $value;
- $this->types[] = $this->getType($field, $value);
+
+ $this->values[] = $valueCallback($value);
@stof

stof Aug 31, 2012

Member

this syntax will only work with closures in 5.3, not with any callable. but your constructor does not mandates using closures

@Josiah

Josiah Aug 31, 2012

Contributor

You've got a good point there, however I won't be able to use closures at all because PHP 5.3 doesn't support executing in the defining classes scope. You'll notice that Travis bot is failing on PHP 5.3 because of this.

Contributor

Josiah commented Aug 31, 2012

@stof do you have any input as to how the SqlValueVistor should be refactored as per my comment

This pull request fails (merged 959c4f0 into bc2476f).

Contributor

Josiah commented Sep 4, 2012

For some reason the @travisbot has the wrong version, the line of code it is referring to does not exist in commit 959c4f0

Member

stof commented Sep 4, 2012

@Josiah Travis is not running the tests on your code, but on your code merged into master

@stof stof commented on the diff Sep 4, 2012

lib/Doctrine/ORM/Persisters/BasicEntityPersister.php
@@ -808,10 +808,24 @@ private function expandCriteriaParameters(Criteria $criteria)
return array(array(), array());
}
- $valueVisitor = new SqlValueVisitor($this->_class);
+ $persister = $this;
@stof

stof Sep 4, 2012

Member

this variable seems unused

@beberlei beberlei merged commit 959c4f0 into doctrine:master Sep 5, 2012

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