From 2d16d69ddb863b55de3a94fd83a49a3f666354af Mon Sep 17 00:00:00 2001 From: Hamish Friedlander Date: Tue, 12 Apr 2016 08:39:55 +1200 Subject: [PATCH] API Use base data class for ChangeSetItem#ObjectClass, not just ClassName This is a data structure change, but makes ChangeSetItems less likely to break on /dev/build where a ClassName changes --- model/versioning/ChangeSet.php | 8 ++++---- model/versioning/ChangeSetItem.php | 13 ++++++++----- tests/model/ChangeSetItemTest.php | 2 +- tests/model/ChangeSetTest.php | 27 +++++++++++++++++++++++++-- tests/model/ChangeSetTest.yml | 4 ++++ 5 files changed, 42 insertions(+), 12 deletions(-) diff --git a/model/versioning/ChangeSet.php b/model/versioning/ChangeSet.php index 8a3716098..953561e29 100644 --- a/model/versioning/ChangeSet.php +++ b/model/versioning/ChangeSet.php @@ -117,7 +117,7 @@ class ChangeSet extends DataObject { $references = [ 'ObjectID' => $object->ID, - 'ObjectClass' => $object->ClassName + 'ObjectClass' => ClassInfo::baseDataClass($object) ]; // Get existing item in case already added @@ -146,7 +146,7 @@ class ChangeSet extends DataObject { public function removeObject(DataObject $object) { $item = ChangeSetItem::get()->filter([ 'ObjectID' => $object->ID, - 'ObjectClass' => $object->ClassName, + 'ObjectClass' => ClassInfo::baseDataClass($object), 'ChangeSetID' => $this->ID ])->first(); @@ -161,7 +161,7 @@ class ChangeSet extends DataObject { protected function implicitKey($item) { if ($item instanceof ChangeSetItem) return $item->ObjectClass.'.'.$item->ObjectID; - return $item->ClassName.'.'.$item->ID; + return ClassInfo::baseDataClass($item).'.'.$item->ID; } protected function calculateImplicit() { @@ -183,7 +183,7 @@ class ChangeSet extends DataObject { $referenced[$key] = [ 'ObjectID' => $referee->ID, - 'ObjectClass' => $referee->ClassName + 'ObjectClass' => ClassInfo::baseDataClass($referee) ]; $references[$key][] = $item->ID; diff --git a/model/versioning/ChangeSetItem.php b/model/versioning/ChangeSetItem.php index 071e2b907..ce4b5f802 100644 --- a/model/versioning/ChangeSetItem.php +++ b/model/versioning/ChangeSetItem.php @@ -9,8 +9,8 @@ use SilverStripe\Filesystem\Thumbnail; * A single line in a changeset * * @property string $Added - * @property string $ObjectClass - * @property int $ObjectID + * @property string $ObjectClass The _base_ data class for the referenced DataObject + * @property int $ObjectID The numeric ID for the referenced object * @method ManyManyList ReferencedBy() List of explicit items that require this change * @method ManyManyList References() List of implicit items required by this change * @method ChangeSet ChangeSet() @@ -65,6 +65,12 @@ class ChangeSetItem extends DataObject implements Thumbnail { ) ); + public function onBeforeWrite() { + // Make sure ObjectClass refers to the base data class in the case of old or wrong code + $this->ObjectClass = ClassInfo::baseDataClass($this->ObjectClass); + parent::onBeforeWrite(); + } + public function getTitle() { // Get title of modified object $object = $this->getObjectLatestVersion(); @@ -74,8 +80,6 @@ class ChangeSetItem extends DataObject implements Thumbnail { return $this->i18n_singular_name() . ' #' . $this->ID; } - - /** * Get a thumbnail for this object * @@ -91,7 +95,6 @@ class ChangeSetItem extends DataObject implements Thumbnail { return null; } - /** * Get the type of change: none, created, deleted, modified, manymany * diff --git a/tests/model/ChangeSetItemTest.php b/tests/model/ChangeSetItemTest.php index 7c75e64d3..31b5907a5 100644 --- a/tests/model/ChangeSetItemTest.php +++ b/tests/model/ChangeSetItemTest.php @@ -28,7 +28,7 @@ class ChangeSetItemTest extends SapphireTest { $item = new ChangeSetItem([ 'ObjectID' => $object->ID, - 'ObjectClass' => $object->ClassName + 'ObjectClass' => ClassInfo::baseDataClass($object->ClassName) ]); $this->assertEquals( diff --git a/tests/model/ChangeSetTest.php b/tests/model/ChangeSetTest.php index d74f27feb..003b4a764 100644 --- a/tests/model/ChangeSetTest.php +++ b/tests/model/ChangeSetTest.php @@ -98,6 +98,16 @@ class ChangeSetTest_End extends DataObject implements TestOnly { ]; } +/** + * @mixin Versioned + */ +class ChangeSetTest_EndChild extends ChangeSetTest_End implements TestOnly { + + private static $db = [ + 'Qux' => 'Int', + ]; +} + /** * Test {@see ChangeSet} and {@see ChangeSetItem} models */ @@ -109,6 +119,7 @@ class ChangeSetTest extends SapphireTest { 'ChangeSetTest_Base', 'ChangeSetTest_Mid', 'ChangeSetTest_End', + 'ChangeSetTest_EndChild', ]; /** @@ -139,7 +150,7 @@ class ChangeSetTest extends SapphireTest { $object = $this->objFromFixture($class, $identifier); foreach($items as $i => $item) { - if ($item->ObjectClass == $object->ClassName && $item->ObjectID == $object->ID && $item->Added == $mode) { + if ($item->ObjectClass == ClassInfo::baseDataClass($object) && $item->ObjectID == $object->ID && $item->Added == $mode) { unset($items[$i]); continue 2; } @@ -160,6 +171,19 @@ class ChangeSetTest extends SapphireTest { ); } } + + public function testAddObject() { + $cs = new ChangeSet(); + $cs->write(); + + $cs->addObject($this->objFromFixture('ChangeSetTest_End', 'end1')); + $cs->addObject($this->objFromFixture('ChangeSetTest_EndChild', 'endchild1')); + + $this->assertChangeSetLooksLike($cs, [ + 'ChangeSetTest_End.end1' => ChangeSetItem::EXPLICITLY, + 'ChangeSetTest_EndChild.endchild1' => ChangeSetItem::EXPLICITLY + ]); + } public function testRepeatedSyncIsNOP() { $this->publishAllFixtures(); @@ -256,7 +280,6 @@ class ChangeSetTest extends SapphireTest { $this->assertTrue($cs->isSynced()); } - public function testCanPublish() { // Create changeset containing all items (unpublished) $this->logInWithPermission('ADMIN'); diff --git a/tests/model/ChangeSetTest.yml b/tests/model/ChangeSetTest.yml index 7c405610a..bc6e4d2f2 100644 --- a/tests/model/ChangeSetTest.yml +++ b/tests/model/ChangeSetTest.yml @@ -6,6 +6,10 @@ ChangeSetTest_End: Baz: 1 end2: Baz: 2 +ChangeSetTest_EndChild: + endchild1: + Baz: 3 + Qux: 3 ChangeSetTest_Mid: mid1: Bar: 1