Too many versions per save action

[NightJar]

Minimum steps to reproduce
composer create-project silverstripe/recipe-cms test ^4
app/src/AnThing.php

<?php

use SilverStripe\ORM\DataObject;
use SilverStripe\Versioned\Versioned;

class AnThing extends DataObject
{
    private static $db = ['Title' => 'Varchar'];
    private static $has_one = ['Page' => Page::class];
    private static $extensions = [Versioned::class];
}

app/src/Page.php

<?php

namespace {

    use SilverStripe\CMS\Model\SiteTree;
    use SilverStripe\Forms\GridField\GridField;
    use SilverStripe\Forms\GridField\GridFieldConfig_RelationEditor;
    
    class Page extends SiteTree
    {
        private static $db = [];
        
        private static $has_many = ['Things' => AnThing::class];
        
        public function getCMSFields()
        {
            $fields = parent::getCMSFields();
            $fields->addFieldToTab(
                'Root.TheThings',
                GridField::create(
                    'Things',
                    'Things',
                    $this->Things(),
                    GridFieldConfig_RelationEditor::create()
                )
            );
            return $fields;
        }
    }
}

Build site.
Load CMS.
Edit Home.
The Things tab
Add An Thing
Enter a Title
Create
-> Database shows one version. ✅ (new record, first version)
Change title.
Publish.
-> Database shows three versions. ✅ (two versions created for two actions {save, publish})
Alter title.
Save.
-> Database shows five versions ❌ (two versions created for one action {save})
Alter title.
Publish.
-> Database shows seven versions ✅ (two versions created for two actions {save, publish})
Alter title.
Save.
-> Database shows nine versions ❌ (two versions created for one action {save})
do not alter title.
Save.
-> Database shows ten versions ✅ (one new version for one action {publish})

silverstripe/admin                 1.1.2   SilverStripe admin interface
silverstripe/asset-admin           1.1.2   Asset management for the SilverStripe CMS
silverstripe/assets                1.1.2   SilverStripe Assets component
silverstripe/campaign-admin        1.1.2   SilverStripe campaign admin interface
silverstripe/cms                   4.1.2   The SilverStripe Content Management System
silverstripe/config                1.0.4   SilverStripe configuration based on YAML and class statics
silverstripe/errorpage             1.1.2   ErrorPage component for SilverStripe CMS
silverstripe/framework             4.1.2   The SilverStripe framework
silverstripe/graphql               1.1.2   GraphQL server for SilverStripe models and other data
silverstripe/recipe-core           1.1.2   SilverStripe framework-only core recipe
silverstripe/recipe-plugin         1.3.0   Helper plugin to install SilverStripe recipes
silverstripe/reports               4.1.2   Reports module for SilverStripe CMS
silverstripe/siteconfig            4.1.2   Site wide settings administration.
silverstripe/vendor-plugin         1.3.3   Allows vendor modules to expose directories to the webroot
silverstripe/versioned             1.1.2   SilverStripe Versioned component

[NightJar]

This issue seems to manifest itself more seriously (read: worsens) in silverstripe/recipe-cms: 4.2.0-beta1 where every save results in 2 records being written (i.e. the set of actions {save, publish} results in 3 new version records instead of 2 as above).

of course the modified files are now app/src/{AnThing,Page}.php
Following on the above steps from "Enter a Title":

Publish.
-> Database shows two versions. ✅ (two versions created for two actions {save,publish})
Alter title.
Publish.
-> Database shows five records ❌ (three versions created for two actions {save,publish})
Alter title.
Save.
-> Database shows seven records ❌ (two versions created for one action {save})
Publish (no alteration).
-> Database shows nine records ❌ (two versions created for one action {publish})

silverstripe/admin                 1.2.0-beta1 SilverStripe admin interface
silverstripe/asset-admin           1.2.0-beta1 Asset management for the SilverStripe CMS
silverstripe/assets                1.2.0-beta1 SilverStripe Assets component
silverstripe/campaign-admin        1.2.0-beta1 SilverStripe campaign admin interface
silverstripe/cms                   4.2.0-beta1 The SilverStripe Content Management System
silverstripe/config                1.0.5-beta1 SilverStripe configuration based on YAML and class statics
silverstripe/errorpage             1.2.0-beta1 ErrorPage component for SilverStripe CMS
silverstripe/framework             4.2.0-beta1 The SilverStripe framework
silverstripe/graphql               2.0.0-beta1 GraphQL server for SilverStripe models and other data
silverstripe/recipe-core           4.2.0-beta1 SilverStripe framework-only core recipe
silverstripe/recipe-plugin         1.3.0       Helper plugin to install SilverStripe recipes
silverstripe/reports               4.2.0-beta1 Reports module for SilverStripe CMS
silverstripe/siteconfig            4.2.0-beta1 Site wide settings administration.
silverstripe/vendor-plugin         1.3.3       Allows vendor modules to expose directories to the webroot
silverstripe/versioned             1.2.0-beta1 SilverStripe Versioned component

[NightJar]

Initial discovery by @robbieaverill at silverstripe/cwp#118

[NightJar]

Interesting above that a publish without making changes (with recipe 4.2.0-beta1) behaves as if a save has happened first. This info may help narrow the search field - although the major manifestation is on the save action (without a publish) - so perhaps the problem exists somewhere in the crossover of the two actions

(LastEdited column can be used in conjunction with PublisherID to distinguish what action happened at what time)

[maxime-rainville]

I just gave a go at replicating this and did manage to do it. I tried with a Dummy DataObject and a SiteTree object and got the same result

If I edit an object and save it a get one new entry in my version table. If I publish my draft version, I get one new entry in my version table. If I edit an object and publish it without saving it first, I get two new versions.

Do you have any ownership relations between your objects?

[NightJar]

Nope. No changes to code were made between first test (4.1.1) and second test (4.2.0-beta1). Everything was as in OP.

[dhensby]

I'm pretty sure this has been reported before and @tractorcow said it was intended

[dhensby]

This was the issue I was thinking about: #136 maybe not an exact duplicate, but sounds similar enough.

[tractorcow]

We have a known limitation where we have to do an initial write (draft only version), and THEN send the whole object into a publish changeset, where it creates a second version (draft + live) alongside all other objects in that changeset.

What we could do to fix this is allow unsaved items to be added to changesets. That way $object->publishRecursive() could be called on unsaved objects, and NOT require ->write() to be called first.

It'll require a bit of magic, since a changeset / changesetitem instance will need to be passed through the publishing lifecycle. You may need to assign a kind of "temp" relation on changeset that holds all the unpublished records being written in this transaction, maybe with a temp ID? (non-numeric id like unsaved-1). It needs some solution architecture there...

[tractorcow]

Sorry that may not address the "three versions" created issue, but it does combine at least two of them into one.

The third is just a dumb logic in gridfield somewhere, probably.

[tractorcow]

The new logic in 4.2 is the deleted version tracking and draft/published state of each version.

[phptek]

My 2c worth is that I can also confirm the same behaviour on SiteTree objects in the following versions:

• Site 1: CWP 2.2.1
• Site 2: Non CWP (silverstripe/framework 4.2.1 via silverstripe/recipe-cms 4.2.1)

[phptek]

I see something interesting if I decorate both File and Image with VersionedExtension (rightly or wrongly, the point is I can) then after 2 publish operations (I think) I got 5 versions: 1-5 in the "File_Versions" table but only 1-3 & 5 in "Image_Versions".

[NightJar]

@phptek this is because File (and thus Image) already has Versioned applied to it. By applying it again you're running the extensions methods twice per hook point.
Interesting that the version numbers mismatch though - are you sure that all these records are for the same actions? How do they join up?

[phptek]

@NightJar Yeah, I'm torn between "It shouldn't be allowed" (Throw an exception if someone tries to do this) and "Let them shoot themselves in the foot ala "rm -rf /".

are you sure that all these records are for the same actions?

Fairly sure...if I get the chance I'll update, but if this isn't supposed to happen as you describe, then it's kinda moot.

[chillu]

We really need to address this when implementing version snapshots as described in #195 soon - every save and publish operation will become a lot more expensive this way.

[chillu]

We have a known limitation where we have to do an initial write (draft only version), and THEN send the whole object into a publish changeset, where it creates a second version (draft + live) alongside all other objects in that changeset.

I can't reproduce this on 1.x-dev - after the steps described, I end up with 7 versions, not 10. I can't really see anything in the versioned commits in the lsat half year which would indicate that's been explicitly fixed.

So the system seems to work as designed now? Can anyone clarify why we are creating two versions on a publish with unsaved changes, one for draft, immediately followed by a live one? We can't call writeWithoutVersion() in $object->publishRecursive(), followed by publish() which then creates a single version? This would be a potential breaking change to our data model, but we need to find a way to reduce creation of versions since that's getting a lot more expensive now. Do we rely anywhere on the distinction between "draft saved", and "draft saved, then published immediately"?

On create:

• DataObject->writeWithoutVersion(), followed by Form->saveInto()
• DataObject->write(), followed by Versioned->augmentWriteVersion() (creates draft version)

On first save and publish:

• DataObject->writeWithoutVersion(), followed by Form->saveInto()
• DataObject->write() followed byVersioned->augmentWriteVersion() (creates draft version)
• DataObject->publishRecursive()
• ChangeSet->write()
• ChangeSet->publish()
• ChangeSetItem->publish()
• ChangesetItem->getObjectInStage(Versioned::DRAFT) performs a Versioned::get_by_stage(), and gets a duplicate in memory object
• DataObject->publishSingle()
• DataObject->writeToStage(static::LIVE), followed by DataObject->write() (creates live version)

I now understand what @tractorcow comment refers to with "unsaved objects". They exist in the database, but have unsaved changes. Without this first draft version write, it would be "unsaved" at the point when ChangeSetItem retrieves it from the database, rather than using the existing in-memory object with up-to-date data.

I don't think it needs a "temp ID" as suggested by @tractorcow. Records are always saved (through CMSMain and GridField) before publishRecursive() is called, so they have a database identifier already. We don't expose "create and publish" as a single action in the UI. If we did, we'd need to ensure it also calls write() first. We do need to get those in-memory objects into the ChangeSet data structure though, instead of querying outdated states from the database.

My suggestion is that we create ChangeSetItem->setTempObject($obj, $stage), and default to using that over Versioned::get_by_stage() throughout it's API. We could frame this as a prepopulated cache as well, but it would violate dev assumptions on how caches work (can be reinstated from database?).

This might also be solved by an identity map as suggested in the eager loading RFC, if we create this map independently from relationships - I don't think that's the case yet, and probably a larger effort. It would also require that this single in-memory object would carry through unsaved changes which aren't persisted in the database yet, which might get horribly confusing.

Env info:

composer info | grep -i silverstripe
silverstripe-themes/simple         3.2.0              The SilverStripe simple theme (default SilverStripe 3 theme)
silverstripe/admin                 1.x-dev f3c0a16    SilverStripe admin interface
silverstripe/asset-admin           1.x-dev dd285d4    Asset management for the SilverStripe CMS
silverstripe/assets                1.x-dev 233eab6    SilverStripe Assets component
silverstripe/campaign-admin        1.x-dev 0ebc16c    SilverStripe campaign admin interface
silverstripe/cms                   4.x-dev 60aadf8    The SilverStripe Content Management System
silverstripe/config                1.0.x-dev ab03d6d  SilverStripe configuration based on YAML and class statics
silverstripe/errorpage             1.x-dev e51ec25    ErrorPage component for SilverStripe CMS
silverstripe/framework             4.x-dev 7cb8256    The SilverStripe framework
silverstripe/frameworktest         dev-master 6dd60fd Aids core and module developers in testing their code against a set of sample...
silverstripe/graphql               3.x-dev 3d403f2    GraphQL server for SilverStripe models and other data
silverstripe/graphql-devtools      dev-master 2af93f7 Tools to help developers building new applications on SilverStripe’s GraphQ...
silverstripe/recipe-cms            4.x-dev d616bc7    SilverStripe recipe for fully featured page and asset content editing
silverstripe/recipe-core           4.x-dev 83fb598    SilverStripe framework-only core recipe
silverstripe/recipe-plugin         1.3.0              Helper plugin to install SilverStripe recipes
silverstripe/reports               4.x-dev ed3ffee    Reports module for SilverStripe CMS
silverstripe/siteconfig            4.x-dev f5e1de9    Site wide settings administration.
silverstripe/vendor-plugin         1.3.3              Allows vendor modules to expose directories to the webroot
silverstripe/versioned             1.x-dev 1d902d3    SilverStripe Versioned component
silverstripe/versioned-admin       1.x-dev ee023e9    SilverStripe versioned admin interface

[chillu]

No feedback, closing