Decouple default Injector configuration for CMSPageHistoryController

[robbieaverill]

It's likely that SilverStripe 4.2 is going to be released without this module included by default, but we'd still like to be able to release this module with dnadesign/silverstripe-elemental so it provides useful history for content blocks. We won't be shipping it by default with 4.2 until it has compare mode (https://github.com/silverstripe/silverstripe-versioned/issues/96).

Currently this module will overload the CMSPageHistoryController by default via Injector configuration.

For us to be able to include this module in the CWP 2.1 recipe (which will include SilverStripe 4.2, and the content blocks recipe) we'll need to remove this default Injector configuration to ensure the legacy history functionality still works for the CMS.

We'd still need to have this module enabled when viewing History for a BlocksPage, to ensure that the top level summary of content block history in a page (https://github.com/dnadesign/silverstripe-elemental/issues/181) works for these page types, while not interfering with regular CMS pages.

We could move the existing configuration (overloading CMSPageHistoryController) to an example in documentation or YAML configuration, e.g. "this is how you can enable it for all page types if you want to".

Acceptance criteria

• I can install versioned admin on a project running SilverStripe 4.2 or CWP 2.1
• When installed, HistoryViewer is activated for BlocksPage pages
• HistoryViewer is activated for all content blocks (BaseElement)
• HistoryViewer is NOT activated for any page type other than BlocksPage (by default)
• Legacy CMS history still works for non BlocksPage page types with versioned-admin installed

Considerations

• Be careful with the template overrides in this module, as they may override the legacy CMS history tab depending on how we implement this logic.
• The "use HistoryViewer instead of legacy CMS controller" injector logic is currently page-type agnostic. We'll need to look at how to do this.

cc @chillu @clarkepaul @be2n

Pull requests

• [x] https://github.com/silverstripe/silverstripe-versioned-admin/pull/18
• [x] https://github.com/dnadesign/silverstripe-elemental/pull/231

[robbieaverill]

cc @be2n @tractorcow @chillu @unclecheese

There's some conflicting views about whether we should actually do this. @tractorcow is concerned that it's introducing some fragility, that the HTTPRequest params we're depending on have no context and assume that the ID param is present and is a page ID (this relates to #18) and that this isn't necessarily the right way to get the page ID.

@tractorcow's suggestion was initially to overload the CMSPageHistoryController and handle fallbacks to default behaviour with conditional checks - we had this in place with #18 originally, but have now converted it to an injector factory mechanism instead. The other suggestion was to leave it as enforcing the history viewer to be active when the module is installed, and suggest that if you don't want it then you should uninstall it.

To recap: the reason we'd originally discussed making it optional when installed is because not having compare mode in the CMS would mean it's a regression in features when included in CWP, but it would be equal to the current elemental history functionality.

[tractorcow]

Oh well, I guess it's already decided @robbieaverill ? @unclecheese merged it.

[robbieaverill]

I'd like to have some discussion around this before we say it's done 😄 cc @chillu @tractorcow @unclecheese @be2n

[unclecheese]

Have chatted to @robbieaverill and the rest of the team about this. Have weighed the cost/benefits of both approaches, and feel the current implementation is suitable given the context.

Sorry for jumping the gun on the merge! This had been stale for a while, and I was a bit too keen to get it over the line.