The 'entity_type' and 'field_name' base fields on Comment are required

I just had a look at the table itself: `entity_type` varchar(32) CHARACTER SET ascii DEFAULT NULL,
this means that setting it required would also have to be reflected in the schema itself, right?

The test seems to prove you wrong.

After some debugging it turns out #2841291: Fix NOT NULL handling in the entity storage and 'primary key' changes when updating the storage definition of an identifier field would be needed to automatically change it from NOT NULL FALSE to NOT NULL TRUE, because \Drupal\Core\Entity\Sql\SqlContentEntityStorageSchema::getSharedTableFieldSchema doesn't take into account the required setting.

Here is a small fix to ensure the query in the test actually works with a table prefix.

Reroll

Not quite sure why we need to specifically make it storage required?

Just posting an alternative patch here from #2975217: Update the default comment entity owner to the current user for now that makes entity_type and field_name regular-required, which doesn't require an update path as it has no effect on storage and also makes the comment name validator more resilient on incomplete data.

Fine if we want to make it storage-required for some reason and combine the two patches, just want to move my patch out of that other issue.

interdiff is against the past patch over there.

makes entity_type and field_name regular-required, which doesn't require an update path as it has no effect on storage

The "storage required" setting also doesn't have any effect on the storage yet! But that will change in #2841291: Fix NOT NULL handling in the entity storage and 'primary key' changes when updating the storage definition of an identifier field, and committing this patch without the corresponding not null additions to \Drupal\comment\CommentStorageSchema::getSharedTableFieldSchema() (and the upgrade path for them) is just kicking the problem down the road and introduces an inconsistency between the field storage definitions and the SQL storage column definitions, which is something I don't really agree with :/

Here's a complete fix, basically a combined version of the patch from #18 with an improved version of #15.

Since this is a bug fix and it's supposed to land in both 8.6.x and 8.7.x, separate patches are required because of #2949964: Add an EntityOwnerTrait to standardize the base field needed by EntityOwnerInterface.

Now that the field_name column is actually required at the database level, some kernel tests need to be updated because they weren't adding comments properly.

+++ b/core/modules/comment/tests/src/Functional/Update/CommentUpdateTest.php
@@ -73,6 +73,34 @@ public function testPublishedEntityKey() {
+    if ($database->driver() === 'mysql') {
+      $table_description = $database
+        ->query('DESCRIBE {comment_field_data}')
+        ->fetchAllAssoc('Field');

I don't think we use this technique anywhere else, is this guaranteed to be stable across MySQL versions?

Could we try inserting NULL, and check that it succeeds before the update and fails afterwards?

The test failures from #22 and the fixes required for them in #25 prove exactly that, so we could remove the update path test if it's deemed too icky because it's only testing the behavior in MySQL.

+++ b/core/modules/comment/src/CommentStorageSchema.php
@@ -60,6 +60,13 @@ protected function getSharedTableFieldSchema(FieldStorageDefinitionInterface $st
 
+        case 'entity_type':
+        case 'field_name':
+          // 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;

this doesn't check that the field is actually set to required, so I think any storage update on that entity type/field is going to trigger the update already, e.g. comment_update_8301(), not just the one we think that does. Not 100% sure, but I wouldn't be surprised if the update path tests passes *without* the new update function :)

I'm always a bit worried about not null storage changes, I kind of expect that there are sites out there that have a messed up database where this update would then fail for them. That's the advantage of just making it required for the validation :)

Discussed a bit with @Berdir and settled on fixing the first paragraph from #30 by doing a 'is storage required' check on those two fields.

As for the second paragraph of #30, no site can get into the situation of having NULL values for those columns in the comment tables thanks to the comment_entity_statistics table schema, which defines its own entity_type and field_name columns as NOT NULL, and that table is updated on each comment save in \Drupal\comment\Entity\Comment::postSave(). If we would have NULL values for any of those two columns, the transaction from \Drupal\Core\Entity\Sql\SqlContentEntityStorage::save would fail :)

Right, and that logic is actually why REST integration was also exploding as our test coverage showed.

Perfect, thanks!

So agreed with the patch, but I think this should be 8.7.x-only, which means we need an 87**() update function.

Okay :)

Rtbc++

That was a random failure.

Committed 876c6c1 and pushed to 8.7.x. Thanks!

I already made new issue about the problem https://www.drupal.org/project/drupal/issues/3052147

This update hook fails on comments that has no value submitted for field_name

Retroactively added a stub change record for this issue to reference from a hook_requirements patch: https://www.drupal.org/node/3053046.

The question is why drupal comments have a title. In no comment service, such as Facebook, or those used in newspapers have a title. I have disabled it to make the comment service more efficient. It occupies space and does not result in anything. The solution: do not update or use external comment services.

You can add a hook_comment_presave() and add something into the subject field, like the first few characters of the text.

There are open issues I think to make it optional, this merely enforced what was already expected.