From 9a59f2f57dfd5fe0f054b01404bc2bd958ad8d99 Mon Sep 17 00:00:00 2001 From: Maxime Rainville Date: Fri, 22 Feb 2019 11:08:43 +1300 Subject: [PATCH 1/2] BUG Renable the ability to do dynamic assignment with DBField --- src/ORM/DataObject.php | 12 +++-- src/ORM/ManyManyList.php | 14 +++-- tests/php/ORM/DataObjectTest.php | 31 +++++++++++ .../MockDynamicAssignmentDBField.php | 54 +++++++++++++++++++ .../MockDynamicAssignmentDataObject.php | 52 ++++++++++++++++++ tests/php/ORM/ManyManyListTest.php | 37 +++++++++++++ 6 files changed, 192 insertions(+), 8 deletions(-) create mode 100644 tests/php/ORM/DataObjectTest/MockDynamicAssignmentDBField.php create mode 100644 tests/php/ORM/DataObjectTest/MockDynamicAssignmentDataObject.php diff --git a/src/ORM/DataObject.php b/src/ORM/DataObject.php index f6e49222d..021349606 100644 --- a/src/ORM/DataObject.php +++ b/src/ORM/DataObject.php @@ -1331,11 +1331,15 @@ class DataObject extends ViewableData implements DataObjectInterface, i18nEntity if (!isset($tableManipulation['fields'])) { continue; } - foreach ($tableManipulation['fields'] as $fieldValue) { + foreach ($tableManipulation['fields'] as $fieldName => $fieldValue) { if (is_array($fieldValue)) { - throw new InvalidArgumentException( - 'DataObject::writeManipulation: parameterised field assignments are disallowed' - ); + $dbObject = $this->dbObject($fieldName); + // If the field allows non-scalar values we'll let it do dynamic assignments + if ($dbObject && $dbObject->scalarValueOnly()) { + throw new InvalidArgumentException( + 'DataObject::writeManipulation: parameterised field assignments are disallowed' + ); + } } } } diff --git a/src/ORM/ManyManyList.php b/src/ORM/ManyManyList.php index a576a33db..3bdb8cbaa 100644 --- a/src/ORM/ManyManyList.php +++ b/src/ORM/ManyManyList.php @@ -279,6 +279,8 @@ class ManyManyList extends RelationList ); } + /** @var DBField[] $fieldObjects */ + $fieldObjects = []; if ($extraFields && $this->extraFields) { // Write extra field to manipluation in the same way // that DataObject::prepareManipulationTable writes fields @@ -288,6 +290,7 @@ class ManyManyList extends RelationList $fieldObject = Injector::inst()->create($fieldSpec, $fieldName); $fieldObject->setValue($extraFields[$fieldName]); $fieldObject->writeToManipulation($manipulation[$this->joinTable]); + $fieldObjects[$fieldName] = $fieldObject; } } } @@ -300,11 +303,14 @@ class ManyManyList extends RelationList if (!isset($tableManipulation['fields'])) { continue; } - foreach ($tableManipulation['fields'] as $fieldValue) { + foreach ($tableManipulation['fields'] as $fieldName => $fieldValue) { if (is_array($fieldValue)) { - throw new InvalidArgumentException( - 'ManyManyList::add: parameterised field assignments are disallowed' - ); + // If the field allows non-scalar values we'll let it do dynamic assignments + if (isset($fieldObjects[$fieldName]) && $fieldObjects[$fieldName]->scalarValueOnly()) { + throw new InvalidArgumentException( + 'ManyManyList::add: parameterised field assignments are disallowed' + ); + } } } } diff --git a/tests/php/ORM/DataObjectTest.php b/tests/php/ORM/DataObjectTest.php index cfda6085e..51cfe1e8a 100644 --- a/tests/php/ORM/DataObjectTest.php +++ b/tests/php/ORM/DataObjectTest.php @@ -57,6 +57,7 @@ class DataObjectTest extends SapphireTest DataObjectTest\RelationParent::class, DataObjectTest\RelationChildFirst::class, DataObjectTest\RelationChildSecond::class, + DataObjectTest\MockDynamicAssignmentDataObject::class ); public static function getExtraDataObjects() @@ -2147,4 +2148,34 @@ class DataObjectTest extends SapphireTest $do->SalaryCap = array('Amount' => 123456, 'Currency' => 'CAD'); $this->assertNotEmpty($do->SalaryCap); } + + public function testWriteManipulationWithNonScalarValuesAllowed() + { + $do = DataObjectTest\MockDynamicAssignmentDataObject::create(); + $do->write(); + + $do->StaticScalarOnlyField = true; + $do->DynamicScalarOnlyField = false; + $do->DynamicField = true; + + $do->write(); + + $this->assertTrue($do->StaticScalarOnlyField); + $this->assertFalse($do->DynamicScalarOnlyField); + $this->assertTrue($do->DynamicField); + } + + public function testWriteManipulationWithNonScalarValuesDisallowed() + { + $this->expectException(InvalidArgumentException::class); + + $do = DataObjectTest\MockDynamicAssignmentDataObject::create(); + $do->write(); + + $do->StaticScalarOnlyField = false; + $do->DynamicScalarOnlyField = true; + $do->DynamicField = false; + + $do->write(); + } } diff --git a/tests/php/ORM/DataObjectTest/MockDynamicAssignmentDBField.php b/tests/php/ORM/DataObjectTest/MockDynamicAssignmentDBField.php new file mode 100644 index 000000000..5ab87481c --- /dev/null +++ b/tests/php/ORM/DataObjectTest/MockDynamicAssignmentDBField.php @@ -0,0 +1,54 @@ +scalarOnly = $scalarOnly; + $this->dynamicAssignment = $dynamicAssignment; + parent::__construct($name); + } + + /** + * If the field value and dynamicAssignment are true, we'll try to do a dynamic assignement + * @param $value + * @return array|int|mixed + */ + public function prepValueForDB($value) + { + if ($value) { + return $this->dynamicAssignment + ? ['GREATEST(?, ?)' => [0, 1]] + : 1; + } + + return 0; + } + + public function scalarValueOnly() + { + return $this->scalarOnly; + } +} diff --git a/tests/php/ORM/DataObjectTest/MockDynamicAssignmentDataObject.php b/tests/php/ORM/DataObjectTest/MockDynamicAssignmentDataObject.php new file mode 100644 index 000000000..8baa26cb8 --- /dev/null +++ b/tests/php/ORM/DataObjectTest/MockDynamicAssignmentDataObject.php @@ -0,0 +1,52 @@ + MockDynamicAssignmentDBField::class . '(1,0)', + + // This field tries to emit dynamic assignment but will fail because of scalar only + 'DynamicScalarOnlyField' => MockDynamicAssignmentDBField::class . '(1,1)', + + // This field does dynamic assignement and will pass + 'DynamicField' => MockDynamicAssignmentDBField::class . '(0,1)', + ]; + + private static $many_many = [ + "MockManyMany" => self::class, + ]; + + private static $belongs_many_many = [ + "MockBelongsManyMany" => self::class, + ]; + + private static $many_many_extraFields = [ + 'MockManyMany' => [ + // This field only emits scalar value and will save + 'ManyManyStaticScalarOnlyField' => MockDynamicAssignmentDBField::class . '(1,0)', + + // This field tries to emit dynamic assignment but will fail because of scalar only + 'ManyManyDynamicScalarOnlyField' => MockDynamicAssignmentDBField::class . '(1,1)', + + // This field does dynamic assignement and will pass + 'ManyManyDynamicField' => MockDynamicAssignmentDBField::class . '(0,1)', + ] + ]; +} diff --git a/tests/php/ORM/ManyManyListTest.php b/tests/php/ORM/ManyManyListTest.php index aa6218474..ab7d30859 100644 --- a/tests/php/ORM/ManyManyListTest.php +++ b/tests/php/ORM/ManyManyListTest.php @@ -20,6 +20,7 @@ class ManyManyListTest extends SapphireTest ManyManyListTest\Category::class, ManyManyListTest\ExtraFieldsObject::class, ManyManyListTest\Product::class, + DataObjectTest\MockDynamicAssignmentDataObject::class ]; public static function getExtraDataObjects() @@ -378,4 +379,40 @@ class ManyManyListTest extends SapphireTest $productsRelatedToProductB = $category->Products()->filter('RelatedProducts.Title', 'Product A'); $this->assertEquals(1, $productsRelatedToProductB->count()); } + + public function testWriteManipulationWithNonScalarValuesAllowed() + { + $left = DataObjectTest\MockDynamicAssignmentDataObject::create(); + $left->write(); + $right = DataObjectTest\MockDynamicAssignmentDataObject::create(); + $right->write(); + + $left->MockManyMany()->add($right, [ + 'ManyManyStaticScalarOnlyField' => true, + 'ManyManyDynamicScalarOnlyField' => false, + 'ManyManyDynamicField' => true, + ]); + + $pivot = $left->MockManyMany()->first(); + + $this->assertNotFalse($pivot->ManyManyStaticScalarOnlyField); + $this->assertNotTrue($pivot->ManyManyDynamicScalarOnlyField); + $this->assertNotFalse($pivot->ManyManyDynamicField); + } + + public function testWriteManipulationWithNonScalarValuesDisallowed() + { + $this->expectException(\InvalidArgumentException::class); + + $left = DataObjectTest\MockDynamicAssignmentDataObject::create(); + $left->write(); + $right = DataObjectTest\MockDynamicAssignmentDataObject::create(); + $right->write(); + + $left->MockManyMany()->add($right, [ + 'ManyManyStaticScalarOnlyField' => false, + 'ManyManyDynamicScalarOnlyField' => true, + 'ManyManyDynamicField' => false, + ]); + } } From 6ff319a0e1c4cfc15e24580bac07dfef38702942 Mon Sep 17 00:00:00 2001 From: Maxime Rainville Date: Wed, 27 Feb 2019 11:14:47 +1300 Subject: [PATCH 2/2] BUG Implement peer review feedback, --- .../MockDynamicAssignmentDBField.php | 15 ++++++++------- .../MockDynamicAssignmentDataObject.php | 4 ++-- tests/php/ORM/ManyManyListTest.php | 3 ++- 3 files changed, 12 insertions(+), 10 deletions(-) diff --git a/tests/php/ORM/DataObjectTest/MockDynamicAssignmentDBField.php b/tests/php/ORM/DataObjectTest/MockDynamicAssignmentDBField.php index 5ab87481c..25828f9dd 100644 --- a/tests/php/ORM/DataObjectTest/MockDynamicAssignmentDBField.php +++ b/tests/php/ORM/DataObjectTest/MockDynamicAssignmentDBField.php @@ -2,6 +2,7 @@ namespace SilverStripe\ORM\Tests\DataObjectTest; +use SilverStripe\Dev\TestOnly; use SilverStripe\ORM\FieldType\DBBoolean; use SilverStripe\ORM\FieldType\DBField; @@ -13,16 +14,16 @@ use SilverStripe\ORM\FieldType\DBField; * If the field is set to false, it will try to do a plain assignment. This is so you can save the initial value no * matter what. If the field is set to true, it will try to do a dynamic assignment. */ -class MockDynamicAssignmentDBField extends DBBoolean +class MockDynamicAssignmentDBField extends DBBoolean implements TestOnly { private $scalarOnly; private $dynamicAssignment; /** - * @param $name - * @param $scalarOnly Whether our fake field should be scalar only - * @param $dynamicAssigment Wheter our fake field will try to do a dynamic assignement + * @param string $name + * @param boolean $scalarOnly Whether our fake field should be scalar only. + * @param boolean $dynamicAssignment Whether our fake field will try to do a dynamic assignment. */ public function __construct($name = '', $scalarOnly = false, $dynamicAssignment = false) { @@ -32,15 +33,15 @@ class MockDynamicAssignmentDBField extends DBBoolean } /** - * If the field value and dynamicAssignment are true, we'll try to do a dynamic assignement + * If the field value and $dynamicAssignment are true, we'll try to do a dynamic assignment. * @param $value - * @return array|int|mixed + * @return array|int */ public function prepValueForDB($value) { if ($value) { return $this->dynamicAssignment - ? ['GREATEST(?, ?)' => [0, 1]] + ? ['ABS(?)' => [1]] : 1; } diff --git a/tests/php/ORM/DataObjectTest/MockDynamicAssignmentDataObject.php b/tests/php/ORM/DataObjectTest/MockDynamicAssignmentDataObject.php index 8baa26cb8..1617734cf 100644 --- a/tests/php/ORM/DataObjectTest/MockDynamicAssignmentDataObject.php +++ b/tests/php/ORM/DataObjectTest/MockDynamicAssignmentDataObject.php @@ -30,11 +30,11 @@ class MockDynamicAssignmentDataObject extends DataObject implements TestOnly ]; private static $many_many = [ - "MockManyMany" => self::class, + 'MockManyMany' => self::class, ]; private static $belongs_many_many = [ - "MockBelongsManyMany" => self::class, + 'MockBelongsManyMany' => self::class, ]; private static $many_many_extraFields = [ diff --git a/tests/php/ORM/ManyManyListTest.php b/tests/php/ORM/ManyManyListTest.php index ab7d30859..008ec8189 100644 --- a/tests/php/ORM/ManyManyListTest.php +++ b/tests/php/ORM/ManyManyListTest.php @@ -10,6 +10,7 @@ use SilverStripe\ORM\Tests\DataObjectTest\Player; use SilverStripe\ORM\Tests\DataObjectTest\Team; use SilverStripe\ORM\Tests\ManyManyListTest\ExtraFieldsObject; use SilverStripe\ORM\Tests\ManyManyListTest\Product; +use InvalidArgumentException; class ManyManyListTest extends SapphireTest { @@ -402,7 +403,7 @@ class ManyManyListTest extends SapphireTest public function testWriteManipulationWithNonScalarValuesDisallowed() { - $this->expectException(\InvalidArgumentException::class); + $this->expectException(InvalidArgumentException::class); $left = DataObjectTest\MockDynamicAssignmentDataObject::create(); $left->write();