RFC: User-action snapshots

[unclecheese]

Explanation of problem

Right now, snapshot creation is a side-effect of an onAfterWrite() hook for nearly all DataObjects with explicit ownership. Most of the time, a write event is one-to-one with a user action -- for instance, "publish page" or "edit gridfield item" usually map to a single write event.

There are cases, however, where this doesn't hold up:

• A related dataobject is created and then paired with a parent (create, edit), particularly in has_many
• The $owns declaration of the dataobject creates implicit writes for its owned objects whose isChanged() checks fail for any number of reasons.
• Objects are written in a non-publishing context (e.g. copying translations)
• Unrelated dataobjects are created as a side effect of a write (e.g. a log)
• Many more examples..

All of these have the undesirable effect of polluting the snapshot UI with irrelevant and/or confusing information, and creating a performance penalty for these unnecessary snapshots.

In the case of a has_many, when a user adds a new related dataobject, they get two snapshots -- one for the creation of the dataobject, and one for modification, when it is assigned a parent ID.

In the case of $owns declarations, the write() events are expected, but the actual database mutation should be thwarted on the strict isChanged() check. This check is easily defeated by absent fields being added with null values, or null values being transformed to 0, etc. While is should be a robust and dependable calculation, it shouldn't be the backstop for an operation that will trigger multiple unnecessary writes and snapshots along with them.

These are hard problems to solve, and they would go away if Snapshots were sympathetic to user actions rather than programatic writes.

Rough proposal

The snapshot module should be aware of the record the user is editing. For lack of a better term, we'll call this the "context object." This would basically be global state, much like the "open snapshot" is now. It ensures that only snapshots with an OriginHash === ContextHash would be written.

Having experimented with this, it seems like the best way to capture the context object is in a shared low-level class, like LeftAndMainFormRequestHandler. When forms are submitted, the global state is set that the user is editing an object of type X. From there, openSnapshot() only works when the origin object is the same as the context object.

ex:

public function httpSubmission($request)
{
  SnapshotPublishable::setContext($obj);
  parent::httpSubmission($request);
  SnapshotPublishable::persist();
}

Where $obj comes from in that example is an open question, but it seems realistic to pull it off of Form::getRecord(), as most of them have loadDataFrom invoked on them. The persist() method would do some kind of reconciliation to ensure a logical set of snapshots is created. For instance, this is where the create/edit duplicity of a has_many would be cleared up. Ideally it would have extension points, as well.

Programatic writes

By default, without a context set, snapshots are not created, so programatic writes will not create snapshots. Most of the time, this is desirable. However, to allow snapshots programatically, we could do so in a closure:

SnapshotPublishable::withSnapshot(DataObject $obj, function () {
  // All the writes in here will trigger snapshots when they are of type $obj.
 // After execution, the context is reset to empty.
});

Challenges

• How can we reliably determine the context object across all admin UIs without having to write special functions and hooks for every new user action?
• Some user actions should be able to bypass snapshots (currently supported by pause, but this would be a cleaner implementation)
• All the tests will break, as they rely on programatic writes. User action based snapshots would make a strong case for adding behat tests.

[chillu]

Form submissions would cover most current cases, but there's still a fair bit of other writes triggered through user actions outside of that:

• GraphQL mutations (e.g. in Assetadmin)
• Controller actions (e.g. admin/pages/savetreenode)
• Batch actions (e.g. batch publishing files, using GridField plugins)
• Forms with custom logic (not using $Form->record)

Due to the way ownership writes work, we can't rely on "first object written is the origin object" - there might be owned relationships which are written first. And in the case of batch actions, there's no single origin object in the first place.

To me, it's a question if we want all controllers with object writes to state their context, or only some of them. There are implications of less than 100% coverage on this (which we have at the moment by the low-level write integration). Objects written through multiple places can get into inconsistent state. ControllerA with snapshot support writes ObjectA, marking owners as changed. ControllerB without snapshot support publishes ObjectA, but owners are still marked as changed because no new snapshot has been written.

I think simply by adding this to core controllers (incl. GridField), we'll cover most of the cases. Even pretty advanced use cases like inline cell editing through GridField extensions uses Form->saveInto().

Maybe let's go through some addons and check how they'd be affected if we implemented the changes as discussed? See how much impact there is, how much likelyhood to creating inconsistent state.