Issue #2885809 by amateescu, Wim Leers, dawehner, Berdir, andypost: The...

Issue #2885809 by amateescu, Wim Leers, dawehner, Berdir, andypost: The 'entity_type' and 'field_name' base fields on Comment are required

core/modules/comment/comment.install

@@ -220,3 +220,17 @@ function comment_update_8700() {
   $definition_update_manager->updateEntityType($entity_type);
   $definition_update_manager->updateFieldStorageDefinition($definition_update_manager->getFieldStorageDefinition('uid', 'comment'));
 }
+
+/**
+ * Make the 'entity_type' and 'field_name' comment fields required.
+ */
+function comment_update_8701() {
+  $definition_update_manager = \Drupal::entityDefinitionUpdateManager();
+  $field_definition = $definition_update_manager->getFieldStorageDefinition('entity_type', 'comment');
+  $field_definition->setRequired(TRUE);
+  $definition_update_manager->updateFieldStorageDefinition($field_definition);
+
+  $field_definition = $definition_update_manager->getFieldStorageDefinition('field_name', 'comment');
+  $field_definition->setRequired(TRUE);
+  $definition_update_manager->updateFieldStorageDefinition($field_definition);
+}

core/modules/comment/src/CommentStorageSchema.php

@@ -5,6 +5,7 @@
 use Drupal\Core\Entity\ContentEntityTypeInterface;
 use Drupal\Core\Entity\Sql\SqlContentEntityStorageSchema;
 use Drupal\Core\Field\FieldStorageDefinitionInterface;
+use Drupal\Core\Field\RequiredFieldStorageDefinitionInterface;
 
 /**
  * Defines the comment schema handler.
@@ -60,6 +61,16 @@ protected function getSharedTableFieldSchema(FieldStorageDefinitionInterface $st
           $schema['fields'][$field_name]['not null'] = TRUE;
           break;
 
+        case 'entity_type':
+        case 'field_name':
+          assert($storage_definition instanceof RequiredFieldStorageDefinitionInterface);
+          if ($storage_definition->isStorageRequired()) {
+            // The 'entity_type' and 'field_name' are required so they also need
+            // to be marked as NOT NULL.
+            $schema['fields'][$field_name]['not null'] = TRUE;
+          }
+          break;
+
         case 'created':
           $this->addSharedTableFieldIndex($storage_definition, $schema, TRUE);
           break;

core/modules/comment/src/Entity/Comment.php

@@ -306,12 +306,14 @@ public static function baseFieldDefinitions(EntityTypeInterface $entity_type) {
 
     $fields['entity_type'] = BaseFieldDefinition::create('string')
       ->setLabel(t('Entity type'))
+      ->setRequired(TRUE)
       ->setDescription(t('The entity type to which this comment is attached.'))
       ->setSetting('is_ascii', TRUE)
       ->setSetting('max_length', EntityTypeInterface::ID_MAX_LENGTH);
 
     $fields['field_name'] = BaseFieldDefinition::create('string')
       ->setLabel(t('Comment field name'))
+      ->setRequired(TRUE)
       ->setDescription(t('The field name through which this comment was added.'))
       ->setSetting('is_ascii', TRUE)
       ->setSetting('max_length', FieldStorageConfig::NAME_MAX_LENGTH);

core/modules/comment/src/Plugin/Validation/Constraint/CommentNameConstraintValidator.php

@@ -72,8 +72,10 @@ public function validate($entity, Constraint $constraint) {
       }
     }
 
-    // Anonymous account might be required - depending on field settings.
-    if ($owner_id === 0 && empty($author_name) &&
+    // Anonymous account might be required - depending on field settings. We
+    // can't validate this without a valid commented entity, which will fail
+    // the validation elsewhere.
+    if ($owner_id === 0 && empty($author_name) && $entity->getCommentedEntity() && $entity->getFieldName() &&
       $this->getAnonymousContactDetailsSetting($entity) === COMMENT_ANONYMOUS_MUST_CONTACT) {
       $this->context->buildViolation($constraint->messageRequired)
         ->atPath('name')

core/modules/comment/tests/src/Functional/Rest/CommentResourceTestBase.php

@@ -291,35 +291,17 @@ public function testPostDxWithoutCriticalBaseFields() {
     // DX: 422 when missing 'entity_type' field.
     $request_options[RequestOptions::BODY] = $this->serializer->encode(array_diff_key($this->getNormalizedPostEntity(), ['entity_type' => TRUE]), static::$format);
     $response = $this->request('POST', $url, $request_options);
-    // @todo Uncomment, remove next 3 lines in https://www.drupal.org/node/2820364.
-    $this->assertSame(500, $response->getStatusCode());
-    $this->assertSame(['text/plain; charset=UTF-8'], $response->getHeader('Content-Type'));
-    $this->assertStringStartsWith('The website encountered an unexpected error. Please try again later.</br></br><em class="placeholder">Symfony\Component\HttpKernel\Exception\HttpException</em>: Internal Server Error in <em class="placeholder">Drupal\rest\Plugin\rest\resource\EntityResource-&gt;post()</em>', (string) $response->getBody());
-    // $this->assertResourceErrorResponse(422, "Unprocessable Entity: validation failed.\nentity_type: This value should not be null.\n", $response);
+    $this->assertResourceErrorResponse(422, "Unprocessable Entity: validation failed.\nentity_type: This value should not be null.\n", $response);
 
     // DX: 422 when missing 'entity_id' field.
     $request_options[RequestOptions::BODY] = $this->serializer->encode(array_diff_key($this->getNormalizedPostEntity(), ['entity_id' => TRUE]), static::$format);
-    // @todo Remove the try/catch in favor of the two commented lines in
-    // https://www.drupal.org/node/2820364.
-    try {
-      $response = $this->request('POST', $url, $request_options);
-      // This happens on DrupalCI.
-      // $this->assertSame(500, $response->getStatusCode());
-    }
-    catch (\Exception $e) {
-      // This happens on Wim's local machine.
-      // $this->assertSame("Error: Call to a member function get() on null\nDrupal\\comment\\Plugin\\Validation\\Constraint\\CommentNameConstraintValidator->getAnonymousContactDetailsSetting()() (Line: 96)\n", $e->getMessage());
-    }
-    // $response = $this->request('POST', $url, $request_options);
-    // $this->assertResourceErrorResponse(422, "Unprocessable Entity: validation failed.\nentity_type: This value should not be null.\n", $response);
+    $response = $this->request('POST', $url, $request_options);
+    $this->assertResourceErrorResponse(422, "Unprocessable Entity: validation failed.\nentity_id: This value should not be null.\n", $response);
 
-    // DX: 422 when missing 'entity_type' field.
+    // DX: 422 when missing 'field_name' field.
     $request_options[RequestOptions::BODY] = $this->serializer->encode(array_diff_key($this->getNormalizedPostEntity(), ['field_name' => TRUE]), static::$format);
     $response = $this->request('POST', $url, $request_options);
-    // @todo Uncomment, remove next 2 lines in https://www.drupal.org/node/2820364.
-    $this->assertSame(500, $response->getStatusCode());
-    $this->assertSame(['text/plain; charset=UTF-8'], $response->getHeader('Content-Type'));
-    // $this->assertResourceErrorResponse(422, "Unprocessable Entity: validation failed.\nfield_name: This value should not be null.\n", $response);
+    $this->assertResourceErrorResponse(422, "Unprocessable Entity: validation failed.\nfield_name: This value should not be null.\n", $response);
   }
 
   /**

core/modules/comment/tests/src/Functional/Update/CommentUpdateTest.php

@@ -90,4 +90,32 @@ public function testOwnerEntityKey() {
     $this->assertEquals('uid', $entity_type->getKey('owner'));
   }
 
+  /**
+   * Tests whether the 'entity_type' and 'field_name' columns are required.
+   *
+   * @see comment_update_8701()
+   */
+  public function testCommentEntityTypeAndFieldNameRequired() {
+    $database = \Drupal::database();
+    $this->assertEquals(2, $database->query('SELECT count(*) FROM {comment_field_data}')->fetchField());
+    if ($database->driver() === 'mysql') {
+      $table_description = $database
+        ->query('DESCRIBE {comment_field_data}')
+        ->fetchAllAssoc('Field');
+      $this->assertEquals('YES', $table_description['entity_type']->Null);
+      $this->assertEquals('YES', $table_description['field_name']->Null);
+    }
+
+    $this->runUpdates();
+
+    $this->assertEquals(2, $database->query('SELECT count(*) FROM {comment_field_data}')->fetchField());
+    if ($database->driver() === 'mysql') {
+      $table_description = $database
+        ->query('DESCRIBE {comment_field_data}')
+        ->fetchAllAssoc('Field');
+      $this->assertEquals('NO', $table_description['entity_type']->Null);
+      $this->assertEquals('NO', $table_description['field_name']->Null);
+    }
+  }
+
 }

core/modules/comment/tests/src/Kernel/CommentItemTest.php

@@ -70,6 +70,7 @@ public function testCommentItem() {
    */
   public function testCommentAuthorName() {
     $this->installEntitySchema('comment');
+    $this->addDefaultCommentField('entity_test', 'entity_test', 'comment');
 
     $host = EntityTest::create(['name' => $this->randomString()]);
     $host->save();
@@ -81,6 +82,7 @@ public function testCommentAuthorName() {
       'name' => 'entity-test',
       'mail' => 'entity@localhost',
       'entity_type' => 'entity_test',
+      'field_name' => 'comment',
       'entity_id' => $host->id(),
       'comment_type' => 'entity_test',
       'status' => 1,
@@ -99,6 +101,7 @@ public function testCommentAuthorName() {
       'mail' => 'test@example.com',
       'homepage' => 'https://example.com',
       'entity_type' => 'entity_test',
+      'field_name' => 'comment',
       'entity_id' => $host->id(),
       'comment_type' => 'entity_test',
       'status' => 1,

core/modules/comment/tests/src/Kernel/Views/CommentAdminViewTest.php

@@ -98,6 +98,7 @@ protected function setUp($import_test_views = TRUE) {
       'subject' => 'My comment title',
       'uid' => $this->adminUser->id(),
       'entity_type' => 'entity_test',
+      'field_name' => 'comment',
       'comment_type' => 'comment',
       'status' => 1,
       'entity_id' => $entity->id(),
@@ -113,6 +114,7 @@ protected function setUp($import_test_views = TRUE) {
       'mail' => 'test@example.com',
       'homepage' => 'https://example.com',
       'entity_type' => 'entity_test',
+      'field_name' => 'comment',
       'comment_type' => 'comment',
       'created' => 123456,
       'status' => 1,

core/modules/comment/tests/src/Kernel/Views/CommentLinksTest.php

@@ -51,6 +51,7 @@ public function testLinkApprove() {
     $comment = $this->commentStorage->create([
       'uid' => $this->adminUser->id(),
       'entity_type' => 'entity_test',
+      'field_name' => 'comment',
       'entity_id' => $host->id(),
       'comment_type' => 'entity_test',
       'status' => 0,

core/modules/comment/tests/src/Kernel/Views/CommentUserNameTest.php

@@ -78,6 +78,7 @@ protected function setUp($import_test_views = TRUE) {
       'uid' => $this->adminUser->id(),
       'name' => $this->adminUser->label(),
       'entity_type' => 'entity_test',
+      'field_name' => 'comment',
       'entity_id' => $host->id(),
       'comment_type' => 'entity_test',
       'status' => 1,
@@ -91,6 +92,7 @@ protected function setUp($import_test_views = TRUE) {
       'mail' => 'test@example.com',
       'homepage' => 'https://example.com',
       'entity_type' => 'entity_test',
+      'field_name' => 'comment',
       'entity_id' => $host->id(),
       'comment_type' => 'entity_test',
       'created' => 123456,

core/modules/comment/tests/src/Kernel/Views/CommentViewsFieldAccessTest.php

@@ -45,6 +45,7 @@ public function testCommentFields() {
       'subject' => 'My comment title',
       'uid' => $user->id(),
       'entity_type' => 'entity_test',
+      'field_name' => 'comment',
       'entity_id' => $host->id(),
       'comment_type' => 'entity_test',
     ]);
@@ -57,6 +58,7 @@ public function testCommentFields() {
       'mail' => 'test@example.com',
       'homepage' => 'https://example.com',
       'entity_type' => 'entity_test',
+      'field_name' => 'comment',
       'entity_id' => $host->id(),
       'comment_type' => 'entity_test',
       'created' => 123456,