[backport] Can't create comments when comment is a base field

+++ b/core/modules/comment/src/Entity/Comment.php
@@ -512,8 +512,8 @@ public function setThread($thread) {
+      $fields = \Drupal::getContainer()->get('entity_field.manager')->getFieldStorageDefinitions($values['entity_type']);

Can you use \Drupal::service here instead of \Drupal::getContainer()

Nice catch, never considered case where someone would do this, we'd need to add a test for this, which would basically be a new test-module that modified the entity_test field definitions to add a comment field and then tried to add a comment.

Test this against 8.3.x.

This different issue popped in my head when I saw this one.

I would like to work on this issue .

Here is the rerolled patch which also follows the suggestion in #4 .

The patch still applies and solves the issue for my projects. The container call has also been replaced by the service and I have no further issues with the solution.

I am rerunning the test for PHP 7, but am wondering whether we should add the suggested test (#4) in this issue or create a new one. In any case, we also still need a change record.

Add the testing script.

I am not understand why the test is fail. Can someone give some suggest?

Hi,

+++ b/core/modules/comment/tests/src/Kernel/CommentBaseFieldTest.php
@@ -0,0 +1,35 @@
+use Drupal\Tests\token\Kernel\KernelTestBase;

Your patch is using the KernelTestBase from token. This module is a contrib module and can therefore not be found in core. DrupalCI also gave this error on rerun:

Fatal error: Class 'Drupal\Tests\token\Kernel\KernelTestBase' not found in /var/www/html/core/modules/comment/tests/src/Kernel/CommentBaseFieldTest.php on line 13

Perhaps you could use "Drupal\KernelTests\KernelTestBase" instead?

@LaravZ Thank you very much. I have miss selected the PhpStorm hint before.

Patch still applies cleanly and the test has been added. I am wondering whether we should also test adding the field to an entity.

I also have the following question:

+++ b/core/modules/comment/tests/src/Kernel/CommentBaseFieldTest.php
@@ -0,0 +1,35 @@
+    $entity->save();

Would it be possible to assert whether the entity was saved correctly? The save() function should return a SAVED_NEW int or EntityStorageException in any case.

+++ b/core/modules/comment/tests/src/Kernel/CommentBaseFieldTest.php
@@ -0,0 +1,35 @@
+  public function testCommentBaseField() {
+    // Verify entity creation.
+    $entity = CommentTestBaseField::create();
+    $entity->name->value = $this->randomMachineName();

We need to create a comment on the given entity, to trigger the ::preCreate method, and then we need to assert that the comment_type value is added.

I have tested and reviewed this extensively in the context of a helpdesk module which uses comments as a base field. Every possible use case has been tested and it works as expected. RTBC

Just a re-roll for 9.x and 8.x core

As 9.0 test fails - patches should go 9.x first

Declaring ::setUp without a void return typehint in Drupal\Tests\comment\Kernel\CommentBaseFieldTest is deprecated in drupal:9.0.0. Typehinting will be required before drupal:10.0.0. See https://www.drupal.org/node/3114724

Updated patch and fixed test case failure issue by adding protected function setUp(): void {, please review.

Reviewed the revised patch, looking good.

Looks unrelated fail Drupal\Tests\media_library\FunctionalJavascript\WidgetUploadTest

Reviewed this as part of bug-smash, RTBC+1

Committed 65e5258 and pushed to 9.1.x. Thanks!

This can be backported to 9.0.x and 8.9.x once the branches are unfrozen from this week's release.

Backported to 9.0.x and 8.9.x.

The commits in #44 and #46 introduced a regression - they use public static $modules in the test instead of protected static $modules.

I posted the fixes in #3164721: More uses of public static $modules - please review!