From 03c2a8b72230c1cfb976cb9ef00eedd1744408f0 Mon Sep 17 00:00:00 2001 From: mark_story Date: Fri, 11 Jul 2014 23:10:16 -0400 Subject: [PATCH 1/5] Unify datetime column default values between MySQL and Postgres. Datetime columns should have 'default' => null, in both Postgres and MySQL. Fixes #3837 --- .../Model/Datasource/Database/Postgres.php | 3 ++ .../Datasource/Database/PostgresTest.php | 50 ++++++++++++++++++- 2 files changed, 51 insertions(+), 2 deletions(-) diff --git a/lib/Cake/Model/Datasource/Database/Postgres.php b/lib/Cake/Model/Datasource/Database/Postgres.php index aa55b7662..a9225349d 100644 --- a/lib/Cake/Model/Datasource/Database/Postgres.php +++ b/lib/Cake/Model/Datasource/Database/Postgres.php @@ -261,6 +261,9 @@ class Postgres extends DboSource { $this->_sequenceMap[$table][$c->name] = $sequenceName; } } + if ($fields[$c->name]['type'] === 'timestamp' && $fields[$c->name]['default'] === '') { + $fields[$c->name]['default'] = null; + } if ($fields[$c->name]['type'] === 'boolean' && !empty($fields[$c->name]['default'])) { $fields[$c->name]['default'] = constant($fields[$c->name]['default']); } diff --git a/lib/Cake/Test/Case/Model/Datasource/Database/PostgresTest.php b/lib/Cake/Test/Case/Model/Datasource/Database/PostgresTest.php index 2cbca6eb5..0c4de769e 100644 --- a/lib/Cake/Test/Case/Model/Datasource/Database/PostgresTest.php +++ b/lib/Cake/Test/Case/Model/Datasource/Database/PostgresTest.php @@ -162,8 +162,8 @@ class PostgresClientTestModel extends Model { 'id' => array('type' => 'integer', 'null' => '', 'default' => '', 'length' => '8', 'key' => 'primary'), 'name' => array('type' => 'string', 'null' => '', 'default' => '', 'length' => '255'), 'email' => array('type' => 'string', 'null' => '1', 'default' => '', 'length' => '155'), - 'created' => array('type' => 'datetime', 'null' => '1', 'default' => '', 'length' => ''), - 'updated' => array('type' => 'datetime', 'null' => '1', 'default' => '', 'length' => null) + 'created' => array('type' => 'datetime', 'null' => true, 'default' => null, 'length' => ''), + 'updated' => array('type' => 'datetime', 'null' => true, 'default' => null, 'length' => null) ); } @@ -551,6 +551,7 @@ class PostgresTest extends CakeTestCase { 'connection' => 'test', 'models' => array('DatatypeTest') )); + $schema->tables = array( 'datatype_tests' => $result['tables']['missing']['datatype_tests'] ); @@ -1098,4 +1099,49 @@ class PostgresTest extends CakeTestCase { $this->assertNotContains($scientificNotation, $result); } +/** + * Test describe() behavior for timestamp columns. + * + * @return void + */ + public function testDescribeTimestamp() { + $this->loadFixtures('User'); + $model = ClassRegistry::init('User'); + $result = $this->Dbo->describe($model); + $expected = array( + 'id' => array( + 'type' => 'integer', + 'null' => false, + 'default' => null, + 'length' => 11, + 'key' => 'primary' + ), + 'user' => array( + 'type' => 'string', + 'null' => true, + 'default' => null, + 'length' => 255 + ), + 'password' => array( + 'type' => 'string', + 'null' => true, + 'default' => null, + 'length' => 255 + ), + 'created' => array( + 'type' => 'datetime', + 'null' => true, + 'default' => null, + 'length' => null + ), + 'updated' => array( + 'type' => 'datetime', + 'null' => true, + 'default' => null, + 'length' => null + ) + ); + $this->assertEquals($expected, $result); + } + } From ace30fdd8ad061545c4b004a0d6e39dd0a534ce7 Mon Sep 17 00:00:00 2001 From: chinpei215 Date: Sat, 12 Jul 2014 11:56:36 +0900 Subject: [PATCH 2/5] Fix a race condition problem Prevents Model::save() from generating a query with WHERE 1 = 1 on race condition. Refs #3857 --- lib/Cake/Model/Datasource/DboSource.php | 2 +- lib/Cake/Model/Model.php | 18 ++++++- .../Case/Model/Datasource/DboSourceTest.php | 41 ++++++++++++++++ lib/Cake/Test/Case/Model/ModelWriteTest.php | 49 +++++++++++++++++++ 4 files changed, 108 insertions(+), 2 deletions(-) diff --git a/lib/Cake/Model/Datasource/DboSource.php b/lib/Cake/Model/Datasource/DboSource.php index f76718073..0bd7693a4 100644 --- a/lib/Cake/Model/Datasource/DboSource.php +++ b/lib/Cake/Model/Datasource/DboSource.php @@ -2399,7 +2399,7 @@ class DboSource extends DataSource { return $conditions; } $exists = $Model->exists(); - if (!$exists && $conditions !== null) { + if (!$exists && ($conditions !== null || !empty($Model->__safeUpdateMode))) { return false; } elseif (!$exists) { return null; diff --git a/lib/Cake/Model/Model.php b/lib/Cake/Model/Model.php index 5207f406e..d654fcf2b 100644 --- a/lib/Cake/Model/Model.php +++ b/lib/Cake/Model/Model.php @@ -593,6 +593,14 @@ class Model extends Object implements CakeEventListener { */ public $__backContainableAssociation = array(); +/** + * Safe update mode + * If true, this prevents Model::save() from generating a query with WHERE 1 = 1 on race condition. + * + * @var bool + */ + public $__safeUpdateMode = false; + // @codingStandardsIgnoreEnd /** @@ -1686,6 +1694,7 @@ class Model extends Object implements CakeEventListener { * * @param array $fieldList List of fields to allow to be saved * @return mixed On success Model::$data if its not empty or true, false on failure + * @throws PDOException * @link http://book.cakephp.org/2.0/en/models/saving-your-data.html */ public function save($data = null, $validate = true, $fieldList = array()) { @@ -1820,7 +1829,14 @@ class Model extends Object implements CakeEventListener { $cache = $this->_prepareUpdateFields(array_combine($fields, $values)); if (!empty($this->id)) { - $success = (bool)$db->update($this, $fields, $values); + $this->__safeUpdateMode = true; + try { + $success = (bool)$db->update($this, $fields, $values); + } catch (Exception $e) { + $this->__safeUpdateMode = false; + throw $e; + } + $this->__safeUpdateMode = false; } else { if (empty($this->data[$this->alias][$this->primaryKey]) && $this->_isUUIDField($this->primaryKey)) { if (array_key_exists($this->primaryKey, $this->data[$this->alias])) { diff --git a/lib/Cake/Test/Case/Model/Datasource/DboSourceTest.php b/lib/Cake/Test/Case/Model/Datasource/DboSourceTest.php index ea7349c33..6d1bde508 100644 --- a/lib/Cake/Test/Case/Model/Datasource/DboSourceTest.php +++ b/lib/Cake/Test/Case/Model/Datasource/DboSourceTest.php @@ -1412,4 +1412,45 @@ class DboSourceTest extends CakeTestCase { $result = $db->insertMulti('articles', array_keys($data[0]), $data); $this->assertTrue($result, 'Data was saved'); } + +/** + * Test defaultConditions() + * + * @return + */ + public function testDefaultConditions() { + $this->loadFixtures('Article'); + $Article = ClassRegistry::init('Article'); + $db = $Article->getDataSource(); + + // Creates a default set of conditions from the model if $conditions is null/empty. + $Article->id = 1; + $result = $db->defaultConditions($Article, null); + $this->assertEquals(array('Article.id' => 1), $result); + + // $useAlias == false + $Article->id = 1; + $result = $db->defaultConditions($Article, null, false); + $this->assertEquals(array($db->fullTableName($Article, false) . '.id' => 1), $result); + + // If conditions are supplied then they will be returned. + $Article->id = 1; + $result = $db->defaultConditions($Article, array('Article.title' => 'First article')); + $this->assertEquals(array('Article.title' => 'First article'), $result); + + // If a model doesn't exist and no conditions were provided either null or false will be returned based on what was input. + $Article->id = 1000000; + $result = $db->defaultConditions($Article, null); + $this->assertNull($result); + + $Article->id = 1000000; + $result = $db->defaultConditions($Article, false); + $this->assertFalse($result); + + // Safe update mode + $Article->id = 1000000; + $Article->__safeUpdateMode = true; + $result = $db->defaultConditions($Article, null); + $this->assertFalse($result); + } } diff --git a/lib/Cake/Test/Case/Model/ModelWriteTest.php b/lib/Cake/Test/Case/Model/ModelWriteTest.php index 6acfd3c51..d36f0b01c 100644 --- a/lib/Cake/Test/Case/Model/ModelWriteTest.php +++ b/lib/Cake/Test/Case/Model/ModelWriteTest.php @@ -7261,4 +7261,53 @@ class ModelWriteTest extends BaseModelTest { $this->assertFalse(isset($model->data['Bid']['name'])); $this->assertFalse(isset($model->data['Bid']['message_id'])); } + +/** + * Test that Model::save() doesn't generate a query with WHERE 1 = 1 on race condition. + * + * @link https://github.com/cakephp/cakephp/issues/3857 + * @return + */ + public function testSafeUpdateMode() { + $this->loadFixtures('User'); + + $User = ClassRegistry::init('User'); + $this->assertFalse($User->__safeUpdateMode); + + $User->getEventManager()->attach(array($this, 'deleteMe'), 'Model.beforeSave'); + + $User->id = 1; + $User->set(array('user' => 'nobody')); + $User->save(); + + $users = $User->find('list', array('fields' => 'User.user')); + + $expected = array( + 2 => 'nate', + 3 => 'larry', + 4 => 'garrett', + ); + $this->assertEquals($expected, $users); + $this->assertFalse($User->__safeUpdateMode); + + $User->id = 2; + $User->set(array('user' => $User->getDataSource()->expression('PDO_EXCEPTION()'))); + try { + $User->save(null, false); + $this->fail('No exception thrown'); + } catch (PDOException $e) { + $this->assertFalse($User->__safeUpdateMode); + } + } + +/** + * Emulates race condition + * + * @param $event + * @return + */ + public function deleteMe($event) { + $Model = $event->subject; + $Model->getDataSource()->delete($Model, array($Model->alias . '.' . $Model->primaryKey => $Model->id)); + } } From ca93bbcd151d0779fe640fb294f3adb0b57acbc9 Mon Sep 17 00:00:00 2001 From: chinpei215 Date: Mon, 14 Jul 2014 01:21:09 +0900 Subject: [PATCH 3/5] Fix CS --- lib/Cake/Test/Case/Model/Datasource/DboSourceTest.php | 5 ++++- lib/Cake/Test/Case/Model/ModelWriteTest.php | 11 ++++++++--- 2 files changed, 12 insertions(+), 4 deletions(-) diff --git a/lib/Cake/Test/Case/Model/Datasource/DboSourceTest.php b/lib/Cake/Test/Case/Model/Datasource/DboSourceTest.php index 6d1bde508..731b309ec 100644 --- a/lib/Cake/Test/Case/Model/Datasource/DboSourceTest.php +++ b/lib/Cake/Test/Case/Model/Datasource/DboSourceTest.php @@ -33,6 +33,9 @@ require_once dirname(dirname(__FILE__)) . DS . 'models.php'; */ class MockPDO extends PDO { +/** + * Constructor. + */ public function __construct() { } @@ -1416,7 +1419,7 @@ class DboSourceTest extends CakeTestCase { /** * Test defaultConditions() * - * @return + * @return void */ public function testDefaultConditions() { $this->loadFixtures('Article'); diff --git a/lib/Cake/Test/Case/Model/ModelWriteTest.php b/lib/Cake/Test/Case/Model/ModelWriteTest.php index d36f0b01c..875fdd19b 100644 --- a/lib/Cake/Test/Case/Model/ModelWriteTest.php +++ b/lib/Cake/Test/Case/Model/ModelWriteTest.php @@ -6371,6 +6371,11 @@ class ModelWriteTest extends BaseModelTest { $this->assertEquals(array(6, 4, 5, 2), $result); } +/** + * testToggleBoolFields method + * + * @return void + */ public function testToggleBoolFields() { $this->loadFixtures('CounterCacheUser', 'CounterCachePost'); $Post = new CounterCachePost(); @@ -7266,7 +7271,7 @@ class ModelWriteTest extends BaseModelTest { * Test that Model::save() doesn't generate a query with WHERE 1 = 1 on race condition. * * @link https://github.com/cakephp/cakephp/issues/3857 - * @return + * @return void */ public function testSafeUpdateMode() { $this->loadFixtures('User'); @@ -7303,8 +7308,8 @@ class ModelWriteTest extends BaseModelTest { /** * Emulates race condition * - * @param $event - * @return + * @param CakeEvent $event containing the Model + * @return void */ public function deleteMe($event) { $Model = $event->subject; From 7b4c5236cfc88e9b999b6727074db621e502bb4a Mon Sep 17 00:00:00 2001 From: David Yell Date: Mon, 14 Jul 2014 09:37:50 +0100 Subject: [PATCH 4/5] Update CONTRIBUTING.md Added some helpful links. Updated PHPUnit version requirement. --- CONTRIBUTING.md | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 211d68744..85f793b6b 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -33,14 +33,14 @@ chance of keeping on top of things. * Core test cases should continue to pass. You can run tests locally or enable [travis-ci](https://travis-ci.org/) for your fork, so all tests and codesniffs will be executed. -* Your work should apply the CakePHP coding standards. +* Your work should apply the [CakePHP coding standards](http://book.cakephp.org/2.0/en/contributing/cakephp-coding-conventions.html). ## Which branch to base the work * Bugfix branches will be based on master. * New features that are backwards compatible will be based on next minor release branch. -* New features or other non-BC changes will go in the next major release branch. +* New features or other non backwards compatible changes will go in the next major release branch. ## Submitting Changes @@ -51,7 +51,7 @@ chance of keeping on top of things. ## Test cases and codesniffer CakePHP tests requires [PHPUnit](http://www.phpunit.de/manual/current/en/installation.html) -3.5 or higher. To run the test cases locally use the following command: +3.7, version 4 is not compatible. To run the test cases locally use the following command: ./lib/Cake/Console/cake test core AllTests --stderr @@ -60,7 +60,7 @@ To run the sniffs for CakePHP coding standards: phpcs -p --extensions=php --standard=CakePHP ./lib/Cake Check the [cakephp-codesniffer](https://github.com/cakephp/cakephp-codesniffer) -repository to setup the CakePHP standard. The README contains installation info +repository to setup the CakePHP standard. The [README](https://github.com/cakephp/cakephp-codesniffer/blob/master/README.mdown) contains installation info for the sniff and phpcs. # Additional Resources From 8d58d936360451c96242a1c5bf9803ece769d64c Mon Sep 17 00:00:00 2001 From: Carl Sutton Date: Tue, 15 Jul 2014 13:21:48 +0100 Subject: [PATCH 5/5] Make the error message better for fixture errors The stack trace has no details about which fixture is the actual problem. --- lib/Cake/TestSuite/Fixture/CakeTestFixture.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/Cake/TestSuite/Fixture/CakeTestFixture.php b/lib/Cake/TestSuite/Fixture/CakeTestFixture.php index c966befab..329eef27f 100644 --- a/lib/Cake/TestSuite/Fixture/CakeTestFixture.php +++ b/lib/Cake/TestSuite/Fixture/CakeTestFixture.php @@ -289,7 +289,7 @@ class CakeTestFixture { foreach ($this->records as $record) { $merge = array_values(array_merge($default, $record)); if (count($fields) !== count($merge)) { - throw new CakeException('Fixture invalid: Count of fields does not match count of values'); + throw new CakeException('Fixture invalid: Count of fields does not match count of values in ' . get_class($this)); } $values[] = $merge; }