Moderating a non-translatable entity type throws exception

I'm wondering whether we could somehow test content_moderation with all kind of entity_test entities, which should give us test coverage for all kind of edge cases.

I'm glad you don't have the problem that the tests aren't passing on drupal.org :P

Following up on #21, all we need to do in order to get entity_test_entity_type_alter() to fire before content_moderation_entity_type_alter() is to implement hook_module_implements_alter() and move 'entity_test' to the beginning of the array.

Here's a patch that does that and also cleans up the test a little. Interdiff is to #21.

This is rc eligible as an experimental module improvement, per: https://www.drupal.org/core/d8-allowed-changes#rc

The rtbc in #27 is not really right - much of the work in the patch is @timmillwood's. The patch looks good to me but committing from this state sets a bad precedent.

+++ b/core/modules/content_moderation/src/EntityOperations.php
@@ -201,11 +200,14 @@ protected function updateOrCreateFromEntity(EntityInterface $entity) {
+      if ($content_moderation_state->language()->getId() !== $entity_langcode) {
+        $content_moderation_state = $content_moderation_state->getTranslation($entity_langcode);
+      }

Just a question for a follow up: Is it worth to have this additional if here in the first place? Why not get always the translation. All objects are available anyway.

+++ b/core/modules/system/tests/modules/entity_test/src/Entity/EntityTestWithBundle.php
@@ -32,7 +32,6 @@
- *     "langcode" = "langcode",

I wonder if this could break contrib tests that use EntityTestWithBundle? Making this test entity non translatable seems like an unexpected change for this issue - how about we just make a new type?

#2812811: Use EntityPublishedInterface during moderation of entities to add support beyond nodes introduces a few new test entities. Maybe langcode can be removed from EntityTestRevWithBundle there?

It seems weird that some entity types which are not translatable define the 'langcode' key, so I opened #2818379: Remove the 'langcode' entity key for non-translatable entity types to discuss that.

I discussed the issue mentioned above with @Berdir and it seems there is a valid use-case for a non-translatable entity type to define the 'langcode' key, which makes me think that the patch is not actually correct, in the sense that we should check the availability of the langcode field instead of whether the entity type is translatable or not.

And, given the above, we do need to provide a new test entity type for this :)

Hm, in other issues I thought I ran into issues with entity_test_with_bundle not being revisionable, but if it works, awesome.

Edit: Ah, I can see you are making the entity revisionable. It looks like we have a few tests popping up that require that functionality. Is there really much overhead to a few new test entity types that cover the cross-section of entity features that we need? Altering the entity definitions in that manner might become tedious?

Looks like the pattern is already in use, so it's an RTBC from me.

1. +++ b/core/modules/content_moderation/src/Plugin/Field/ModerationStateFieldItemList.php
   @@ -43,12 +43,14 @@ protected function getModerationState() {
   +        if ($entity->getEntityType()->isTranslatable()) {
   

   Missed a spot :)

2. +++ b/core/modules/content_moderation/tests/src/Kernel/ContentModerationStateTest.php
   @@ -211,6 +218,90 @@ public function testMultilingualModeration() {
   +  public function testNonTranslatableEntityTypeModeration() {
   ...
   +  public function testNonLangcodeEntityTypeModeration() {
   

   The new test method is an exact copy of the other one, shouldn't there be a difference somewhere?

Ah, okay, maybe you can put that explanation in the patch as well? And point 1) still stands.

Nice, looks good now :)

+++ b/core/modules/content_moderation/config/schema/content_moderation.schema.yml
@@ -70,6 +70,22 @@ block_content.type.*.third_party.content_moderation:
+entity_test.entity_test_bundle.*.third_party.content_moderation:
+  type: mapping
+  label: 'Enable moderation states for this entity test type'
+  mapping:
+    enabled:
+      type: boolean
+      label: 'Moderation states enabled'
+    allowed_moderation_states:
+      type: sequence
+      sequence:
+        type: string
+        label: 'Moderation state'
+    default_moderation_state:
+      type: string
+      label: 'Moderation state for new entity test'
+

Imo this being in the content moderation schema shows the problem with the current approach of it handles bundles. This is rendered completely unnecessary by #2779647: Add a workflow component, ui module, and implement it in content moderation. If we want to get this in before that we need to move this schema to a test module because it is just not relevant for regular running of a site using content moderation.

I think anything made irrelevant by #2779647 is worth holding off on for the moment.

Imo this being in the content moderation schema shows the problem with the current approach of it handles bundles. This is rendered completely unnecessary by #2779647: Add a workflow component, ui module, and implement it in content moderation

This is also *exactly* what made me open and work on #2799785: Entity types with non-config bundles can not be moderated, which was closed in favor of the larger refactoring.

However, that refactoring is mostly better suited for 8.3.x, while this is a genuine 8.2.x bug fix so it's not irrelevant at all.

@alexpott's review has been addressed, back to RTBC.

Committed and pushed 144fe78 to 8.3.x and 1506901 to 8.2.x. Thanks!

@amateescu #2779647: Add a workflow component, ui module, and implement it in content moderation is 8.2.x eligible btw - content_moderation is experimental. The whole point of being in alpha is fix things like this.

diff --git a/core/modules/content_moderation/tests/src/Kernel/ContentModerationStateTest.php b/core/modules/content_moderation/tests/src/Kernel/ContentModerationStateTest.php
index c1eef69..67a7a74 100644
--- a/core/modules/content_moderation/tests/src/Kernel/ContentModerationStateTest.php
+++ b/core/modules/content_moderation/tests/src/Kernel/ContentModerationStateTest.php
@@ -4,9 +4,7 @@
 
 use Drupal\content_moderation\Entity\ContentModerationState;
 use Drupal\content_moderation\Entity\ModerationState;
-use Drupal\Core\Config\Entity\ThirdPartySettingsInterface;
 use Drupal\entity_test\Entity\EntityTestBundle;
-use Drupal\entity_test\Entity\EntityTestRev;
 use Drupal\entity_test\Entity\EntityTestWithBundle;
 use Drupal\KernelTests\KernelTestBase;
 use Drupal\language\Entity\ConfigurableLanguage;

Removed unused uses on commit.

Here's a followup to clean up the tests that were added here: #2898020: Simplify ContentModerationStateTest