Skip to content

SiteTree.Version field don't update as expected. #2422

Open
stojg opened this Issue Sep 16, 2013 · 4 comments

2 participants

@stojg
stojg commented Sep 16, 2013

I found a somewhat annoying issue with the Version attribute doesn't update in certain circumstances.

The initial issue was discovered by changing a Page.Content field with data from another DataObject during a onBeforeWrite(). This saves fine when pressing the "save draft", but the version number of that Page doesnt update properly. This in it turns causes the version history view to be somewhat borked with the current version being below the latest version.

I've done a test case that I think should pass, but doesnt. It would be interesting to hear anyone else ideas around this.

<?php
class SiteTreeVersionBugTest extends SapphireTest {
    /**
     * Mimic the logic from CMSMain::save() with writeWithoutVersion() and later write()
     */
    public function testVersionShouldUpdate() {
        // Setup
        $page = new SiteTree();
        $page->Title = 'Initial Title';
        $page->writeWithoutVersion();
        $page->write();
        // assert passes
        $this->assertEquals(2, $page->Version, 'Version should be to 2');

        // Setup
        DataObject::flush_and_destroy_cache();
        $pageFromDB = SiteTree::get()->byId($page->ID);
        // assert fails
        $pageFromDB->Title = 'This is a new Title';
        $pageFromDB->writeWithoutVersion();
        $pageFromDB->write();
        $this->assertEquals(3, $pageFromDB->Version, 'Version should be 3');
    }
}
@stojg
stojg commented Sep 16, 2013

Note, dropping the $pageFromDB->writeWithoutVersion();will increment the Version.

@stojg
stojg commented Sep 16, 2013

Here's another testcase were I am using form to make sure that nothing fishy going on while setting the values on the DataObject:

    /**
     * Mimic the logic from CMSMain::save() with writeWithoutVersion and later write
     * but adding the changed field in a form
     */
    public function testVersionWithFormShouldUpdate() {
        // Setup
        $page = new SiteTree();
        $page->Title = 'Initial Title';
        $page->writeWithoutVersion();
        $page->write();
        // Setup
        DataObject::flush_and_destroy_cache();
        $pageFromDB = SiteTree::get()->byId($page->ID);
        $titleField = new TextField('Title', 'Title', 'This is a new Title');
        $form = new Form($this, 'TestForm', new FieldList(array($titleField)), new FieldList());
        $form->saveInto($pageFromDB);
        // Assert fails
        $pageFromDB->writeWithoutVersion();
        $pageFromDB->write();
        $this->assertEquals(3, $pageFromDB->Version, 'Version should be 3');
    }
@stojg stojg added a commit to stojg/sapphire that referenced this issue Sep 16, 2013
@stojg stojg Illustrating a version bug #2422 dbd9c2a
@stojg
stojg commented Sep 16, 2013

I've added these tests to stojg@dbd9c2a if anyone would like to test this out easily.

@stojg
stojg commented Sep 17, 2013

I found a way to hack this in a onBeforeWrite by some trickery.

public function onBeforeWrite() {       
    $versioned = $this->getExtensionInstance('Versioned');
    if(!$versioned) {
        return parent::onBeforeWrite();
    }

    // Don't change the content yet, because this will not update the version on the record
    if($versioned->_nextWriteWithoutVersion) {
        return parent::onBeforeWrite();
    }

    $this->Content = '';
    foreach ($this->Widgets() as $widget) {
        $this->Content .= $widget->Content;
    }
    parent::onBeforeWrite();
}
@simonwelsh simonwelsh added the 3.1 label Mar 15, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.