Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

combine replaces previous conditions added by filterby #450

Merged
merged 2 commits into from

4 participants

@hholzer

No description provided.

@travisbot

This pull request passes (merged 740d8ff into 441f969).

@willdurand
Owner

Heya,

Could you write a unit test to prove both your fix and the bug?

@willdurand
Owner

ping @hholzer

@hholzer

hello,

we sadly don't have any experience with unit tests yet, so it's probably for the best if we simply explain the problem a bit more, so that you can reproduce it by yourself

we have a pretty complex application with a behavior wo filters rows automatically by user credentials

our behavior adds the filter by credential method wich does something like this and it's called in preSelectQuery

$this->condition('user_type', ...);
$this->condition('credential', ...);
$this->condition('user_type2', ...);
$this->combine(array('user_type', 'credential'), 'and', 'user_type_credential');
$this->combine(array('user_type_credential', 'user_type2'), 'or');

now we have the use case that we want to filter the list a bit more wich works for filterByUsername fine ... but not for ->filterByUserType because it would be overridden by the combine.

if we would use first the combine and the filterByXXX after it would work because filterByXXX seems to use Criteria::addAnd while combine uses Criteria::add

kind regards
hholzer

@Tschebel

hello,

i can confirm the issue and i wrote a unit test wich should prove that a filterByXXX before a combine gets overriden instead of added while a filterByXXX after a combine gets correclty added.

The unit test should fail without the first commit of @hholzer and work afterwards

kind regards

@willdurand willdurand merged commit 51983e4 into propelorm:master
@willdurand
Owner

Great, thank you!

@willdurand willdurand referenced this pull request in propelorm/Propel2
Closed

Port PR #450 from Propel 1.6 #311

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Aug 17, 2012
  1. @hholzer
Commits on Oct 9, 2012
  1. @Tschebel
This page is out of date. Refresh to see the latest.
View
2  runtime/lib/query/Criteria.php
@@ -822,7 +822,7 @@ public function combine($criterions = array(), $operator = self::LOGICAL_AND, $n
$firstCriterion->$operatorMethod($criterion);
}
if ($name === null) {
- $this->add($firstCriterion, null, null);
+ $this->addAnd($firstCriterion, null, null);
} else {
$this->addCond($name, $firstCriterion, null, null);
}
View
23 test/testsuite/runtime/query/ModelCriteriaTest.php
@@ -2493,6 +2493,29 @@ public function testExistsWithNonExistentCondition()
$c->where('b.Title = ?', 'jenexistepas');
$this->assertFalse($c->exists());
}
+
+ public function testCombineAndFilterBy()
+ {
+ $params = array();
+ $sql = "SELECT FROM `book` WHERE ((book.TITLE LIKE :p1 OR book.ISBN LIKE :p2) AND book.TITLE LIKE :p3)";
+ $c = BookQuery::create()
+ ->condition('u1', 'book.TITLE LIKE ?', '%test1%')
+ ->condition('u2', 'book.ISBN LIKE ?', '%test2%')
+ ->combine(array('u1', 'u2'), 'or')
+ ->filterByTitle('%test3%');
+ $result = BasePeer::createSelectSql($c, $params);
+ $this->assertEquals($result, $sql);
+
+ $params = array();
+ $sql = "SELECT FROM `book` WHERE (book.TITLE LIKE :p1 AND (book.TITLE LIKE :p2 OR book.ISBN LIKE :p3))";
+ $c = BookQuery::create()
+ ->filterByTitle('%test3%')
+ ->condition('u1', 'book.TITLE LIKE ?', '%test1%')
+ ->condition('u2', 'book.ISBN LIKE ?', '%test2%')
+ ->combine(array('u1', 'u2'), 'or');
+ $result = BasePeer::createSelectSql($c, $params);
+ $this->assertEquals($result, $sql);
+ }
}
class TestableModelCriteria extends ModelCriteria
Something went wrong with that request. Please try again.