Default workflow states and transitions

The problem is to do with ordering. Yay for the default config test :)

I think we need to translate the default config...

Questions and comments.

1. +++ b/core/modules/content_moderation/src/Plugin/WorkflowType/ContentModeration.php
   @@ -145,12 +145,59 @@ public function addEntityTypeAndBundle($entity_type_id, $bundle_id) {
   +        'label' => $this->t('Draft'),
   

   Are you sure this needs translating. The config schema defines the key as "type: label" which is already translatable once it's stored in config.

2. +++ b/core/modules/content_moderation/src/Plugin/WorkflowType/ContentModeration.php
   @@ -145,12 +145,59 @@ public function addEntityTypeAndBundle($entity_type_id, $bundle_id) {
   +        'from' => [
   +          'draft',
   +          'published',
   +          ],
   

   funky indentation.

3. +++ b/core/modules/content_moderation/src/Plugin/WorkflowType/ContentModeration.php
   @@ -145,12 +145,59 @@ public function addEntityTypeAndBundle($entity_type_id, $bundle_id) {
   +      ]
   

   nit, trailing comma

4. +++ b/core/modules/content_moderation/tests/src/Functional/ContentModerationWorkflowTypeTest.php
   @@ -43,13 +43,13 @@ public function testNewWorkflow() {
          'id' => 'test_workflow',
          'workflow_type' => 'content_moderation',
        ], 'Save');
   -    $this->assertSession()->pageTextContains('Created the Test Workflow Workflow. In order for the workflow to be enabled there needs to be at least one state.');
   

   Should we stick some asserts in here to make sure the default states show up in the UI?

5. +++ b/core/modules/content_moderation/tests/src/Functional/ContentModerationWorkflowTypeTest.php
   --- a/core/modules/content_moderation/tests/src/Kernel/ContentModerationPermissionsTest.php
   +++ b/core/modules/content_moderation/tests/src/Kernel/ContentModerationPermissionsTest.php
   
   +++ b/core/modules/content_moderation/tests/src/Kernel/ContentModerationPermissionsTest.php
   @@ -55,36 +55,14 @@ public function permissionsTestCases() {
   -          'transitions' => [
   -            'publish' => [
   -              'label' => 'Publish',
   -              'from' => ['draft'],
   -              'to' => 'published',
   -              'weight' => 0,
   -            ],
   

   I don't think it's a bad thing to decouple tests like this from module defaults, if that's possible.

6. +++ b/core/modules/workflows/src/Entity/Workflow.php
   @@ -111,6 +112,23 @@ class Workflow extends ConfigEntityBase implements WorkflowInterface, EntityWith
   +  public function preSave(EntityStorageInterface $storage) {
   

   missing docblock

7. +++ b/core/modules/workflows/src/Entity/Workflow.php
   @@ -111,6 +112,23 @@ class Workflow extends ConfigEntityBase implements WorkflowInterface, EntityWith
   +  public function preSave(EntityStorageInterface $storage) {
   +    $plugin = $this->getTypePlugin();
   +    $default_states = $plugin->defaultStates();
   +    foreach ($default_states as $state_id => $default_state) {
   +      if (!$this->hasState($state_id)) {
   +        $this->addState($state_id, $default_state['label']);
   +      }
   +    }
   +    $default_transitions = $plugin->defaultTransitions();
   +    foreach ($default_transitions as $transition_id => $default_transition) {
   +      if (!$this->hasTransition($transition_id)) {
   +        $this->addTransition($transition_id, $default_transition['label'], $default_transition['from'], $default_transition['to']);
   +      }
   +    }
   +    parent::preSave($storage);
   +  }
   

   Would it be more consistent with the behaviour of defaults in general to not add any states or transitions if a Workflow is created with any of this information already. Feels like it would be strange getting two extra states if you already intended to create some.

   Edit: also, I think this might have some implicit coverage based on the permissions test, but might be good to spin off a test to explicitly cover this.

8. +++ b/core/modules/workflows/src/Form/WorkflowAddForm.php
   @@ -84,11 +84,16 @@ public function form(array $form, FormStateInterface $form_state) {
   +    else {
   +      $form_state->setRedirectUrl($workflow->toUrl('edit-form'));
   +    }
   

   What about a success message here as well?

Re: #6.1, once the config is saved, isn't it already going to be translated back out in the right language by the config system? Wouldn't storing it in a non english language actually break translations? I might see if I can replicate this in a test, could just be thinking about the wrong.

Re: #6.7, if I go Workflow::create(['states' => [...]]), do I still get the two defaults? I would expect not to see them.

Right, my point is if you're passing in some states to be created, you probably don't want two extra ones added at the end.

Okay, my misunderstanding, I assumed users would be free to remove these if they chose and it would just be an initial seed. Reread the issue summary more closely.

I did get around to testing the language stuff and the test confirmed wrapping in t() is indeed correct, my bad! Attached interdiff will be green on top of #15 if you're curious, but I think given it confirms something that we can probably take for granted based on the config system as an abstraction, it should probably be left out.

Side note: figuring out how to test this was quite tricky, but also TIL a lot about translation.

Reviewing this now.

The functionality works great and solves the issue!

I have also reviewed the code and it looks solid. Two small changes:

• Fixed a phpcs complaint around namespacing in the entity operations
• Modified the form to match the entity operations alter so the delete button is not given as an option for states that are not deletable

1. +++ b/core/modules/content_moderation/content_moderation.module
   @@ -122,6 +122,29 @@ function content_moderation_form_alter(&$form, FormStateInterface $form_state, $
   +  if ($form_id == 'workflow_edit_form') {
   +
   +    // Get default states.
   +    $workflow_type_plugin_manager = \Drupal::service('plugin.manager.workflows.type');
   +    $default_states = $workflow_type_plugin_manager->createInstance('content_moderation')->defaultStates();
   +
   +    // Remove delete option on undeletable default states since they are not
   +    // deletable.
   +    foreach ($form['states_container']['states'] as $state => $data) {
   +      if (!is_array($data) || !is_array($data['operations'])) {
   +        continue;
   +      }
   +
   +      // Check if this operation is one of the default states.
   +      if (isset($data['operations']['#links']['delete'])) {
   +        if (in_array($state, array_keys($default_states))) {
   +          unset($data['operations']['#links']['delete']);
   +          $form['states_container']['states'][$state] = $data;
   +        }
   +      }
   +    }
   +  }
   

   This shouldn't be necessary. If we make the changes below.

2. +++ b/core/modules/content_moderation/content_moderation.module
   @@ -193,6 +216,18 @@ function content_moderation_node_access(NodeInterface $node, $operation, Account
    /**
   + * Implements hook_entity_access().
   + */
   +function content_moderation_entity_access(EntityInterface $entity, $operation, AccountInterface $account) {
   +  if ($operation == 'delete-state') {
   

   I think we should use hook_ENTITY_TYPE_access() so that this limited to workflows. But actually we already have the opportunity to do this in \Drupal\content_moderation\Plugin\WorkflowType\ContentModeration::checkWorkflowAccess().

   One of the problems here is that we can't pre-work out what state is being deleted. Maybe we could make the operation delete-state:STATE_ID. That way we could parse the state from the operation. It might be good to get an opinion from another entity API maintainer here.

3. +++ b/core/modules/workflows/src/Entity/Workflow.php
   @@ -114,6 +115,26 @@ class Workflow extends ConfigEntityBase implements WorkflowInterface, EntityWith
   +    $plugin = $this->getTypePlugin();
   +    $default_states = $plugin->defaultStates();
   +    foreach ($default_states as $state_id => $default_state) {
   +      if (!$this->hasState($state_id)) {
   +        $this->addState($state_id, $default_state['label']);
   +      }
   +    }
   +    $default_transitions = $plugin->defaultTransitions();
   +    foreach ($default_transitions as $transition_id => $default_transition) {
   +      if (!$this->hasTransition($transition_id)) {
   +        $this->addTransition($transition_id, $default_transition['label'], $default_transition['from'], $default_transition['to']);
   +      }
   +    }
   +    parent::preSave($storage);
   

   This just ensures that the states and transitions exist - not that they have the expected values... think if someone makes the published state be unpublished? We have protection against this in the form but not in the API.

   I'm still not sure that this is the right place to do this.

4. We should have tests for things like the form element disabling

Could we simplify the access issues and avoid the dynamic operations like delete-state:STATE_ID by allowing deletion of any state and simply either:

1. Putting a warning if the state is missing at least 1 published and 1 non-published state
2. and optionally having an Active/Disabled state for workflows, auto disabling (and not allowing activating) if (1) is not met

+++ b/core/modules/content_moderation/content_moderation.module
@@ -122,6 +122,29 @@ function content_moderation_form_alter(&$form, FormStateInterface $form_state, $
+      if (!is_array($data) || !is_array($data['operations'])) {
+        continue;
+      }
+
+      // Check if this operation is one of the default states.
+      if (isset($data['operations']['#links']['delete'])) {
+        if (in_array($state, array_keys($default_states))) {
+          unset($data['operations']['#links']['delete']);
+          $form['states_container']['states'][$state] = $data;
+        }
+      }

could you avoid to use the continue?

@alexpott I think is better fix this /issue/ first and then if someone expected or want the behavior you describe he can create another issue to fix that? I'm not saying ignore it.. just what @scott_euser said make sense to me.

One thing I think I mentioned on IRC but failed to add to the issue was the use of "default". All other contexts that have an interface like this imply that the config can be changed after it's installed, but that isn't the case here. These fit more inline with terminology like "enforced".

I think there is a variation on the way to do this which would make things more flexible and solve two other use cases at the same time:

• Allowing modules to provide default states/transitions that are installed and can be deleted.
• Allow modules/install profiles to add states/transitions that can’t be deleted, for any other type plugin (without introducing their own).

The proposal would be to keep the same methods and interface but introduce a new key called "enforced" on both states and transitions. Once that key exists, all of the existing constraints in the patch are there, just for those states. It might looks something like:

   /**
+   * {@inheritdoc}
+   */
+  public function defaultStates() {
+    return [
+      'draft' => [
+        'label' => $this->t('Draft'),
+        'enforced' => TRUE,
+      ],
+      'published' => [
+        'label' => $this->t('Published'),
+        'enforced' => TRUE,
+      ],
+    ];
+  }

That way, if I'm an install profile I can ship my config for the content moderation workflow with as many states as I like, enforce them all and then be assured they will exist in code. Similarly, a new CM plugin can come along, implement this method, omit the enforced key and then these act like other default* methods in Drupal with config that can be overriden and deleted.

I like that idea @Sam152, I guess the challenge we need to first overcome is preventing deletion in reaction to the operation in a robust way. I can't see how to implement delete-state:STATE_ID that alexpott suggests, but perhaps someone can point in the right direction as to how to make dynamic operations.

Alternatively, to avoid getting too stuck on the delete aspect of this issue, we can go down the warn / deactivate route I suggested and avoid the deletion protection issue altogether?

Attached is what I am proposing, but if we would go down this route, it still needs work to stop the workflow from being attached to the entity if !$workflow->status(). This uses the core ConfigEntityBase enable / disable / status methods. I imagine there is an easy way to stop the workflow from being attached, perhaps in core/modules/content_moderation/src/EntityTypeInfo.php

The approach suggested in #27does make sense, but I don't think it meets the needs of the three scenarios mentioned in the issue summary, that is, actually having something concrete to rely on. Disabling or unpublishing a workflow doesn't mean the content that was using that workflow previously doesn't still need to be accounted for in a reasonable way.

Here's a approach that allows us to prevent deleting states in default states via the UI using the access system. I'm not convinced about the disabling solution because we still have to decide what happens to existing content entities that become under a workflow once it is enabled or disabled. That feels complex. I think that mandating the default states has a lot of benefits. Basically we need to ensure the content moderation can handle what the most important content entity (node) supports by default ie. unpublished and published. And there is a thought that content moderation should only work on entity types that are for entities which implement EntityPublishedInterface which makes sense to me.

Interdiff to #19.

I'm not sure about the use case for default transitions. It's a bit more complex and not required for many of the problems that doing this allows us to fix - ie. what to do with existing content entities when we put them under moderation.

Nice one with the WorkflowDeleteAccessCheck, I really couldn't figure out how to do that myself!

If there a case where someone might want to use workflows for some purpose that might not be typical (ie, might not have a non-published state), I suppose they can implement WorkflowTypeInterface in their own way (ie, not use content_moderation).

@scott_euser or set the published state as the initial state and prevent transitions to the unpublished state. This is kind of how core is configured out of the box when content_moderation is not installed.

I've reviewed the patch. I ran into an error where after adding a 3rd state, the Published default state would also get a delete button. Adding $links = []; at the start of the for loop where the operations are added to the rows in states container solves that.

diff -u b/core/modules/workflows/src/Form/WorkflowEditForm.php b/core/modules/workflows/src/Form/WorkflowEditForm.php
--- b/core/modules/workflows/src/Form/WorkflowEditForm.php
+++ b/core/modules/workflows/src/Form/WorkflowEditForm.php
@@ -43,7 +43,7 @@
     $header = [
       'state' => $this->t('State'),
       'weight' => $this->t('Weight'),
-      'operations' => $this->t('Operations')
+      'operations' => $this->t('Operations'),
     ];
     $form['states_container'] = [
       '#type' => 'details',
@@ -79,6 +79,7 @@
     }
 
     foreach ($states as $state) {
+      $links = [];
       $links['edit'] = [

I cleaned up a couple minor phpcs complaints my editor was throwing.

Finally I added a Save and edit button; I think it is important for the UX when you reorder states or transitions that you can save and remain on the same page (similar to when editing a view and there are unsaved changes, you can remain on the same page).

Otherwise all worked well for me in testing.

@scott_euser nice find on links! Can you add an automated test for that? Plus the other changes might be good but they are out-of-scope here - please don't mix unrelated UI and coding standard changes in this change - just create new issues to discuss these separate things.

Okay sounds good, thanks for the feedback; still new to how things work, especially with core. Will try to send an updated patch tomorrow morning.

Discussed with @Sam152 and @timmillwood in IRC. The mix of defaultConfiguration(), defaultStates(), defaultTransitions() in latest patch is not very nice. Going to explore using just defaultConfiguration and making the plugin responsible for all the state and transition configuration. This will also impact transition/state decoration - it might end up making everything a bit simpler :)

Makes sense and simplicity sounds like a good thing here.

Here's step 1 - moves all the state and transition configuration to be part of the workflow type config. This results in use not having to duplicate state IDs in core/modules/content_moderation/config/install/workflows.workflow.editorial.yml which is really nice. The next step of this will be to move how the classes used for decorating states and transitions to the annotation as well - and generalise the ability to get states and transitions to WorkflowTypeBase. And after that instead of having all the setStateLabel etc on Workflow just have addState, deleteState, updateState which should all just accept a state object (same for transitions). We'll end up with way less methods. These steps are necessary to solve this issue but they are implied by the change made to solve this issue because now the plugin has the complete state and transition configuration. My preference would be to do the necessary to implement required states and have followups for the next steps since they should not have data structure change at all. Just code and annotation change.

So @scott_euser there's some way to go to get things to be as simple as they can be :)

I need to add further test coverage before we're done here.

There's no interdiff because whilst some of the code is the same it is just distracting from the bulk of the change cause this is new approach.

The patch contains test coverage for the bug noticed by @scott_euser in #36.

Here's some kernel test coverage of the new required_states annotation and the population of the default configuration - separate from the implied coverage of having the content_moderation module.

This fixes the sorting and merging issues due to default configuration and adds more test coverage.

+++ b/core/modules/workflows/src/Entity/Workflow.php
@@ -502,7 +507,34 @@ protected function getNextWeight(array $items) {
+  /**
+   * Keeps the plugin configuration in sync.
+   */
+  protected function updatePluginConfig() {
+    if ($this->pluginCollection) {
+      $this->pluginCollection->setConfiguration($this->settings);
+    }
   }

What happens if the plugin makes changes to configuration, do they go out of sync until the entity is saved? Should the plugin at least have access to the entity, to use the setters there for making config changes? Juggling these two properties seems like it could be problematic, but I could be missing something. As an alternative could we:

• Drop the plugin collection as the mechanism for saving data.
• Provide the entity as an argument to the plugin.
• Change all settings and data by calling methods on the Workflow entity, not modifying an internal property from the plugin.
• This allows the entity to protect the integrity and enforce the state/transition structure, otherwise when each plugin makes changes they are doing their own version of all of those checks, isset/empty/sorting et al.

The way these two things pass config between each other is the only thing that really has me scratching my head, rest looks great.

The plugin should never have access to the configuration entity - that'd create a circular dependency. The config entity should configure the plugin and that's it. This code exists because we do this in ConfigEntityBase...

  /**
   * {@inheritdoc}
   */
  public function preSave(EntityStorageInterface $storage) {
    parent::preSave($storage);

    if ($this instanceof EntityWithPluginCollectionInterface) {
      // Any changes to the plugin configuration must be saved to the entity's
      // copy as well.
      foreach ($this->getPluginCollections() as $plugin_config_key => $plugin_collection) {
        $this->set($plugin_config_key, $plugin_collection->getConfiguration());
      }
    }

Which ensures that the latest config is got from the plugin. What we probably want to do is to move all the workflow methods to the plugin. That way the config entity just becomes about storage and the Workflow plugin is the state machine. Basically we should do what blocks do.

Having had this spelled out for me on IRC, this totally makes sense. There is a bit of follow up to ensure that plugins can define and manage the structure of the "settings" key on the workflow object and so long as WorkflowTypeInterface is satisfied, each plugin can essentially do what it wants.

Follow-ups ahoy, but this seems like a good stepping stone to get in now.

Thanks for the rtbc @Sam152 - just need to clean up this.

+++ b/core/modules/workflows/src/Entity/Workflow.php
@@ -502,7 +507,34 @@ protected function getNextWeight(array $items) {
+  /**
+   * @todo make unnecessary
+   */
...
+  /**
+   * @todo make unnecessary
+   */

We need to fix the docs here and point to the follow up that should get rid of these.

Opened the followup as I understand it here #2849827: Move workflow "settings" setters and getters to WorkflowTypeInterface, updated docs.

I've gone through this and tested as a user as thoroughly as I could creating various states and transitions and generating contents in various orders to try to get stuck.

At first, with own user error, I got stuck in a Needs Review state I had created and was unable to move from there into another state until I realised I needed to update the default Create New Draft and Publish states to allow transition from my new Needs Review state. That helped me realise the power of this and how customised a workflow you could get; nice!

I further tested the permissions as an editor / author / reviewer and that seems solid as well, no issues.

If we're going to put this in we should obey docs standards...

+++ b/core/modules/workflows/tests/src/Kernel/WorkflowTypeBaseTest.php
@@ -0,0 +1,80 @@
+    // Ensure that transitions can't be deleted, but if they are they will
+    // revert back to default configuration
+    $workflow->deleteTransition('rot')->save();
+    $this->assertTrue($workflow->hasTransitionFromStateToState('fresh', 'rotten'));
+    $this->assertFalse($workflow->hasTransitionFromStateToState('cooked', 'rotten'));

I'm not sure about this behaviour. It makes sense to not delete required states but the transitions aren't required. The thing is \Drupal\Component\Plugin\ConfigurablePluginInterface::defaultConfiguration() is not the same initial configuration. And the behaviour of allowing the meanings of the transitions to be changed but not deleted is super weird. Also this fix leads to the very strange behaviour that you can try to delete the "Create New Draft" transition via UI - it looks like it works but then it does not.

In the new patch it makes lots of sense that the required states are part of the annotation. But I guess using ::defaultConfiguration() is a wrong move - oops. This is such a tricky issue.

I'm going to merge #36 and with the require_states annotation and improved test coverage.

Here's an attempt to add the necessary to do the required states. It does not suffer from the problems of deleting the transition described in #53.

I still think that in an ideal world we'd do #2849827: Move workflow "settings" setters and getters to WorkflowTypeInterface so that WorkflowType's don't have any knowledge of the Workflow configuration entity. This would make the system more like Blocks and allow full programmatic creation of workflows without config entities that might be very useful for things like commerce. But this is a step in the right direction. Being able to guarantee the existence on draft and published for content moderation workflows means that we can start to solve the problem of what happens when installing content_moderation on existing sites. Or adding a workflow to a bundle with existing content.

Not entirely what sure to interdiff too since the patch is a chimera of #52 and #36 - so no interdiff proved.

Couldn't fault this, mostly nits and adding links to follow ups.

1. +++ b/core/modules/content_moderation/src/Plugin/WorkflowType/ContentModeration.php
   @@ -26,6 +30,20 @@ class ContentModeration extends WorkflowTypeBase {
   +  public function initializeWorkflow(WorkflowInterface $workflow) {
   

   I wonder if 2849827 should be added as a @todo to indicate $workflow should go away.

2. +++ b/core/modules/content_moderation/src/Plugin/WorkflowType/ContentModeration.php
   @@ -51,12 +69,15 @@ public function decorateState(StateInterface $state) {
   +    $required_state = isset($state) ? in_array($state->id(), $this->getRequiredStates(), TRUE) : FALSE;
   +
   ...
   +      '#disabled' => $required_state,
   
   @@ -64,6 +85,7 @@ public function buildStateConfigurationForm(FormStateInterface $form_state, Work
   +      '#disabled' => $required_state,
   

   There is a functional test for the UI this plugin provides, would be good to add these.

3. +++ b/core/modules/workflows/src/Entity/Workflow.php
   @@ -114,6 +116,20 @@ class Workflow extends ConfigEntityBase implements WorkflowInterface, EntityWith
   +        // @todo use specific exception.
   +        throw new RequiredStateMissingException("Workflow type '{$workflow_type->label()}' requires a state with the ID '$state_id' in workflow '{$this->id()}'");
   

   Note to create a follow up.

4. +++ b/core/modules/workflows/src/Exception/RequiredStateMissingException.php
   @@ -0,0 +1,11 @@
   + * Indicates that a workflow does not contain a required state
   

   Nit, full stop.

5. +++ b/core/modules/workflows/tests/src/Functional/WorkflowUiTest.php
   @@ -190,7 +196,26 @@ public function testWorkflowCreation() {
   +      'workflow' => $workflow->id(),
   +      'workflow_state' => 'draft'
   

   trailing comma

6. +++ b/core/modules/workflows/tests/src/Kernel/RequiredStatesTest.php
   @@ -0,0 +1,109 @@
   +      'id' => 'test',
   +      'type' => 'workflow_type_required_state_test'
   

   Tailing comma.

7. +++ b/core/modules/workflows/tests/src/Kernel/RequiredStatesTest.php
   @@ -0,0 +1,109 @@
   +      'id' => 'test',
   +      'type' => 'workflow_type_required_state_test'
   

   trailing comma

8. +++ b/core/modules/workflows/tests/src/Kernel/RequiredStatesTest.php
   @@ -0,0 +1,109 @@
   +      'id' => 'test',
   +      'type' => 'workflow_type_required_state_test'
   

   trailing comma

9. +++ b/core/modules/workflows/tests/src/Kernel/RequiredStatesTest.php
   @@ -0,0 +1,109 @@
   +    // Ensure that transitions can't be deleted, but if they are they will
   +    // revert back to default configuration
   +    $workflow->deleteTransition('rot')->save();
   +    $this->assertFalse($workflow->hasTransition('rot'));
   

   Does the comment here mismatch the behavior?

10. +++ b/core/modules/workflows/workflows.services.yml
    @@ -3,4 +3,8 @@ services:
    -      - { name: plugin_manager_cache_clear }
    \ No newline at end of file
    

    missing newline

Thanks @Sam152

1. I think it is okay to not add the @todo - we're not 100% how that is going to work out and we have the issue already - not sure what is gained.
2. Good idea - adding tests
3. Need to remove the @todo - I created the exception already :) - fixed and made the exception better.
4. Fixed
5. Fixed
6. Fixed
7. Fixed
8. Fixed
9. Indeed it does - nice spot - fixed
10. The missing new line is in HEAD

\Drupal\Tests\content_moderation\Kernel\ContentModerationPermissionsTest() needs to use real config - missing weight info on the states.

Looks good +1 RTBC

1. We could add this I guess - although it just adds complexity to State construction.
2. Yes they could - we could validate this in setConfiguration I guess - but this is always a question of how far should we go in preventing odd configuration. Through the UI this makes total sense. Through the API we could argue that whatever has allowed that is the place that should be fixed. Not sure.
3 & 4. required states are exactly that - states the workflow requires. Hence this should be enforced at the workflow level. They are not default states. If you want to add default states then you need to use the initializeWorkflow() method - the states and transitions added are not required.

Re #60.1 we can improve the variable name of course.

Just a couple things I noticed:

1) On the states edit form, when there are no transitions yet, it says there are no states yet. Perhaps like the attached so it's clear that the list of transitions are the ones that involve the selected state (as the table headings don't indicate that).

2) Is it necessary to always disable the 'to' on the transition edit form given that they are deletable? Is there some undesired behaviour if it were to be changed that needs the user to delete first and recreate it?

Hey @scott_euser you've found yet another bug! Thanks. But it is out of scope for this issue. We need a new issue to fix that. See https://www.drupal.org/core/scope for guidelines and examples for Drupal core issue scope. I'm going to upload the most recent in-scope patch again so the most recent patch on the issue is the correct one. @scott_euser if you file another issue I'm sure it'll get reviewed and committed quickly! Also feel free to reach out to me on IRC or Drupal slack if you've got questions about this.

Ah okay, I thought that was directly related to what you were working on, sorry! I'll learn soon enough :) (and gave that a re-read).
https://www.drupal.org/node/2850546 and https://www.drupal.org/node/2850549 opened.

Switching back to RTBC.

Generally this looks good, couple of questions though, first might be off topic here but feels conceptually important.

1. +++ b/core/modules/content_moderation/src/Plugin/WorkflowType/ContentModeration.php
   @@ -150,7 +172,16 @@ public function addEntityTypeAndBundle($entity_type_id, $bundle_id) {
   -      'states' => [],
   +      'states' => [
   +        'draft' => [
   +          'published' => FALSE,
   +          'default_revision' => FALSE,
   +        ],
   +        'published' => [
   +          'published' => TRUE,
   +          'default_revision' => TRUE,
   +        ],
   +      ],
   

   Setting published false when it's a forward revision of a published node is a bit tricky. For example with CPE when you preview a changeset/workspace, you'll see all the 'published' revisions connected to that workspace (and anything published that's not in the workspace).

   This includes content that's never been published yet too - if you want to preview new content in a changeset/workspace, the workspace revision would need to be published to (the default revision stays unpublished).

2. +++ b/core/modules/workflows/src/WorkflowAccessControlHandler.php
   @@ -56,17 +56,21 @@ public function __construct(EntityTypeInterface $entity_type, PluginManagerInter
   +    $workflow_type = $entity->getTypePlugin();
   +    if (strpos($operation, 'delete-state') === 0) {
   +      list(, $state_id) = explode(':', $operation, 2);
   

   Is this the only place we do this?

3. +++ b/core/modules/workflows/src/WorkflowDeleteAccessCheck.php
   @@ -0,0 +1,53 @@
   +   * The value of the '_workflow_state_delete_access' is ignored. The route must
   

   First sentence doesn't scan for me, should it just drop the 'the'?

Committed/pushed to 8.4.x and cherry-picked to 8.3.x. Thanks!