From a570e2f2a0258c312960156d09f3af38953017c9 Mon Sep 17 00:00:00 2001 From: Ingo Schommer Date: Sat, 14 Jan 2012 11:31:01 +0100 Subject: [PATCH] API CHANGE Publish ex-VirtualPage records when their page type changes, propagate type change to live (AIR-78) BUGFIX Remove extraneous database rows when changing a page from VirtualPage to another type (AIR-78) --- code/model/VirtualPage.php | 32 ++++++++ tests/model/VirtualPageTest.php | 134 ++++++++++++++++++++++++++++---- 2 files changed, 149 insertions(+), 17 deletions(-) diff --git a/code/model/VirtualPage.php b/code/model/VirtualPage.php index 46fdfbe8..1f563e14 100644 --- a/code/model/VirtualPage.php +++ b/code/model/VirtualPage.php @@ -244,6 +244,38 @@ class VirtualPage extends Page { $this->updateImageTracking(); } } + + // Check if page type has changed to a non-virtual page. + // Caution: Relies on the fact that the current instance is still of the old page type. + if($this->isChanged('ClassName', 2)) { + $changed = $this->getChangedFields(); + $classBefore = $changed['ClassName']['before']; + $classAfter = $changed['ClassName']['after']; + if($classBefore != $classAfter) { + // Remove all database rows for the old page type to avoid inconsistent data retrieval. + // TODO This should apply to all page type changes, not only on VirtualPage - but needs + // more comprehensive testing as its a destructive operation + $removedTables = array_diff(ClassInfo::dataClassesFor($classBefore), ClassInfo::dataClassesFor($classAfter)); + if($removedTables) foreach($removedTables as $removedTable) { + // Note: *_versions records are left intact + foreach(array('', 'Live') as $stage) { + if($stage) $removedTable = "{$removedTable}_{$stage}"; + DB::query(sprintf('DELETE FROM "%s" WHERE "ID" = %d', $removedTable, $this->ID)); + } + } + + // Also publish the change immediately to avoid inconsistent behaviour between + // a non-virtual draft and a virtual live record (e.g. republishing the original record + // shouldn't republish the - now unrelated - changes on the ex-VirtualPage draft). + // Copies all stage fields to live as well. + $source = DataObject::get_one("SiteTree",sprintf('"SiteTree"."ID" = %d', $this->CopyContentFromID)); + $this->copyFrom($source); + $this->publish('Stage', 'Live'); + + // Change reference on instance (as well as removing the underlying database tables) + $this->CopyContentFromID = 0; + } + } } function validate() { diff --git a/tests/model/VirtualPageTest.php b/tests/model/VirtualPageTest.php index e3d591b6..de44a1a3 100644 --- a/tests/model/VirtualPageTest.php +++ b/tests/model/VirtualPageTest.php @@ -6,8 +6,29 @@ class VirtualPageTest extends SapphireTest { protected $extraDataObjects = array( 'VirtualPageTest_ClassA', 'VirtualPageTest_ClassB', - 'VirtualPageTest_NotRoot', + 'VirtualPageTest_VirtualPageSub', ); + + protected $requiredExtensions = array( + 'Page' => array('VirtualPageTest_PageExtension') + ); + + function setUp() { + parent::setUp(); + + $this->origInitiallyCopiedFields = VirtualPage::$initially_copied_fields; + VirtualPage::$initially_copied_fields[] = 'MyInitiallyCopiedField'; + $this->origNonVirtualField = VirtualPage::$non_virtual_fields; + VirtualPage::$non_virtual_fields[] = 'MyNonVirtualField'; + VirtualPage::$non_virtual_fields[] = 'MySharedNonVirtualField'; + } + + function tearDown() { + parent::tearDown(); + + VirtualPage::$initially_copied_fields = $this->origInitiallyCopiedFields; + VirtualPage::$non_virtual_fields = $this->origNonVirtualField; + } /** * Test that, after you update the source page of a virtual page, all the virtual pages @@ -355,11 +376,6 @@ class VirtualPageTest extends SapphireTest { } function testGetVirtualFields() { - $origInitiallyCopiedFields = VirtualPage::$initially_copied_fields; - VirtualPage::$initially_copied_fields[] = 'MyInitiallyCopiedField'; - $origNonVirtualField = VirtualPage::$non_virtual_fields; - VirtualPage::$non_virtual_fields[] = 'MyNonVirtualField'; - // Needs association with an original, otherwise will just return the "base" virtual fields $page = new VirtualPageTest_ClassA(); $page->write(); @@ -370,17 +386,9 @@ class VirtualPageTest extends SapphireTest { $this->assertContains('MyVirtualField', $virtual->getVirtualFields()); $this->assertNotContains('MyNonVirtualField', $virtual->getVirtualFields()); $this->assertNotContains('MyInitiallyCopiedField', $virtual->getVirtualFields()); - - VirtualPage::$initially_copied_fields = $origInitiallyCopiedFields; - VirtualPage::$non_virtual_fields = $origNonVirtualField; } function testCopyFrom() { - $origInitiallyCopiedFields = VirtualPage::$initially_copied_fields; - VirtualPage::$initially_copied_fields[] = 'MyInitiallyCopiedField'; - $origNonVirtualField = VirtualPage::$non_virtual_fields; - VirtualPage::$non_virtual_fields[] = 'MyNonVirtualField'; - $original = new VirtualPageTest_ClassA(); $original->MyInitiallyCopiedField = 'original'; $original->MyVirtualField = 'original'; @@ -416,9 +424,6 @@ class VirtualPageTest extends SapphireTest { $virtual->MyInitiallyCopiedField, 'Fields listed in $initially_copied_fields are not copied on subsequent copyFrom() invocations' ); - - VirtualPage::$initially_copied_fields = $origInitiallyCopiedFields; - VirtualPage::$non_virtual_fields = $origNonVirtualField; } function testWriteWithoutVersion() { @@ -494,6 +499,82 @@ class VirtualPageTest extends SapphireTest { if(!$isDetected) $this->fail('Fails validation with $can_be_root=false'); } + + function testPageTypeChangeDoesntKeepOrphanedVirtualPageRecord() { + $page = new SiteTree(); + $page->write(); + $page->publish('Stage', 'Live'); + + $virtual = new VirtualPageTest_VirtualPageSub(); + $virtual->CopyContentFromID = $page->ID; + $virtual->write(); + $virtual->publish('Stage', 'Live'); + + $nonVirtual = $virtual; + $nonVirtual->ClassName = 'VirtualPageTest_ClassA'; + $nonVirtual->write(); // not publishing + + $this->assertNotNull( + DB::query(sprintf('SELECT "ID" FROM "SiteTree" WHERE "ID" = %d', $nonVirtual->ID))->value(), + "Shared base database table entry exists after type change" + ); + $this->assertNull( + DB::query(sprintf('SELECT "ID" FROM "VirtualPage" WHERE "ID" = %d', $nonVirtual->ID))->value(), + "Base database table entry no longer exists after type change" + ); + $this->assertNull( + DB::query(sprintf('SELECT "ID" FROM "VirtualPageTest_VirtualPageSub" WHERE "ID" = %d', $nonVirtual->ID))->value(), + "Sub database table entry no longer exists after type change" + ); + $this->assertNull( + DB::query(sprintf('SELECT "ID" FROM "VirtualPage_Live" WHERE "ID" = %d', $nonVirtual->ID))->value(), + "Base live database table entry no longer exists after type change" + ); + $this->assertNull( + DB::query(sprintf('SELECT "ID" FROM "VirtualPageTest_VirtualPageSub_Live" WHERE "ID" = %d', $nonVirtual->ID))->value(), + "Sub live database table entry no longer exists after type change" + ); + } + + function testPageTypeChangePropagatesToLive() { + $page = new SiteTree(); + $page->MySharedNonVirtualField = 'original'; + $page->write(); + $page->publish('Stage', 'Live'); + + $virtual = new VirtualPageTest_VirtualPageSub(); + $virtual->CopyContentFromID = $page->ID; + $virtual->write(); + $virtual->publish('Stage', 'Live'); + + $page->Title = 'original'; // 'Title' is a virtual field + // Publication would causes the virtual field to copy through onBeforeWrite(), + // but we want to test that it gets copied on class name change instead + $page->write(); + + $nonVirtual = $virtual; + $nonVirtual->ClassName = 'VirtualPageTest_ClassA'; + $nonVirtual->MySharedNonVirtualField = 'changed on new type'; + $nonVirtual->write(); // not publishing the page type change here + + $this->assertEquals('original', $nonVirtual->Title, + 'Copies virtual fields from original draft into new instance on type change ' + ); + + $nonVirtualLive = Versioned::get_one_by_stage('SiteTree', 'Live', '"SiteTree_Live"."ID" = ' . $nonVirtual->ID); + $this->assertNotNull($nonVirtualLive); + $this->assertEquals('VirtualPageTest_ClassA', $nonVirtualLive->ClassName); + $this->assertEquals('changed on new type', $nonVirtualLive->MySharedNonVirtualField); + + $page->MySharedNonVirtualField = 'changed only on original'; + $page->write(); + $page->publish('Stage', 'Live'); + + $nonVirtualLive = Versioned::get_one_by_stage('SiteTree', 'Live', '"SiteTree_Live"."ID" = ' . $nonVirtual->ID, false); + $this->assertEquals('changed on new type', $nonVirtualLive->MySharedNonVirtualField, + 'No field copying from previous original after page type changed' + ); + } } class VirtualPageTest_ClassA extends Page implements TestOnly { @@ -517,4 +598,23 @@ class VirtualPageTest_ClassC extends Page implements TestOnly { class VirtualPageTest_NotRoot extends Page implements TestOnly { static $can_be_root = false; +} + +class VirtualPageTest_VirtualPageSub extends VirtualPage implements TestOnly { + static $db = array( + 'MyProperty' => 'Varchar', + ); +} + +class VirtualPageTest_PageExtension extends DataExtension implements TestOnly { + function extraStatics() { + return array( + 'db' => array( + // These fields are just on an extension to simulate shared properties between Page and VirtualPage. + // Not possible through direct $db definitions due to VirtualPage inheriting from Page, and Page being defined elsewhere. + 'MySharedVirtualField' => 'Text', + 'MySharedNonVirtualField' => 'Text', + ) + ); + } } \ No newline at end of file