Comment types do not depend on the provider of the target entity type

Thanks. I'll take a look at the config later today.

I'm still feeling my way a little with this.

I can see that the default comment type and forum comment type both have dependencies on the node module. I don't quite understand the other errors but let's see what happens with this first.

Well ... I thought I understood what the failed tests are saying but obviously not. I'll take another look tomorrow but I'm probably going to need some help or at least a point in the right direction.

Thanks. I just figured that out minutes ago by digging deeper into the test that compares the active and default configs.

I think this is what it needs.

dependencies:
  enforced:
    module:
      - forum
  module:
    - node

And similar for the standard profile comment.

Making a new patch now.

1. +++ b/core/modules/comment/src/Entity/CommentType.php
   @@ -93,4 +93,13 @@ public function getTargetEntityTypeId() {
   +    $target_entity_type = \Drupal::entityTypeManager()->getDefinition($this->target_entity_type_id);
   

   There's always a getTargetEntityTypeId() method available to get the $this->target_entity_type_id. Let's use it.

2. +++ b/core/modules/comment/src/Tests/CommentTypeTest.php
   @@ -187,4 +187,15 @@ public function testCommentTypeDeletion() {
   +   * Tests that the module providing the target entity is listed as a dependency
   

   Should end with a dot. Try to shorten the entire phrase.

3. +++ b/core/modules/comment/src/Tests/CommentTypeTest.php
   @@ -187,4 +187,15 @@ public function testCommentTypeDeletion() {
   +    $type = $this->createCommentType('dep_test');
   

   Naming: Let's be more specific. $comment_type?

4. +++ b/core/modules/comment/src/Tests/CommentTypeTest.php
   @@ -187,4 +187,15 @@ public function testCommentTypeDeletion() {
   +    $dependencies = $type->calculateDependencies()->getDependencies();
   

   We don't need to (re)calculate the dependencies here. They are already computed when the comment type is created/saved. This is enough:

   $dependencies = $comment_type->getDependencies();
   

5. +++ b/core/modules/comment/src/Tests/CommentTypeTest.php
   @@ -187,4 +187,15 @@ public function testCommentTypeDeletion() {
   +    $this->assertTrue(in_array($target_entity_type->getProvider(), $dependencies['module']), 'Module dependency was found.');
   

   There's no need for assertion messages. They make the tests hard to be followed/debugged. Consider removing the message. Also, what if there's no 'module' key in $dependencies? The test will not fail softly (as we prefer). Shouldn't we assure an empty array? For example:

   $modules = !empty($dependencies['module']) ? $dependencies['module'] : [];
   

   ...and then use $modules in assertion.

6. It still lacks upgrade path. We need to implement a post update function and simply re-save all comment type entities. Then we need also a test for this.

#18.5 can be solved more clever by getting the dependencies in this way:

$dependencies = $comment_type->getDependencies() + ['module' => []];

In this way we always have a $dependencies['module'], even it's empty.

Thanks @claudiu.cristea. My first patch so I have a lot to learn :)

I haven't looked into the upgrade path yet.

@tetranz, it's a very promising start :)

Is still missing the upgrade path and a corresponding test. What you'll need to do is to create in the module root a file comment.post_update.php and add an update function with the naming pattern comment_post_update_SOME_NAME(). See https://api.drupal.org/api/drupal/core%21lib%21Drupal%21Core%21Extension...

In this function you should only iterate through all comment type entities and re-save them. Something like this should work:

function comment_post_update_provider_dependency() {
  $comment_types = CommentType::loadMultiple();
  array_walk($comment_types, function (CommentTypeInterface $comment_type) {
    $comment_type->save();
  });
}

This would automatically add the dependency to the entity.

But we need also to test this. You'll find some source of inspiration in other update tests:

• \Drupal\image\Tests\Update\ImageUpdateTest
• \Drupal\editor\Tests\Update\EditorUpdateTest
• \Drupal\field\Tests\Update\FieldUpdateTest::testFieldUpdate8002()

Somehow I messed up the interdiff.

I've been trying to understand the final failed test but I just noticed https://www.drupal.org/node/2724871 which I think explains it.

I'll move on to the upgrade path.

Trying the test on 8.2.x to see if the Migrate test still fails.

@tetranz,

Looks nice to me too. Only few nits.

1. +++ b/core/modules/comment/comment.post_update.php
   @@ -0,0 +1,19 @@
   +/**
   + * Saves the dependency on the entity provider into comment_type entities.
   + */
   +function comment_post_update_provider_dependency() {
   

   We need to group post update functions by the minor Drupal version to which they are referring. See core/modules/field/field.post_update.php for an example.

   Here, probably we add

   /**
    * @addtogroup updates-8.1.x
    * @{
    */
   

   and close that.

2. +++ b/core/modules/comment/src/Tests/Update/CommentUpdateTest.php
   @@ -0,0 +1,52 @@
   +    $dependencies = $this->config($forum_comment_type)->get('dependencies.module') ?: [];
   +    $this->assertTrue(in_array('node', $dependencies));
   +
   +  }
   +
   

   Un-needed extra empty line.

Thanks to both of you for the help and guidance. It was a good learning experience. Just what I needed to get started. I'll go find another issue now. :)

For me this is RTBC if green. When the other issue is fixed we'll need to run the test again againts 8.1.x.

I think something changed in the node module. I'll have to take a look later.

NodeLoadMultipleTest.php doesn't appear to have a use statement for Node. That's why it's trying to find it in the same namespace, i.e., Drupal\node\Tests\Node

NodeLoadMultipleTest was failing due to a borked commit which has been reverted.

Running test again.

+1 for RTBC. Looks clean now.

I was about to commit this, but then realised I have a question:

If you've already uninstalled node module, but have comments missing the dependency, what happens to them with the update? This could possibly be handled in a new issue adding either an update and/or test coverage - since this at least stops the issue on sites that are only missing the dependency itself.

I see what you mean. Should I change the post_update and related test to only do something if the node module exists? Without that I think the $comment_type->save is going to crash or do something meaningless.

I think that's a good idea here.

We should then open up a follow-up for sites that are already affected - and that could start with test coverage for the case to see how exactly it breaks.

+++ b/core/modules/comment/comment.post_update.php
@@ -0,0 +1,34 @@
+  // The comment types that need upgrading depend on the node module.
+  // There is no point in doing this if node is not installed.
+  if (!\Drupal::moduleHandler()->moduleExists('node')) {
+    return;
+  }

+++ b/core/modules/comment/src/Tests/CommentTypeTest.php
@@ -187,4 +187,15 @@ public function testCommentTypeDeletion() {
+    $target_entity_type = \Drupal::entityTypeManager()->getDefinition($comment_type->getTargetEntityTypeId());

+++ b/core/modules/comment/src/Tests/Update/CommentUpdateTest.php
@@ -0,0 +1,56 @@
+    // This test is meaningless if the node module is not installed.
+    if (!\Drupal::moduleHandler()->moduleExists('node')) {
+      return;
+    }

No, sorry. It's not only node. A comment type can be defined for any other arbitrary entity type. I think, inside array_walk() we need to:

if (\Drupal::entityTypeManager()->getDefinition($comment_type->getTargetEntityTypeId())) {
  $comment_type->save();
} 

Then in test we need to test the same, probably using $this->config.

+++ b/core/modules/comment/src/Entity/CommentType.php
@@ -93,4 +93,14 @@ public function getTargetEntityTypeId() {
+    $target_entity_type = \Drupal::entityTypeManager()->getDefinition($this->getTargetEntityTypeId());

Also, what if there's no definition for this target entity type? As is called now it will throw an exception. Do we want that? I'm not sure, I thin we most probably want to avid the dependency. This is open for discussion.

Yes, of course, that makes sense for the post_update.

I'm a bit confused about the update test. I thought drupal-8.filled.standard.php.gz gives us a known set of data which only has the default comment type and the forum comment type. We know that both of those depend on the node entity provided by the node module.

Are you saying that it would be cleaner to not make that assumption in the update test and instead iterate over what we really do have and use the configuration properly rather than hard coding node? That sounds better and more generic.

The existing update test looks OK to me, we know what data there is. Although I think we could drop the early return for node being installed - it should always be installed no?

What I meant is in follow-up issue, we'd want a test where the database has node module uninstalled and without the early return - to validate whatever we end up doing for comment types where the provider module has already been uninstalled. But there not here.

post_update now checks properly that the entity type definition exists before attempting to save and therefore recalculate dependencies.

I implemented @claudiu.cristea's suggested definition check in calculateDependencies then I changed my mind and removed it. I happy to put it back if it seems necessary.

I will open another for the situation where the provider module does not exist.

I created a related issue here. https://www.drupal.org/node/2733819. The wording could probably be improved a bit.

I'll have a go at it if nobody else grabs it in the next day or so.

I guess this needs to be tested on 8.1.x.

Trying to bring this back to life. Redone it for 8.3.x.

Forum now sets its comment type as a dependency so I've removed any reference to the forum module.

Node still does not set the default comment type as a dependency.

@tetranz, for the first failure, you probably need to add 'target_entity_type_id' => 'entity_test' in:

    CommentType::create([
      'id' => 'comment',
      'label' => $this->randomString(),
    ])->save();

inside \Drupal\Tests\comment\Kernel\CommentIntegrationTest::setUp()

Also check the coding standards messages

+++ b/core/modules/comment/comment.post_update.php
@@ -0,0 +1,32 @@
+    // Make sure that a definition exists for the target entity.
+    // The providing module may have been uninstalled.

The first line of comment should be continued. From https://www.drupal.org/docs/develop/coding-standards/api-documentation-a...

Lines containing comments (including docblocks) must wrap as close to 80 characters as possible without going over

Thanks @claudiu.cristea. I think this might do it.

I shouldn't have removed the forum test.

Looks perfect to me, I have nothing more to comment here.

1. +++ b/core/modules/comment/comment.post_update.php
   @@ -0,0 +1,32 @@
   +/**
   + * @addtogroup updates-8.3.x
   + * @{
   + */
   ...
   +/**
   + * @} End of "addtogroup updates-8.3.x".
   + */
   

   These should be removed. New policy - see #2860096: Remove api doc groups for updates eg. updates-8.2.x-to-8.3.x.

2. +++ b/core/modules/comment/comment.post_update.php
   @@ -0,0 +1,32 @@
   +    // Make sure that a definition exists for the target entity. The providing
   +    // module may have been uninstalled.
   +    if (\Drupal::entityTypeManager()->hasDefinition($comment_type->getTargetEntityTypeId())) {
   +      $comment_type->save();
   +    }
   
   +++ b/core/modules/comment/src/Entity/CommentType.php
   @@ -99,4 +99,14 @@ public function getTargetEntityTypeId() {
   +    parent::calculateDependencies();
   +    $target_entity_type = \Drupal::entityTypeManager()->getDefinition($this->getTargetEntityTypeId());
   +    $this->addDependency('module', $target_entity_type->getProvider());
   +    return $this;
   

   So in the update we defend against missing target entity providers and in calculateDependencies() we don't. Should we be deleting comment types in this position? Or should calculateDependencies() be similarly defensive. Not sure.

@alexpott, in update we deal with the case where a comment type was created for a target entity whose module has been disabled in the meantime. For such cases we decided not to delete them but, in #2733819: Minimize the effect of a comment type's target entity provider being uninstalled., we raise the red flag.

In calculateDependecies() we don't want to defend, we want to crash because that should be a very edge case, happening in very special circumstances like creating comment types through API and not ensuring a reliable target entity type. Most of the time the comment types are created through the UI where the user cannot select invalid target entity types. So, I don't think we want to mute such events.

@claudiu.cristea hmmm... this doesn't make sense to me - we're leaving around invalid config that can't be saved - because of this change. That's just not right. That means we couldn't ever again write a generic update all the config thing like system_post_update_recalculate_configuration_entity_dependencies() because something might fail. It also means that on these sites config import is completely broken.

@alexpott, you propose to delete them in update?

Re-rolling patch against 8.6.x branch.
I have also fixed couple of issues mentioned by @alexpott
Fixed usage of DI
We still needs some inputs from @alexpott about implementation approach as mentioned by @claudiu.cristea

Attempt to fix test case failures.

This issue is being reviewed by the kind folks in Slack, #needs-review-queue-initiative. We are working to keep the size of Needs Review queue [2700+ issues] to around 400 (1 month or less), following Review a patch or merge request as a guide.

At this time we will need a D10 MR of this issue.
Since it's been 5 years tagged for IS update if any of the remaining tasks have changed as the module has been updated some.