Entity types with non-config bundles can not be moderated

rc eligible is just added by committers

IMHO in #2363155: content_translation.settings config is not scalable the point was that it also storing stuff per bundle/field, which we at least wouldn't do here :) But yeah, for example the fact that we can use config entities is a huge plus.

Just some quick feedback.

1. +++ b/core/modules/content_moderation/content_moderation.services.yml
   --- /dev/null
   +++ b/core/modules/content_moderation/src/ContentModerationSettingsException.php
   

   Can we put them into an exception folder?

2. +++ b/core/modules/content_moderation/src/Entity/ContentModerationSettings.php
   @@ -0,0 +1,204 @@
   + *   list_cache_tags = { "rendered" }
   

   Any idea why this is added here?

3. +++ b/core/modules/content_moderation/src/Entity/ContentModerationSettings.php
   @@ -0,0 +1,204 @@
   +  /**
   +   * The default moderation state for new content of the entity type bundle.
   +   *
   +   * @var array
   +   */
   +  protected $default_moderation_state;
   

   IMHO its hard to define multiple default states, I guess this is a string?

4. +++ b/core/modules/content_moderation/src/Entity/ContentModerationSettings.php
   @@ -0,0 +1,204 @@
   +      $entity = self::create(['target_entity_type_id' => $entity_type_id, 'target_bundle' => $bundle]);
   

   Nitpick: let's use static::create

5. +++ b/core/modules/content_moderation/src/Entity/ContentModerationSettings.php
   @@ -0,0 +1,204 @@
   +    // Create dependency on the target bundle.
   +    $entity_type = \Drupal::entityTypeManager()->getDefinition($this->target_entity_type_id);
   +    $bundle_config_dependency = $entity_type->getBundleConfigDependency($this->target_bundle);
   +    $this->addDependency($bundle_config_dependency['type'], $bundle_config_dependency['name']);
   +
   +    // Create dependencies on the the allowed moderation states. We don't need
   

   Does this code assumes that there is always a target bundle?

6. +++ b/core/modules/content_moderation/src/EntityTypeInfo.php
   @@ -127,7 +136,7 @@ public function entityTypeAlter(array &$entity_types) {
   -  protected function addModerationToEntity(ContentEntityTypeInterface $type) {
   +  protected function addModerationToEntityType(ContentEntityTypeInterface $type) {
        if (!$type->hasHandlerClass('moderation')) {
          $handler_class = !empty($this->moderationHandlers[$type->id()]) ? $this->moderationHandlers[$type->id()] : ModerationHandler::class;
          $type->setHandlerClass('moderation', $handler_class);
   @@ -161,13 +170,13 @@ protected function addModerationToEntity(ContentEntityTypeInterface $type) {
   
   @@ -161,13 +170,13 @@ protected function addModerationToEntity(ContentEntityTypeInterface $type) {
       * @return \Drupal\Core\Config\Entity\ConfigEntityTypeInterface
       *   The modified config entity definition.
       */
   -  protected function addModerationToEntityType(ConfigEntityTypeInterface $type) {
   +  protected function addModerationToBundleEntityType(ConfigEntityTypeInterface $type) {
        if ($type->hasLinkTemplate('edit-form') && !$type->hasLinkTemplate('moderation-form')) {
          $type->setLinkTemplate('moderation-form', $type->getLinkTemplate('edit-form') . '/moderation');
        }
   

   These new method names are a bit weird, IMHO.

1. +++ b/core/modules/content_moderation/src/Entity/ContentModerationSettings.php
   @@ -0,0 +1,203 @@
   +  public static function loadByEntityTypeBundle($entity_type_id, $bundle) {
   +    if ($entity_type_id == NULL || $bundle == NULL) {
   +      return NULL;
   +    }
   

   I know this is just c&p, but IMHO that's horrible code. We should throw an exception or something. If someone passes in invalid values, than getting back the wrong value is a bad idea.

2. +++ b/core/modules/content_moderation/src/ModerationInformation.php
   @@ -34,6 +33,13 @@ public function __construct(EntityTypeManagerInterface $entity_type_manager) {
   +  public function getModerationSettings($entity_type_id, $bundle) {
   +    return ContentModerationSettings::loadByEntityTypeBundle($entity_type_id, $bundle);
   +  }
   

   I would actually turn that around. Write the proper method in the service and just call it from ContentModerationSettings for the lazy people out there.

3. +++ b/core/modules/content_moderation/tests/src/Kernel/ModerationInformationTest.php
   @@ -0,0 +1,63 @@
   +    $this->assertEquals(FALSE, $moderation_information->isModeratedEntity($entity));
   ...
   +    $this->assertEquals(FALSE, $moderation_information->canModerateEntitiesOfEntityType($entity_type));
   ...
   +    $this->assertEquals(TRUE, $moderation_information->canModerateEntitiesOfEntityType($entity_type));
   ...
   +    $this->assertEquals(FALSE, $moderation_information->shouldModerateEntitiesOfBundle($entity_type, 'entity_test_rev'));
   ...
   +    $this->assertEquals(TRUE, $moderation_information->shouldModerateEntitiesOfBundle($entity_type, 'entity_test_rev'));
   ...
   +    $this->assertEquals(FALSE, $moderation_information->shouldModerateEntitiesOfBundle($entity_type, 'entity_test_rev'));
   

   Nitpick: Those cold all use $this->assertTrue/$this->assertFalse

4. +++ b/core/modules/content_moderation/tests/src/Kernel/ViewsDataIntegrationTest.php
   deleted file mode 100644
   index 2b2e6f2..0000000
   
   index 2b2e6f2..0000000
   --- a/core/modules/content_moderation/tests/src/Unit/ModerationInformationTest.php
   
   --- a/core/modules/content_moderation/tests/src/Unit/ModerationInformationTest.php
   +++ /dev/null
   
   +++ /dev/null
   +++ /dev/null
   @@ -1,124 +0,0 @@
   

   Is there a reason why this test got removed?

As content_moderation is an experimental module this is RC eligible. Looking at the patch - nice to see dependency calculations there already. Also given this is a new config entity it'd be nice to have this done before 8.2.0.

In general this patch looks really sane.

Thank you for the quick fixing

That unit test was changed into a kernel test: \Drupal\Tests\content_moderation\Kernel\ModerationInformationTest

Ah gotcha, I missed that.

+++ b/core/modules/content_moderation/src/EntityTypeInfo.php
@@ -262,16 +271,10 @@ public function entityExtraFieldInfo() {
+    foreach (ContentModerationSettings::loadMultiple() as $moderation_settings) {

+++ b/core/modules/content_moderation/src/Form/BundleModerationSettingsForm.php
@@ -33,40 +55,44 @@ public function __construct(EntityTypeManagerInterface $entity_type_manager) {
+    return ContentModerationSettings::loadByEntityTypeBundle($bundle->getEntityType()->getBundleOf(), $bundle->id());

Since this thing is injected I don't think we should be using statics. I think we should do this in a follow since making this change prior to the 8.2.0 is advantageous.

That said #2779647: Add a workflow component, ui module, and implement it in content moderation also gives me pause for thought because if we go down that route we might have no need for this either.

@amateescu - we probably will need an object to store which workflow applies to which bundle. I'm not sure that the default should be on bundle. I think the default is a property of the workflow not the way the workflow is applied to the bundle.

> Move the per-bundle moderation settings to a new config entity type: ContentModerationSettings, since we already know that keeping all the settings in a single config object is not scalable

So sometimes settings about a bundle go in the bundle config entity, and sometimes they go elsewhere? That's kind of breaking our own system...

Surely the case when bundles are not config entities breaks *all* systems that use third-party system to store settings in a config entity bundle. So does that mean that the third-party settings is now no longer to be used? If not, when should it be used and when should it not be used?

Discussed with @amateescu and @berdir we agreed to do #2779647: Add a workflow component, ui module, and implement it in content moderation and use hook_entity_bundle_info_alter() which would mean that we would end up removing the config entity objects added here. So let's postpone this issue on that one and probably once that is completed we'll just mark this as a duplicate.

Now #2779647: Add a workflow component, ui module, and implement it in content moderation has been committed, can we close this?

Moving this firmly back to "needs work".

Specifically for altering \Drupal\content_moderation\EntityTypeInfo::filterNonRevisionableEntityTypes (http://cgit.drupalcode.org/drupal/tree/core/modules/content_moderation/s...) to not look for instances of ConfigEntityTypeInterface

Here's a little patch that changes Content Moderation to work with entity types without a config entity bundle.

With a simpler test which comes from the patch in #15.

This is great. Love to see CM becoming more flexible.

IMHO this needs an issue summary update, it says "Move the per-bundle moderation settings to a new config entity type: ContentModerationSettings" but then it does not introduce a new entity type.

Good call, done.

Committed 82f1668 and pushed to 8.3.x. Thanks!

/me ponders when we're going to change the name to entity_moderation :)

#2846830: Add changelog for Drupal 8.3.0