ContentModeration workflow type plugin incorrectly preserves bundle keys on sorting and does not sort entity types

Confirmed that the first two steps are not necessary, so updating the IS.

And making full STR from installation.

+++ b/core/modules/content_moderation/tests/src/Kernel/ContentModertaionWorkflowTypeApiTest.php
@@ -78,4 +78,20 @@ public function testAppliesToEntityTypeAndBundle() {
+    // The content moderation plugin does not valid the existence of the entity
+    // type or bundle.
+    $workflow_plugin->addEntityTypeAndBundle('fake_node', 'fake_page');
+    $workflow_plugin->addEntityTypeAndBundle('fake_node', 'fake_article');

Probably this bug and coverage relies on P coming before A in the alphabet, so let's mention that somehow?

Also I think the inline comment is a copypaste from the empty array test from the other issue.

I'd prefer not to postpone a borderline-major sorting bug that causes diffs in config (and that causes diffs even by being fixed) on spelling fixes in a test.

Re: the diffs, we don't need an upgrade path because the module is alpha, but we might have considered it if it were stable to avoid unpredictable fixes at random the next time every affected workflow is resaved. Upgrade paths tend to get more annoying with time.

It looks like the sorting for the entity type itself has the opposite bug (they are not sorted). Following the STR but replacing articles with basic content blocks:

diff --git a/workflows.workflow.editorial.yml b/workflows.workflow.editorial.yml
index ec86e27..01e1a5d 100644
--- a/workflows.workflow.editorial.yml
+++ b/workflows.workflow.editorial.yml
@@ -64,7 +64,5 @@ type_settings:
       published: true
       default_revision: true
   entity_types:
-    block_content:
-      - basic
     node:
       - page

then

diff --git a/workflows.workflow.editorial.yml b/workflows.workflow.editorial.yml
index 01e1a5d..b9550c2 100644
--- a/workflows.workflow.editorial.yml
+++ b/workflows.workflow.editorial.yml
@@ -66,3 +66,5 @@ type_settings:
   entity_types:
     node:
       - page
+    block_content:
+      - basic

Not sure about the scope but seems related.

#2850592: Typos in ContentModertaionWorkflowTypeApiTest is in now.

NW for #5, #6, and possibly #11.

+++ b/core/modules/content_moderation/tests/src/Kernel/ContentModerationWorkflowTypeApiTest.php
@@ -78,4 +78,26 @@ public function testAppliesToEntityTypeAndBundle() {
+   * @covers ::removeEntityTypeAndBundle

How does this cover removeEntityTypeAndBundle? Maybe it'd be worth adding a few more bundles in the test, then removing one or two and checking the order again?

ok, sounds good.

1. +++ b/core/modules/content_moderation/tests/src/Kernel/ContentModerationWorkflowTypeApiTest.php
   @@ -78,4 +78,25 @@ public function testAppliesToEntityTypeAndBundle() {
   +    // The content moderation plugin does not validate the existence of the
   +    // entity type or bundle. The entity types and bundles are intentionally
   +    // added in reverse alphabetical order.
   +    $workflow_plugin->addEntityTypeAndBundle('fake_node', 'fake_page');
   +    $workflow_plugin->addEntityTypeAndBundle('fake_block', 'fake_custom');
   +    $workflow_plugin->addEntityTypeAndBundle('fake_node', 'fake_article');
   ...
   +    $this->assertSame(
   +      ['fake_block' => ['fake_custom'], 'fake_node' => ['fake_article', 'fake_page']],
   +      $workflow_plugin->getConfiguration()['entity_types']
   +    );
   

   Does this provide the coverage it says for the entity types? Had to read it twice. I was going to suggest testing the two levels separately, but maybe that's not necessary. At least though let's have a test-only patch that's this test + the original fix only for the bundle keys to prove the coverage for the other bug as well.

2. +++ b/core/modules/content_moderation/tests/src/Kernel/ContentModerationWorkflowTypeApiTest.php
   @@ -78,4 +78,25 @@ public function testAppliesToEntityTypeAndBundle() {
   +    // The entity type keys and bundle values should be sorted
   +    // sorted alphabetically. The bundle array index should not reflect the
   

   sorted sorted. Also looks like it can be rewrapped.

The interdiff doesn't seem right, but the patch look like it resolves the concerns in #19.

+1 to the RTBC in #20.

Back to rtbc as these are comment improvements asked by a committer.

Hmm, meh to that. It's still a change that should undergo review. But @timmillwood has +1ed it so it is RTBC in any case.

Also the removal of the bit about validation was what I asked for in #6, so that looks good to me too. Thanks for the test-only patch; I double-checked for the "correct" fail on https://www.drupal.org/pift-ci-job/593625.

Committed to 8.4.x and cherry-picked to 8.3.x, since this is eliminating noisy/confusing config diffs (so improving the data model), and also since it's for an experimental module. Thanks!

@alexpott and I agreed that this does not need to go in the 8.3.0 release notes/changelog, since it is just a followup to the workflow conversion that happened in 8.3.x only and so was a regression since 8.2.x, but I am mentioning it in the beta release notes since the bug affected the alpha.