Disallow moderation of internal entity types

Looks good to me.

Do we have test coverage? I looked in \Drupal\Tests\content_moderation\Kernel\EntityTypeInfoTest but I couldn't spot any.

I couldn't find a test that tested that "content_moderation_state" entity type wasn't moderatable, so here's one. It also checks entity_test which has lots of revisionable entity types which can be moderated, and an internal entity type "entity_test_no_label" to double check all internal types are not able to be moderated.

+++ b/core/modules/content_moderation/tests/src/Kernel/EntityTypeInfoTest.php
@@ -61,4 +61,35 @@ public function testEntityBaseFieldInfo() {
+    foreach ($entity_types as $entity_type) {
+      if (in_array($entity_type->id(), [
+        'node',
+        'block_content',
+        'entity_test_no_uuid',
+        'entity_test_no_bundle',
+        'entity_test_mulrevpub',
+        'entity_test_rev',
+        'entity_test_mulrev_chnged_revlog',
+        'entity_test_mulrev_changed',
+        'entity_test_mulrev',
+      ])) {
+        $this->assertTrue($entity_type->hasHandlerClass('moderation'), $entity_type->id() . " has the moderation handler.");
+      }
+      else {
+        // Non-revisionable and internal entity types, such as
+        // entity_test_no_label and content_moderation_state, should not have
+        // the moderation handler added.
+        $this->assertFalse($entity_type->hasHandlerClass('moderation'), $entity_type->id() . " doesn't have the moderation handler.");
+      }
+    }

To make the test more resilient to change I'd do something like

  /**
   * Test the correct entity types have moderation added.
   *
   * @covers ::entityTypeAlter
   */
  public function testEntityTypeAlter() {
    $entity_types = $this->entityTypeManager->getDefinitions();
    $this->entityTypeInfo->entityTypeAlter($entity_types);
    $moderated_types = [
      'node',
      'block_content',
      'entity_test_no_uuid',
      'entity_test_no_bundle',
      'entity_test_mulrevpub',
      'entity_test_rev',
      'entity_test_mulrev_chnged_revlog',
      'entity_test_mulrev_changed',
      'entity_test_mulrev',
    ];
    foreach ($moderated_types as $entity_type_id) {
      $this->assertTrue($entity_types[$entity_type_id]->hasHandlerClass('moderation'), $entity_type_id . " has the moderation handler.");
    }

    $internal_types = [
      'content_moderation',
      'entity_test_no_label',
    ];
    // Non-revisionable and internal entity types, such as
    // entity_test_no_label and content_moderation_state, should not have
    // the moderation handler added.
    foreach ($internal_types as $entity_type_id) {
      $this->assertFalse($entity_types[$entity_type_id]->hasHandlerClass('moderation'), $entity_type_id . " does not have the moderation handler.");
    }
  }

It also has no conditions which makes the test easier to understand and less prone to error. Basically it is more explicit.

So yeah some of those types in the original patch and my suggested test don't exist because node and block are not installed...

  public function testEntityTypeAlter() {
    $entity_types = $this->entityTypeManager->getDefinitions();
    $moderated_types = [
      'entity_test_no_uuid',
      'entity_test_no_bundle',
      'entity_test_mulrevpub',
      'entity_test_rev',
      'entity_test_mulrev_chnged_revlog',
      'entity_test_mulrev_changed',
      'entity_test_mulrev',
    ];
    foreach ($moderated_types as $entity_type_id) {
      $this->assertTrue($entity_types[$entity_type_id]->hasHandlerClass('moderation'), $entity_type_id . " has the moderation handler.");
    }

    $internal_types = [
      'content_moderation_state',
      'entity_test_no_label',
    ];
    // Non-revisionable and internal entity types, such as
    // entity_test_no_label and content_moderation_state, should not have
    // the moderation handler added.
    foreach ($internal_types as $entity_type_id) {
      $this->assertFalse($entity_types[$entity_type_id]->hasHandlerClass('moderation'), $entity_type_id . " does not have the moderation handler.");
    }
  }

Plus we don't actually need to call the alter manually - it's already applied to the definitions.

Updating based on #7, with the added explicit check for non-revisionable entity types.

+++ b/core/modules/content_moderation/tests/src/Kernel/EntityTypeInfoTest.php
@@ -61,4 +61,70 @@ public function testEntityBaseFieldInfo() {
+      'entity_test_no_uuid',
+      'entity_test_no_bundle',
+      'entity_test_mulrevpub',
+      'entity_test_rev',
+      'entity_test_mulrev_chnged_revlog',
+      'entity_test_mulrev_changed',
+      'entity_test_mulrev',
...
+      'workflow',
+      'entity_test_multivalue_basefield',
+      'entity_test_with_bundle',
+      'entity_test_default_access',
+      'entity_test_admin_routes',
+      'entity_test_constraints',
+      'entity_test_string_id',
+      'entity_test_default_value',
+      'entity_test_base_field_display',
+      'entity_test_composite_constraint',
+      'entity_test_no_label',
+      'entity_test',
+      'entity_test_label',
+      'entity_test_field_override',
+      'entity_test_mul',
+      'entity_test_constraint_violation',
+      'entity_test_mul_changed',
+      'entity_test_field_methods',
+      'entity_test_label_callback',
+      'entity_test_mul_langcode_key',
+      'entity_test_cache',
+      'entity_test_computed_field',
+      'entity_test_no_id',
+      'entity_test_view_builder',
+      'entity_test_bundle',
+      'entity_test_mul_default_value',
+      'entity_form_display',
+      'entity_view_mode',
+      'entity_view_display',
+      'entity_form_mode',
+      'base_field_override',
+      'date_format',
...
+      'content_moderation_state',
+      'entity_test_no_label',

I would have liked to see this test pick 1 test entity type for each cross section of criteria we are using for adding moderation.

That is:

• A revisionable entity type.
• A non-revisionable entity type
• An internal revisionable entity type (if we have one).
• A internal non-revisionable entity type (if we have one).
• Maybe 1 config entity type as well?

Ideally the identity of each test entity type should be based on this criteria (ie, we use "entity_test_rev" for a revisionable entity type). I think doing this would mean:

• A less fragile test when the characteristics of our test entity types change.
• A more logical changeset when the criteria we used for adding moderation changes.

That's my only feedback, if we'd rather maintain the list as a snapshot of all the entity types the test installs, then I'm happy to RTBC.

I think we need to test more than one of each type, so we might as well test all of them.

Although one anomaly is entity_test_no_label is internal but not revisionable, so I believe this test would still pass without the "fix".

I agree with @Sam152 listing and testing all available entity types is not as useful as testing each specific situation.

LGTM

+++ b/core/modules/content_moderation/tests/src/Kernel/EntityTypeInfoTest.php
@@ -65,66 +65,35 @@ public function testEntityBaseFieldInfo() {
+    if ($moderatable) {
+      $this->assertTrue($entity_types[$entity_type_id]->hasHandlerClass('moderation'));
     }
...
+    else {
+      $this->assertFalse($entity_types[$entity_type_id]->hasHandlerClass('moderation'));
     }

Isn't this equivalent to $this->assertEquals($moderatable, $entity_types[$entity_type_id]->hasHandlerClass('moderation'))?

Not blocking RTBC on this, since I think you could consider it feedback on style.

Committed and pushed 1e7cb6ac40 to 8.6.x and c6db9b2581 to 8.5.x. Thanks!

Backported to 8.5.x as I think this is really a bug and not a task.

diff --git a/core/modules/content_moderation/tests/src/Kernel/EntityTypeInfoTest.php b/core/modules/content_moderation/tests/src/Kernel/EntityTypeInfoTest.php
index 7f28537fa0..09990fe796 100644
--- a/core/modules/content_moderation/tests/src/Kernel/EntityTypeInfoTest.php
+++ b/core/modules/content_moderation/tests/src/Kernel/EntityTypeInfoTest.php
@@ -70,12 +70,7 @@ public function testEntityBaseFieldInfo() {
    */
   public function testEntityTypeAlter($entity_type_id, $moderatable) {
     $entity_types = $this->entityTypeManager->getDefinitions();
-    if ($moderatable) {
-      $this->assertTrue($entity_types[$entity_type_id]->hasHandlerClass('moderation'));
-    }
-    else {
-      $this->assertFalse($entity_types[$entity_type_id]->hasHandlerClass('moderation'));
-    }
+    $this->assertSame($moderatable, $entity_types[$entity_type_id]->hasHandlerClass('moderation'));
   }
 
   /**

Changed on commit - I ran the test locally and it passed. Nice idea @Sam152. Less conditionality in tests is always preferable.