From af317a107bdb179bce31323d0d28af2c333ebc12 Mon Sep 17 00:00:00 2001 From: Mark Story Date: Mon, 8 Mar 2010 23:07:36 -0500 Subject: [PATCH 1/9] Fixing issues in Set::combine() when data or paths used result in empty datasets. Tests added. Fixes #414 --- cake/libs/set.php | 7 ++++++- cake/tests/cases/libs/set.test.php | 3 +++ 2 files changed, 9 insertions(+), 1 deletion(-) diff --git a/cake/libs/set.php b/cake/libs/set.php index fc921b26a..ed8e19c91 100644 --- a/cake/libs/set.php +++ b/cake/libs/set.php @@ -947,6 +947,9 @@ class Set extends Object { } else { $keys = Set::extract($data, $path1); } + if (empty($keys)) { + return array(); + } if (!empty($path2) && is_array($path2)) { $format = array_shift($path2); @@ -978,7 +981,9 @@ class Set extends Object { return $out; } } - + if (empty($vals)) { + return array(); + } return array_combine($keys, $vals); } /** diff --git a/cake/tests/cases/libs/set.test.php b/cake/tests/cases/libs/set.test.php index a42d0cc9e..6867ae4f1 100644 --- a/cake/tests/cases/libs/set.test.php +++ b/cake/tests/cases/libs/set.test.php @@ -1621,6 +1621,9 @@ class SetTest extends CakeTestCase { $result = Set::combine($b, 'users.{n}.User.id', 'users.{n}.User.non-existant'); $expected = array(2 => null, 14 => null, 25 => null); $this->assertIdentical($result, $expected); + + $result = Set::combine($a, 'fail', 'fail'); + $this->assertEqual($result, array()); } /** * testMapReverse method From 64c627a35241cf9766a035fc02cdd78a908755ef Mon Sep 17 00:00:00 2001 From: Mark Story Date: Wed, 10 Mar 2010 21:46:28 -0500 Subject: [PATCH 2/9] Adding checks to force limit to always be a positive integer. Fixes potential out of bounds type queries with paginate(). Fixes #418 --- cake/libs/controller/controller.php | 7 ++++++- cake/tests/cases/libs/controller/controller.test.php | 8 ++++++++ 2 files changed, 14 insertions(+), 1 deletion(-) diff --git a/cake/libs/controller/controller.php b/cake/libs/controller/controller.php index 16524a986..a7e5a9722 100644 --- a/cake/libs/controller/controller.php +++ b/cake/libs/controller/controller.php @@ -1046,8 +1046,13 @@ class Controller extends Object { $type = $defaults[0]; unset($defaults[0]); } + $options = array_merge(array('page' => 1, 'limit' => 20), $defaults, $options); - $options['limit'] = (empty($options['limit']) || !is_numeric($options['limit'])) ? 1 : $options['limit']; + $options['limit'] = (int) $options['limit']; + if (empty($options['limit']) || $options['limit'] < 1) { + $options['limit'] = 1; + } + extract($options); if (is_array($scope) && !empty($scope)) { diff --git a/cake/tests/cases/libs/controller/controller.test.php b/cake/tests/cases/libs/controller/controller.test.php index 1b06f487a..e65a639fa 100644 --- a/cake/tests/cases/libs/controller/controller.test.php +++ b/cake/tests/cases/libs/controller/controller.test.php @@ -526,6 +526,14 @@ class ControllerTest extends CakeTestCase { $this->assertIdentical($Controller->params['paging']['ControllerPost']['pageCount'], 3); $this->assertIdentical($Controller->params['paging']['ControllerPost']['prevPage'], false); $this->assertIdentical($Controller->params['paging']['ControllerPost']['nextPage'], true); + + $Controller->passedArgs = array(); + $Controller->paginate = array('limit' => '-1'); + $Controller->paginate('ControllerPost'); + $this->assertIdentical($Controller->params['paging']['ControllerPost']['page'], 1); + $this->assertIdentical($Controller->params['paging']['ControllerPost']['pageCount'], 3); + $this->assertIdentical($Controller->params['paging']['ControllerPost']['prevPage'], false); + $this->assertIdentical($Controller->params['paging']['ControllerPost']['nextPage'], true); } /** * testPaginateExtraParams method From 8d58b40642c182afe307d00debf44a1dbd8a3c62 Mon Sep 17 00:00:00 2001 From: Mariano Iglesias Date: Sun, 14 Mar 2010 16:34:57 -0300 Subject: [PATCH 3/9] Fixing issue in Containable where if bindModel was used to add / change a binding not permanently, Containable was making the change permanent --- cake/libs/model/behaviors/containable.php | 6 ++++- .../libs/model/behaviors/containable.test.php | 25 +++++++++++++++++++ 2 files changed, 30 insertions(+), 1 deletion(-) diff --git a/cake/libs/model/behaviors/containable.php b/cake/libs/model/behaviors/containable.php index 7aa02f55e..8ace8760f 100644 --- a/cake/libs/model/behaviors/containable.php +++ b/cake/libs/model/behaviors/containable.php @@ -129,7 +129,11 @@ class ContainableBehavior extends ModelBehavior { if ($contain) { $backupBindings = array(); foreach ($this->types as $relation) { - $backupBindings[$relation] = $instance->{$relation}; + if (!empty($instance->__backAssociation[$relation])) { + $backupBindings[$relation] = $instance->__backAssociation[$relation]; + } else { + $backupBindings[$relation] = $instance->{$relation}; + } } foreach ($this->types as $type) { $unbind = array(); diff --git a/cake/tests/cases/libs/model/behaviors/containable.test.php b/cake/tests/cases/libs/model/behaviors/containable.test.php index ebfa57a54..a2a6fdc71 100644 --- a/cake/tests/cases/libs/model/behaviors/containable.test.php +++ b/cake/tests/cases/libs/model/behaviors/containable.test.php @@ -3311,6 +3311,31 @@ class ContainableBehaviorTest extends CakeTestCase { $this->assertTrue(Set::matches('/Comment[article_id=1]', $result)); $this->Article->resetBindings(); } +/** + * testResetAddedAssociation method + * + * @access public + */ + function testResetAddedAssociation() { + $this->assertTrue(empty($this->Article->hasMany['ArticlesTag'])); + + $this->Article->bindModel(array( + 'hasMany' => array('ArticlesTag') + )); + $this->assertTrue(!empty($this->Article->hasMany['ArticlesTag'])); + + $result = $this->Article->find('first', array( + 'conditions' => array('Article.id' => 1), + 'contain' => array('ArticlesTag') + )); + $expected = array('Article', 'ArticlesTag'); + $this->assertTrue(!empty($result)); + $this->assertEqual('First Article', $result['Article']['title']); + $this->assertTrue(!empty($result['ArticlesTag'])); + $this->assertEqual($expected, array_keys($result)); + + $this->assertTrue(empty($this->Article->hasMany['ArticlesTag'])); + } /** * testResetAssociation method * From 3ab687043e57130cbee76ead4ad7865478da718b Mon Sep 17 00:00:00 2001 From: Mark Story Date: Sun, 14 Mar 2010 19:27:36 -0400 Subject: [PATCH 4/9] Updating DboPostgres test to reflect changes in test suite. --- .../datasources/dbo/dbo_postgres.test.php | 18 +++++++++++++----- 1 file changed, 13 insertions(+), 5 deletions(-) diff --git a/cake/tests/cases/libs/model/datasources/dbo/dbo_postgres.test.php b/cake/tests/cases/libs/model/datasources/dbo/dbo_postgres.test.php index e7d2de630..bfd375d77 100644 --- a/cake/tests/cases/libs/model/datasources/dbo/dbo_postgres.test.php +++ b/cake/tests/cases/libs/model/datasources/dbo/dbo_postgres.test.php @@ -157,7 +157,8 @@ class DboPostgresTest extends CakeTestCase { * @access public */ var $fixtures = array('core.user', 'core.binary_test', 'core.comment', 'core.article', - 'core.tag', 'core.articles_tag', 'core.attachment', 'core.person', 'core.post', 'core.author'); + 'core.tag', 'core.articles_tag', 'core.attachment', 'core.person', 'core.post', 'core.author', + ); /** * Actual DB connection used in testing * @@ -477,16 +478,23 @@ class DboPostgresTest extends CakeTestCase { )'); $model =& ClassRegistry::init('datatypes'); $schema = new CakeSchema(array('connection' => 'test_suite')); - $result = $schema->read(array('connection' => 'test_suite')); - $schema->tables = $result['tables']['missing']; + $result = $schema->read(array( + 'connection' => 'test_suite', + 'models' => array('Datatype') + )); + $schema->tables = array('datatypes' => $result['tables']['datatypes']); $result = $db1->createSchema($schema, 'datatypes'); $this->assertNoPattern('/timestamp DEFAULT/', $result); $this->assertPattern('/timestamp\s*,/', $result); $db1->query('DROP TABLE ' . $db1->fullTableName('datatypes')); + $db1->query($result); - $result2 = $schema->read(array('connection' => 'test_suite')); - $schema->tables = $result2['tables']['missing']; + $result2 = $schema->read(array( + 'connection' => 'test_suite', + 'models' => array('Datatype') + )); + $schema->tables = array('datatypes' => $result2['tables']['datatypes']); $result2 = $db1->createSchema($schema, 'datatypes'); $this->assertEqual($result, $result2); From daf02cad61a0979fe6cf85b0aa4e5f2d68a03c2b Mon Sep 17 00:00:00 2001 From: Mark Story Date: Sun, 14 Mar 2010 23:31:52 -0400 Subject: [PATCH 5/9] Fixing CakeSchema index comparison that was causing failures in postgres tests. --- cake/libs/model/schema.php | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/cake/libs/model/schema.php b/cake/libs/model/schema.php index 30c1a37da..db68169b1 100644 --- a/cake/libs/model/schema.php +++ b/cake/libs/model/schema.php @@ -372,7 +372,7 @@ class CakeSchema extends Object { */ function compare($old, $new = null) { if (empty($new)) { - $new = $this; + $new =& $this; } if (is_array($new)) { if (isset($new['tables'])) { @@ -406,6 +406,7 @@ class CakeSchema extends Object { $tables[$table]['drop'] = $diff; } } + foreach ($fields as $field => $value) { if (isset($old[$table][$field])) { $diff = array_diff_assoc($value, $old[$table][$field]); @@ -427,10 +428,13 @@ class CakeSchema extends Object { if (isset($old[$table]['indexes']) && isset($new[$table]['indexes'])) { $diff = $this->_compareIndexes($new[$table]['indexes'], $old[$table]['indexes']); if ($diff) { - if (isset($tables[$table]['drop']['indexes']) && isset($diff['drop'])) { + if (!isset($tables[$table])) { + $tables[$table] = array(); + } + if (isset($diff['drop'])) { $tables[$table]['drop']['indexes'] = $diff['drop']; } - if (isset($tables[$table]['add']['indexes']) && isset($diff['add'])) { + if ($diff && isset($diff['add'])) { $tables[$table]['add']['indexes'] = $diff['add']; } } From 190066fd51c222de91989aec97b0042d7bcda7c9 Mon Sep 17 00:00:00 2001 From: Mark Story Date: Mon, 15 Mar 2010 22:15:03 -0400 Subject: [PATCH 6/9] Adding array_filter() to remove empty conditions that can be caused by array casting an empty string. --- cake/libs/model/model.php | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/cake/libs/model/model.php b/cake/libs/model/model.php index d76510fc8..c0ad6aaa1 100644 --- a/cake/libs/model/model.php +++ b/cake/libs/model/model.php @@ -1370,10 +1370,10 @@ class Model extends Overloadable { } if ($this->hasAndBelongsToMany[$assoc]['unique']) { - $conditions = array_merge( + $conditions = array_filter(array_merge( array($join . '.' . $this->hasAndBelongsToMany[$assoc]['foreignKey'] => $id), (array)$this->hasAndBelongsToMany[$assoc]['conditions'] - ); + )); $links = $this->{$join}->find('all', array( 'conditions' => $conditions, 'recursive' => empty($this->hasAndBelongsToMany[$assoc]['conditions']) ? -1 : 0, @@ -1397,7 +1397,7 @@ class Model extends Overloadable { } if (!empty($newValues)) { - $fields = implode(',', $fields); + $fields = implode(',', $fields); $db->insertMulti($this->{$join}, $fields, $newValues); } } From ea64588a814b09565b82c428845f7a096814c327 Mon Sep 17 00:00:00 2001 From: Mark Story Date: Mon, 15 Mar 2010 22:55:14 -0400 Subject: [PATCH 7/9] Adding tests from 'Stephen Cuppert' to test incorrectly generate DELETE queries for habtm join tables that do not have a primary key when using PostgreSQL. Updating DboSource::_matchRecords() to only query the table if the supplied conditions are actually multi-table conditions. Fixes #459 --- cake/libs/model/datasources/dbo_source.php | 16 ++++++ .../cases/libs/model/model_delete.test.php | 55 +++++++++++++++++++ 2 files changed, 71 insertions(+) diff --git a/cake/libs/model/datasources/dbo_source.php b/cake/libs/model/datasources/dbo_source.php index 9f087946d..5982e137d 100644 --- a/cake/libs/model/datasources/dbo_source.php +++ b/cake/libs/model/datasources/dbo_source.php @@ -1477,6 +1477,22 @@ class DboSource extends DataSource { } elseif ($conditions === null) { $conditions = $this->conditions($this->defaultConditions($model, $conditions, false), true, true, $model); } else { + $noJoin = true; + foreach ($conditions as $field => $value) { + $originalField = $field; + if (strpos($field, '.') !== false) { + list($alias, $field) = explode('.', $field); + } + if (!$model->hasField($field)) { + $noJoin = false; + break; + } + $conditions[$field] = $value; + unset($conditions[$originalField]); + } + if ($noJoin === true) { + return $this->conditions($conditions); + } $idList = $model->find('all', array( 'fields' => "{$model->alias}.{$model->primaryKey}", 'conditions' => $conditions diff --git a/cake/tests/cases/libs/model/model_delete.test.php b/cake/tests/cases/libs/model/model_delete.test.php index e2d4874f8..d299cc905 100644 --- a/cake/tests/cases/libs/model/model_delete.test.php +++ b/cake/tests/cases/libs/model/model_delete.test.php @@ -580,6 +580,61 @@ class ModelDeleteTest extends BaseModelTest { $exists = $Model->findById(1); $this->assertTrue(is_array($exists)); } +/** + * test for a habtm deletion error that occurs in postgres but should not. + * And should not occur in any dbo. + * + * @return void + */ + function testDeleteHabtmPostgresFailure() { + $this->loadFixtures('Article', 'Tag', 'ArticlesTag'); + + $Article =& ClassRegistry::init('Article'); + $Article->hasAndBelongsToMany['Tag']['unique'] = true; + + $Tag =& ClassRegistry::init('Tag'); + $Tag->bindModel(array('hasAndBelongsToMany' => array( + 'Article' => array( + 'className' => 'Article', + 'unique' => true + ) + )), true); + + // Article 1 should have Tag.1 and Tag.2 + $before = $Article->find("all", array( + "conditions" => array("Article.id" => 1), + )); + $this->assertEqual(count($before[0]['Tag']), 2, 'Tag count for Article.id = 1 is incorrect, should be 2 %s'); + + // From now on, Tag #1 is only associated with Post #1 + $submitted_data = array( + "Tag" => array("id" => 1, 'tag' => 'tag1'), + "Article" => array( + "Article" => array(1) + ) + ); + $Tag->save($submitted_data); + + // One more submission (The other way around) to make sure the reverse save looks good. + $submitted_data = array( + "Article" => array("id" => 2, 'title' => 'second article'), + "Tag" => array( + "Tag" => array(2, 3) + ) + ); + // ERROR: + // Postgresql: DELETE FROM "articles_tags" WHERE tag_id IN ('1', '3') + // MySQL: DELETE `ArticlesTag` FROM `articles_tags` AS `ArticlesTag` WHERE `ArticlesTag`.`article_id` = 2 AND `ArticlesTag`.`tag_id` IN (1, 3) + $Article->save($submitted_data); + + // Want to make sure Article #1 has Tag #1 and Tag #2 still. + $after = $Article->find("all", array( + "conditions" => array("Article.id" => 1), + )); + + // Removing Article #2 from Tag #1 is all that should have happened. + $this->assertEqual(count($before[0]["Tag"]), count($after[0]["Tag"])); + } } ?> \ No newline at end of file From 661fcd32ab56b3c288c922c570ea141461d794ba Mon Sep 17 00:00:00 2001 From: Mark Story Date: Mon, 15 Mar 2010 23:07:18 -0400 Subject: [PATCH 8/9] Fixing failing tests in PostgreSQL cause by invalid datatype comparisons and missing id fields. --- cake/tests/cases/libs/model/model_write.test.php | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/cake/tests/cases/libs/model/model_write.test.php b/cake/tests/cases/libs/model/model_write.test.php index f57dc0eb4..b43b0facf 100644 --- a/cake/tests/cases/libs/model/model_write.test.php +++ b/cake/tests/cases/libs/model/model_write.test.php @@ -322,6 +322,7 @@ class ModelWriteTest extends BaseModelTest { $data = array( 'OverallFavorite' => array( + 'id' => 22, 'model_type' => '8-track', 'model_id' => '3', 'priority' => '1' @@ -383,6 +384,7 @@ class ModelWriteTest extends BaseModelTest { $User = new CounterCacheUser(); $Post = new CounterCachePost(); $data = array('Post' => array( + 'id' => 22, 'title' => 'New Post', 'user_id' => 66 )); @@ -2007,7 +2009,7 @@ class ModelWriteTest extends BaseModelTest { 'DoomedSomethingElse' => array( 'className' => 'SomethingElse', 'joinTable' => 'join_things', - 'conditions' => 'JoinThing.doomed = 1', + 'conditions' => 'JoinThing.doomed = true', 'unique' => true ), 'NotDoomedSomethingElse' => array( From 01a5738f3c7f8553d8203eb565c08beba6e79ca0 Mon Sep 17 00:00:00 2001 From: Mark Story Date: Mon, 15 Mar 2010 23:14:23 -0400 Subject: [PATCH 9/9] Effectively reverting changes made in [190066fd51c222de91989aec97b0042d7bcda7c9] which caused conditions using a falsey values to be removed. --- cake/libs/model/model.php | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/cake/libs/model/model.php b/cake/libs/model/model.php index c0ad6aaa1..184a75fee 100644 --- a/cake/libs/model/model.php +++ b/cake/libs/model/model.php @@ -1370,10 +1370,12 @@ class Model extends Overloadable { } if ($this->hasAndBelongsToMany[$assoc]['unique']) { - $conditions = array_filter(array_merge( - array($join . '.' . $this->hasAndBelongsToMany[$assoc]['foreignKey'] => $id), - (array)$this->hasAndBelongsToMany[$assoc]['conditions'] - )); + $conditions = array( + $join . '.' . $this->hasAndBelongsToMany[$assoc]['foreignKey'] => $id + ); + if (!empty($this->hasAndBelongsToMany[$assoc]['conditions'])) { + $conditions = array_merge($conditions, (array)$this->hasAndBelongsToMany[$assoc]['conditions']); + } $links = $this->{$join}->find('all', array( 'conditions' => $conditions, 'recursive' => empty($this->hasAndBelongsToMany[$assoc]['conditions']) ? -1 : 0,