Release 1.x.x regroup

[mfendeksilverstripe]

Release 1.x.x regroup

This is a realy useful module but it's technically still unfinished. It would be great if we can identify the missing pieces and try to push this module over the line.

Current state assessment

Code review done on master branch (61c9c5bbece843ca8df0b9768d0bdc0f71778dd1).

One of the main issues with this module is the unclear scope of the intended Versioned history integration. This module is supposed to provide an override to the default history viewer, providing richer and more performant experience. This includes exposing versioned history events of nested objects (like blocks) and providing extensibility options for other modules (like Fluent).

More specifically, this covers:

• Version history viewer tab itself which exposes events from nested objects
• Action button state enhancement in Edit form which uses the snapshot records instead of versioned history records as the source of truth
• Site tree flag status enhancement which uses the snapshot records instead of versioned history records as the source of truth

As far as I can see there are multiple ways how to approach this.

Option 1 - History viewer only

We could scale back this module to only cover history viewer itself, leaving the button states and site tree flags to be dealt with by other modules. This would make the default versioned history records the source of truth not the snapshot records.

• concerns of nested objects stage state could be addressed by introducing stagesDifferRecursive() to the Versioned module
• concerns of localised versioned history records can be left to the Fluent module as Fluent 6 handles these correctly

Pros

• potentially less compatibility issues with other modules as the scope is smaller

Cons

• we are likely losing a very useful part of the functionality of this module
• performance of the stagesDifferRecursive() is a concern as is scales with the number of objects in the traversal tree unlike the snapshot records

Follow up actions

• remove any edit form UI enhancement related code as it's no longer needed

Option 2 - UI enhancement level integration

Covers version history viewer and UI enhancements such as button states and site tree flags. This seems to be the option closest to the current state.

Pros

• UI enhancement is not very invasive so it's less likely to break any core functionality

Cons

• potentially inconsistent behaviour as only a few UI components are covered by this enhancement, other components which use stagesDiffer() may yield different (unexpected) results

Follow up actions

• there is currently no UI enhancement for GridFieldItem edit form so we need to add this
• there is quite a bit of raw queries in the code which makes the code not very extensible, we should improve this by refactoring the code, reviewing the method scope and add new extension points

Option 3 - deep integration

Covers version history viewer and override for stagesDiffer() via its extension point. This indirectly updates the state of all related UI components such as publish button state.

Pros

• consistent behaviour across the board
• UI is updated indirectly in one place, so it's easier to manage

Cons

• consistency of this behaviour may be unwanted in some cases like specific unit tests which lack snapshot data (this can be mitigated by providing a global state that can turn off the stagesDiffer() enhancement)
• there may be conflict with other modules like Fluent which uses the same extension point (this can be mitigated by documenting the correct order of extensions)

Follow up actions

• hook up hasOwnedModifications() to updateStagesDiffer extension point
• remove UI enhancements for buttons states and site tree flags as they are covered by stagesDiffer() enhancement
• there is quite a bit of raw queries in the code which makes the code not very extensible, we should improve this by refactoring the code, reviewing the method scope and add new extension points

Other issues

• Performance: Cache hasOwnedModifications() #28
• silverstripe/graphql doesn't have a tagged release for 4 other than RCs

Feedback is appreciated.

[mfendeksilverstripe]

After some analysis the general agreement is to go for Option 2. The main reasoning is that:

• Option 1 gets rid of useful functionality
• Option 3 doesn't work properly as the average snapshots coverage is not high enough to be used for operations like stagesDiffer()