From 6a6cf2f9ebe72061f002143bc2c3c75f7805586c Mon Sep 17 00:00:00 2001 From: Damian Mooyman Date: Thu, 16 Nov 2017 12:54:07 +1300 Subject: [PATCH 1/3] ENHANCEMENT Raise warning if DBField::create_field() would behave unpredictably and improve PHPDoc --- src/ORM/FieldType/DBField.php | 27 ++++++++++++++++++++------- 1 file changed, 20 insertions(+), 7 deletions(-) diff --git a/src/ORM/FieldType/DBField.php b/src/ORM/FieldType/DBField.php index e8d04b650..70aa13cb4 100644 --- a/src/ORM/FieldType/DBField.php +++ b/src/ORM/FieldType/DBField.php @@ -2,8 +2,9 @@ namespace SilverStripe\ORM\FieldType; -use SilverStripe\Core\Injector\Injector; +use InvalidArgumentException; use SilverStripe\Core\Convert; +use SilverStripe\Core\Injector\Injector; use SilverStripe\Forms\FormField; use SilverStripe\Forms\TextField; use SilverStripe\ORM\DataObject; @@ -139,7 +140,7 @@ abstract class DBField extends ViewableData implements DBIndexable if ($options) { if (!is_array($options)) { - throw new \InvalidArgumentException("Invalid options $options"); + throw new InvalidArgumentException("Invalid options $options"); } $this->setOptions($options); } @@ -152,16 +153,26 @@ abstract class DBField extends ViewableData implements DBIndexable * * Useful for accessing the classes behaviour for other parts of your code. * - * @param string $className class of field to construct + * @param string $spec Class specification to construct. May include both service name and additional + * constructor arguments in the same format as DataObject.db config. * @param mixed $value value of field * @param string $name Name of field - * @param mixed $object Additional parameter to pass to field constructor + * @param mixed $args Additional arguments to pass to constructor if not using args in service $spec + * Note: Will raise a warning if using both * @return static */ - public static function create_field($className, $value, $name = null, $object = null) + public static function create_field($spec, $value, $name = null, ...$args) { + // Raise warning if inconsistent with DataObject::dbObject() behaviour + // This will cause spec args to be shifted down by the number of provided $args + if ($args && strpos($spec, '(') !== false) { + trigger_error('Additional args provided in both $spec and $args', E_USER_WARNING); + } + // Ensure name is always first argument + array_unshift($args, $name); + /** @var DBField $dbField */ - $dbField = Injector::inst()->create($className, $name, $object); + $dbField = Injector::inst()->createWithArgs($spec, $args); $dbField->setValue($value, null, false); return $dbField; } @@ -633,11 +644,13 @@ DBG; public function getIndexSpecs() { - if ($type = $this->getIndexType()) { + $type = $this->getIndexType(); + if ($type) { return [ 'type' => $type, 'columns' => [$this->getName()], ]; } + return null; } } From ef58799103ea380d7f1db106e2535fd922c46fd6 Mon Sep 17 00:00:00 2001 From: Damian Mooyman Date: Thu, 16 Nov 2017 12:54:51 +1300 Subject: [PATCH 2/3] BUG Fix DBEnum ignoring empty defaults FIxes #7582 --- src/ORM/FieldType/DBEnum.php | 48 +++++++++++++++++++----------------- tests/php/ORM/DBEnumTest.php | 31 +++++++++++++++++++++++ 2 files changed, 56 insertions(+), 23 deletions(-) create mode 100644 tests/php/ORM/DBEnumTest.php diff --git a/src/ORM/FieldType/DBEnum.php b/src/ORM/FieldType/DBEnum.php index fc7802de7..e2c9d542c 100644 --- a/src/ORM/FieldType/DBEnum.php +++ b/src/ORM/FieldType/DBEnum.php @@ -32,45 +32,46 @@ class DBEnum extends DBString private static $default_search_filter_class = 'ExactMatchFilter'; /** - * Create a new Enum field. + * Create a new Enum field, which is a value within a defined set, with an optional default. * - * Example usage in {@link DataObject::$db} with comma-separated string - * notation ('Val1' is default) + * Example field specification strings: * * - * "MyField" => "Enum('Val1, Val2, Val3', 'Val1')" - * - * - * Example usage in in {@link DataObject::$db} with array notation - * ('Val1' is default) - * - * - * "MyField" => "Enum(array('Val1', 'Val2', 'Val3'), 'Val1')" + * "MyField" => "Enum('Val1, Val2, Val3')" // First item 'Val1' is default implicitly + * "MyField" => "Enum('Val1, Val2, Val3', 'Val2')" // 'Val2' is default explicitly + * "MyField" => "Enum('Val1, Val2, Val3', null)" // Force empty (no) default + * "MyField" => "Enum(array('Val1', 'Val2', 'Val3'), 'Val1')" // Supports array notation as well * * * @param string $name * @param string|array $enum A string containing a comma separated list of options or an array of Vals. - * @param string $default The default option, which is either NULL or one of the items in the enumeration. + * @param string|int|null $default The default option, which is either NULL or one of the items in the enumeration. + * If passing in an integer (non-string) it will default to the index of that item in the list. + * Set to null or empty string to allow empty values * @param array $options Optional parameters for this DB field */ - public function __construct($name = null, $enum = null, $default = null, $options = []) + public function __construct($name = null, $enum = null, $default = 0, $options = []) { if ($enum) { $this->setEnum($enum); + $enum = $this->getEnum(); - // If there's a default, then - if ($default) { - if (in_array($default, $this->getEnum())) { + // If there's a default, then use this + if ($default && !is_int($default)) { + if (in_array($default, $enum)) { $this->setDefault($default); } else { - user_error("Enum::__construct() The default value '$default' does not match any item in the" - . " enumeration", E_USER_ERROR); + user_error( + "Enum::__construct() The default value '$default' does not match any item in the enumeration", + E_USER_ERROR + ); } - - // By default, set the default value to the first item + } elseif (is_int($default) && $default < count($enum)) { + // Set to specified index if given + $this->setDefault($enum[$default]); } else { - $enum = $this->getEnum(); - $this->setDefault(reset($enum)); + // Set to null if specified + $this->setDefault(null); } } @@ -187,7 +188,7 @@ class DBEnum extends DBString ltrim(preg_replace('/,\s*\n\s*$/', '', $enum)) ); } - $this->enum = $enum; + $this->enum = array_values($enum); return $this; } @@ -210,6 +211,7 @@ class DBEnum extends DBString public function setDefault($default) { $this->default = $default; + $this->setDefaultValue($default); return $this; } } diff --git a/tests/php/ORM/DBEnumTest.php b/tests/php/ORM/DBEnumTest.php new file mode 100644 index 000000000..3ed633f15 --- /dev/null +++ b/tests/php/ORM/DBEnumTest.php @@ -0,0 +1,31 @@ +assertEquals('A', $enum1->getDefaultValue()); + $this->assertEquals('A', $enum1->getDefault()); + $this->assertEquals(null, $enum2->getDefaultValue()); + $this->assertEquals(null, $enum2->getDefault()); + $this->assertEquals(null, $enum3->getDefaultValue()); + $this->assertEquals(null, $enum3->getDefault()); + $this->assertEquals('B', $enum4->getDefaultValue()); + $this->assertEquals('B', $enum4->getDefault()); + } +} From cbf9e40115cfc6d6ed42e3936d70343e697b77d4 Mon Sep 17 00:00:00 2001 From: Damian Mooyman Date: Fri, 17 Nov 2017 12:35:55 +1300 Subject: [PATCH 3/3] BUG Fix postgres / PDO support --- src/ORM/Connect/PDOConnector.php | 2 +- tests/php/ORM/PDODatabaseTest.php | 8 ++++---- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/src/ORM/Connect/PDOConnector.php b/src/ORM/Connect/PDOConnector.php index 753957488..7b3902251 100644 --- a/src/ORM/Connect/PDOConnector.php +++ b/src/ORM/Connect/PDOConnector.php @@ -163,7 +163,7 @@ class PDOConnector extends DBConnector $connCollation = Config::inst()->get('SilverStripe\ORM\Connect\MySQLDatabase', 'connection_collation'); // Set charset if given and not null. Can explicitly set to empty string to omit - if ($parameters['driver'] !== 'sqlsrv') { + if (!in_array($parameters['driver'], ['sqlsrv', 'pgsql'])) { $charset = isset($parameters['charset']) ? $parameters['charset'] : $connCharset; diff --git a/tests/php/ORM/PDODatabaseTest.php b/tests/php/ORM/PDODatabaseTest.php index d7025cbf9..d90e85382 100644 --- a/tests/php/ORM/PDODatabaseTest.php +++ b/tests/php/ORM/PDODatabaseTest.php @@ -104,17 +104,17 @@ class PDODatabaseTest extends SapphireTest $this->markTestSkipped('This test requires the current DB connector is PDO'); } - $query = new SQLUpdate('MySQLDatabaseTest_Data'); - $query->setAssignments(array('Title' => 'New Title')); + $query = new SQLUpdate('"MySQLDatabaseTest_Data"'); + $query->setAssignments(array('"Title"' => 'New Title')); // Test update which affects no rows - $query->setWhere(array('Title' => 'Bob')); + $query->setWhere(array('"Title"' => 'Bob')); $result = $query->execute(); $this->assertInstanceOf(PDOQuery::class, $result); $this->assertEquals(0, DB::affected_rows()); // Test update which affects some rows - $query->setWhere(array('Title' => 'First Item')); + $query->setWhere(array('"Title"' => 'First Item')); $result = $query->execute(); $this->assertInstanceOf(PDOQuery::class, $result); $this->assertEquals(1, DB::affected_rows());