From 4ec1a682cf354e2425ef4fd6598c7de8e807bcc7 Mon Sep 17 00:00:00 2001 From: Maxime Rainville Date: Fri, 22 Feb 2019 12:09:15 +1300 Subject: [PATCH 1/3] BUG Renable the ability to do dynamic assignment with DBField --- model/DataObject.php | 14 ++++-- model/ManyManyList.php | 18 ++++--- tests/model/DataObjectTest.php | 31 ++++++++++++ tests/model/ManyManyListTest.php | 35 +++++++++++++ .../Mock/MockDynamicAssignmentDBField.php | 49 +++++++++++++++++++ .../Mock/MockDynamicAssignmentDataObject.php | 44 +++++++++++++++++ 6 files changed, 180 insertions(+), 11 deletions(-) create mode 100644 tests/model/Mock/MockDynamicAssignmentDBField.php create mode 100644 tests/model/Mock/MockDynamicAssignmentDataObject.php diff --git a/model/DataObject.php b/model/DataObject.php index 5092b4a96..26997cdfa 100644 --- a/model/DataObject.php +++ b/model/DataObject.php @@ -1362,12 +1362,16 @@ 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)) { - user_error( - 'DataObject::writeManipulation: parameterised field assignments are disallowed', - E_USER_ERROR - ); + $dbObject = $this->dbObject($fieldName); + // If the field allows non-scalar values we'll let it do dynamic assignments + if ($dbObject && $dbObject->scalarValueOnly()) { + user_error( + 'DataObject::writeManipulation: parameterised field assignments are disallowed', + E_USER_ERROR + ); + } } } } diff --git a/model/ManyManyList.php b/model/ManyManyList.php index 32963f5fc..9beea3e6f 100644 --- a/model/ManyManyList.php +++ b/model/ManyManyList.php @@ -254,8 +254,10 @@ class ManyManyList extends RelationList { ); } + /** @var DBField[] $fieldObjects */ + $fieldObjects = array(); if($extraFields && $this->extraFields) { - // Write extra field to manipluation in the same way + // Write extra field to manipulation in the same way // that DataObject::prepareManipulationTable writes fields foreach($this->extraFields as $fieldName => $fieldSpec) { // Skip fields without an assignment @@ -263,6 +265,7 @@ class ManyManyList extends RelationList { $fieldObject = Object::create_from_string($fieldSpec, $fieldName); $fieldObject->setValue($extraFields[$fieldName]); $fieldObject->writeToManipulation($manipulation[$this->joinTable]); + $fieldObjects[$fieldName] = $fieldObject; } } } @@ -275,12 +278,15 @@ class ManyManyList extends RelationList { if (!isset($tableManipulation['fields'])) { continue; } - foreach ($tableManipulation['fields'] as $fieldValue) { + foreach ($tableManipulation['fields'] as $fieldName => $fieldValue) { if (is_array($fieldValue)) { - user_error( - 'ManyManyList::add: parameterised field assignments are disallowed', - E_USER_ERROR - ); + // If the field allows non-scalar values we'll let it do dynamic assignments + if (isset($fieldObjects[$fieldName]) && $fieldObjects[$fieldName]->scalarValueOnly()) { + user_error( + 'ManyManyList::add: parameterised field assignments are disallowed', + E_USER_ERROR + ); + } } } } diff --git a/tests/model/DataObjectTest.php b/tests/model/DataObjectTest.php index be3e01484..5521c1aa5 100644 --- a/tests/model/DataObjectTest.php +++ b/tests/model/DataObjectTest.php @@ -32,6 +32,7 @@ class DataObjectTest extends SapphireTest { 'DataObjectTest_Sortable', 'ManyManyListTest_Product', 'ManyManyListTest_Category', + 'MockDynamicAssignmentDataObject' ); /** @@ -1773,6 +1774,36 @@ class DataObjectTest extends SapphireTest { $this->assertNotEmpty($do->CompositeMoneyField); } + + public function testWriteManipulationWithNonScalarValuesAllowed() + { + $do = MockDynamicAssignmentDataObject::create(); + $do->write(); + $do->StaticScalarOnlyField = true; + $do->DynamicScalarOnlyField = false; + $do->DynamicField = true; + $do->write(); + $this->assertEquals(1, $do->StaticScalarOnlyField); + $this->assertEquals(0, $do->DynamicScalarOnlyField); + $this->assertEquals(1, $do->DynamicField); + } + + public function testWriteManipulationWithNonScalarValuesDisallowed() + { + + $do = MockDynamicAssignmentDataObject::create(); + $do->write(); + $do->StaticScalarOnlyField = false; + $do->DynamicScalarOnlyField = true; + $do->DynamicField = false; + + try { + $do->write(); + $this->fail("Expected exception \"parameterised field assignments are disallowed\""); + } catch (Exception $ex) { + $this->assertContains('parameterised field assignments are disallowed', $ex->getMessage()); + } + } } class DataObjectTest_Sortable extends DataObject implements TestOnly { diff --git a/tests/model/ManyManyListTest.php b/tests/model/ManyManyListTest.php index 7990a8b51..11d3c2b9c 100644 --- a/tests/model/ManyManyListTest.php +++ b/tests/model/ManyManyListTest.php @@ -21,6 +21,7 @@ class ManyManyListTest extends SapphireTest { 'ManyManyListTest_ExtraFields', 'ManyManyListTest_Product', 'ManyManyListTest_Category', + 'MockDynamicAssignmentDataObject', ); @@ -306,7 +307,41 @@ class ManyManyListTest extends SapphireTest { $this->assertEquals(1, $productsRelatedToProductB->count()); } + public function testWriteManipulationWithNonScalarValuesAllowed() + { + $left = MockDynamicAssignmentDataObject::create(); + $left->write(); + $right = MockDynamicAssignmentDataObject::create(); + $right->write(); + $left->MockManyMany()->add($right, array( + 'ManyManyStaticScalarOnlyField' => true, + 'ManyManyDynamicScalarOnlyField' => false, + 'ManyManyDynamicField' => true + )); + $pivot = $left->MockManyMany()->first(); + $this->assertEquals(1, $pivot->ManyManyStaticScalarOnlyField); + $this->assertEquals(0, $pivot->ManyManyDynamicScalarOnlyField); + $this->assertEquals(1, $pivot->ManyManyDynamicField); + } + public function testWriteManipulationWithNonScalarValuesDisallowed() + { + $left = MockDynamicAssignmentDataObject::create(); + $left->write(); + $right = MockDynamicAssignmentDataObject::create(); + $right->write(); + + try { + $left->MockManyMany()->add($right, array( + 'ManyManyStaticScalarOnlyField' => true, + 'ManyManyDynamicScalarOnlyField' => false, + 'ManyManyDynamicField' => true + )); + $this->fail("Expected exception \"parameterised field assignments are disallowed\""); + } catch (Exception $ex) { + $this->assertContains('parameterised field assignments are disallowed', $ex->getMessage()); + } + } } /** diff --git a/tests/model/Mock/MockDynamicAssignmentDBField.php b/tests/model/Mock/MockDynamicAssignmentDBField.php new file mode 100644 index 000000000..bd82d1700 --- /dev/null +++ b/tests/model/Mock/MockDynamicAssignmentDBField.php @@ -0,0 +1,49 @@ +scalarOnly = $scalarOnly; + $this->dynamicAssignment = $dynamicAssignment; + parent::__construct($name); + } + + /** + * If the field value and dynamicAssignment are true, we'll try to do a dynamic assignment + * @param $value + * @return array|int|mixed + */ + public function prepValueForDB($value) + { + if ($value) { + return $this->dynamicAssignment + ? array('GREATEST(?, ?)' => array(0, 1)) + : 1; + } + + return 0; + } + + public function scalarValueOnly() + { + return $this->scalarOnly; + } +} diff --git a/tests/model/Mock/MockDynamicAssignmentDataObject.php b/tests/model/Mock/MockDynamicAssignmentDataObject.php new file mode 100644 index 000000000..5363de1fa --- /dev/null +++ b/tests/model/Mock/MockDynamicAssignmentDataObject.php @@ -0,0 +1,44 @@ + 'MockDynamicAssignmentDBField(1,0)', + + // This field tries to emit dynamic assignment but will fail because of scalar only + 'DynamicScalarOnlyField' => 'MockDynamicAssignmentDBField(1,1)', + + // This field does dynamic assignment and will pass + 'DynamicField' => 'MockDynamicAssignmentDBField(0,1)', + ); + + private static $many_many = array( + "MockManyMany" => 'MockDynamicAssignmentDataObject' + ); + + private static $belongs_many_many = array( + "MockBelongsManyMany" => 'MockDynamicAssignmentDataObject' + ); + + private static $many_many_extraFields = array( + 'MockManyMany' => array( + // This field only emits scalar value and will save + 'ManyManyStaticScalarOnlyField' => 'MockDynamicAssignmentDBField(1,0)', + + // This field tries to emit dynamic assignment but will fail because of scalar only + 'ManyManyDynamicScalarOnlyField' => 'MockDynamicAssignmentDBField(1,1)', + + // This field does dynamic assignment and will pass + 'ManyManyDynamicField' => 'MockDynamicAssignmentDBField(0,1)', + ) + ); +} From adbc560bd70ba2e071f94a41a084768819196ee7 Mon Sep 17 00:00:00 2001 From: Maxime Rainville Date: Mon, 25 Feb 2019 15:16:26 +1300 Subject: [PATCH 2/3] BUG Address PR feedback. --- tests/model/DataObjectTest.php | 11 +++++------ tests/model/ManyManyListTest.php | 19 +++++++++---------- .../Mock/MockDynamicAssignmentDBField.php | 4 ++-- 3 files changed, 16 insertions(+), 18 deletions(-) diff --git a/tests/model/DataObjectTest.php b/tests/model/DataObjectTest.php index 5521c1aa5..9c071564f 100644 --- a/tests/model/DataObjectTest.php +++ b/tests/model/DataObjectTest.php @@ -1788,6 +1788,10 @@ class DataObjectTest extends SapphireTest { $this->assertEquals(1, $do->DynamicField); } + /** + * @expectedException PHPUnit_Framework_Error + * @expectedExceptionMessageRegExp /parameterised field assignments are disallowed/ + */ public function testWriteManipulationWithNonScalarValuesDisallowed() { @@ -1797,12 +1801,7 @@ class DataObjectTest extends SapphireTest { $do->DynamicScalarOnlyField = true; $do->DynamicField = false; - try { - $do->write(); - $this->fail("Expected exception \"parameterised field assignments are disallowed\""); - } catch (Exception $ex) { - $this->assertContains('parameterised field assignments are disallowed', $ex->getMessage()); - } + $do->write(); } } diff --git a/tests/model/ManyManyListTest.php b/tests/model/ManyManyListTest.php index 11d3c2b9c..a75c92cc2 100644 --- a/tests/model/ManyManyListTest.php +++ b/tests/model/ManyManyListTest.php @@ -324,6 +324,10 @@ class ManyManyListTest extends SapphireTest { $this->assertEquals(1, $pivot->ManyManyDynamicField); } + /** + * @expectedException PHPUnit_Framework_Error + * @expectedExceptionMessageRegExp /parameterised field assignments are disallowed/ + */ public function testWriteManipulationWithNonScalarValuesDisallowed() { $left = MockDynamicAssignmentDataObject::create(); @@ -331,16 +335,11 @@ class ManyManyListTest extends SapphireTest { $right = MockDynamicAssignmentDataObject::create(); $right->write(); - try { - $left->MockManyMany()->add($right, array( - 'ManyManyStaticScalarOnlyField' => true, - 'ManyManyDynamicScalarOnlyField' => false, - 'ManyManyDynamicField' => true - )); - $this->fail("Expected exception \"parameterised field assignments are disallowed\""); - } catch (Exception $ex) { - $this->assertContains('parameterised field assignments are disallowed', $ex->getMessage()); - } + $left->MockManyMany()->add($right, array( + 'ManyManyStaticScalarOnlyField' => false, + 'ManyManyDynamicScalarOnlyField' => true, + 'ManyManyDynamicField' => false + )); } } diff --git a/tests/model/Mock/MockDynamicAssignmentDBField.php b/tests/model/Mock/MockDynamicAssignmentDBField.php index bd82d1700..5949434fd 100644 --- a/tests/model/Mock/MockDynamicAssignmentDBField.php +++ b/tests/model/Mock/MockDynamicAssignmentDBField.php @@ -15,7 +15,7 @@ class MockDynamicAssignmentDBField extends Boolean private $dynamicAssignment; /** - * @param $name + * @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. */ @@ -28,7 +28,7 @@ class MockDynamicAssignmentDBField extends Boolean /** * If the field value and dynamicAssignment are true, we'll try to do a dynamic assignment - * @param $value + * @param mixed $value * @return array|int|mixed */ public function prepValueForDB($value) From bd929694188dc7df7277d8430df5534dcb2b914a Mon Sep 17 00:00:00 2001 From: Maxime Rainville Date: Tue, 26 Feb 2019 14:20:14 +1300 Subject: [PATCH 3/3] FIX Use a function common to MySQL, SQLite and PostgreSQL to test dynamic DBFIeld assigment --- tests/model/Mock/MockDynamicAssignmentDBField.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/model/Mock/MockDynamicAssignmentDBField.php b/tests/model/Mock/MockDynamicAssignmentDBField.php index 5949434fd..4a09c4a29 100644 --- a/tests/model/Mock/MockDynamicAssignmentDBField.php +++ b/tests/model/Mock/MockDynamicAssignmentDBField.php @@ -35,7 +35,7 @@ class MockDynamicAssignmentDBField extends Boolean { if ($value) { return $this->dynamicAssignment - ? array('GREATEST(?, ?)' => array(0, 1)) + ? array('ABS(?)' => array(1)) : 1; }