comment_update_8701 fails if there are comments without field_name

So this is directly because of #2885809: The 'entity_type' and 'field_name' base fields on Comment are required which is meant to enforce that they are required.

I don't know if we should skip the update for comments without a field name, as sites like yours missing that data would still have a data integrity problem.

Is it possible to investigate how the comments came to have no field name to begin with? Is there a particular contrib module that caused this?

As per #5

In reality I think you're going to have to find and delete those comments. Because their functionality is likely to be broken, throughout core we expect that field to be populated.

We could try a "best guess" approach and fill the missing data if there's only one comment field available.

Marked #3052256: database update fails after 8.7.0 update on altering comment_field_data as a duplicate of this issue.

An approach @cilefen suggested for the update hook was checking for data where the newly required fields were null, and logging an error if the update can't be made. That would make the update more defensive, so that sites with busted comment data can still update Drupal. I think we should try to mitigate it since we've gotten multiple reports. If we did that, I'd also want to add a release note about it explaining that the data needs to be fixed manually.

The error from the other issue was:

Failed: Drupal\Core\Entity\EntityStorageException: Exception thrown while performing a schema update. SQLSTATE[22004]: Null value not allowed: 1138 Invalid use of NULL value: ALTER TABLE {comment_field_data} CHANGE `field_name` `field_name` VARCHAR(32) CHARACTER SET ascii COLLATE ascii_general_ci NOT NULL; Array ( ) in Drupal\Core\Entity\Sql\SqlContentEntityStorage->wrapSchemaException() (line 1615 of /web/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorage.php)

I have this issue also. I do not see how to fix it myself manually. It only happens in one of my 6 sites, but in the offending site, core is in web/core as per the "official" way of doing things.
Are you saying that on my site, there are comments with nothing in them?
But the site is quite broken unless I can do the database update. I need a fix please.

I had the same bug on a site Drupal 8 upgraded from a version 7. Having not found a solution, I deleted all the comments of the blog. What misery. Recall that the blog was part of the core of Drupal 7, and more in Drupal 8. I find it painful ... The site in question was not yet in production, but I am really disappointed by this update.

I had the same bug on a site Drupal 8 upgraded from a version 7. Having not found a solution, I deleted all the comments of the blog. What misery. Recall that the blog was part of the core of Drupal 7, and more in Drupal 8. I find it painful ... The site in question was not yet in production, but I am really disappointed by this update.

In your case you should have been able to apply a default value, as the migration would have created a comment field. This sounds like an issue in the migraiton.

@tjtj - do you have multiple comment fields or just one? If just one, you should be able to set the value based on that.

I had an idea that's very similar to the one from @cilefen (before seeing #10), but I would like to propose to be *a lot* more defensive with this update.

I was thinking that we should add a hook_requirements() implementation for the update phase, and actively prevent the update if we find NULL values for the field_name field. That way, we can provide a very clear warning that the corrupted data needs to be fixed _before_ updating to 8.7.0.

However, this feels like a release and/or framework manager decision so I'll wait for some more discussion (or approval) before starting to work it :)

Here is an implementation of #16, which might be helpful to evaluate if this is worth doing.

How this looks in the verbose test output:

This looks great! Thanks @Sam152 :)

1. +++ b/core/modules/comment/comment.install
   @@ -11,6 +11,28 @@
   +  if ($phase === 'update') {
   

   We should add an additional check that the installed schema version is lower than 8701, because we don't want this requirement check to be done for every future update.

2. +++ b/core/modules/comment/comment.install
   @@ -11,6 +11,28 @@
   +      ->range(0, 1)
   ...
   +      ->count()
   

   The count query might be expensive, but I think we can remove it since we're using a range already.

3. +++ b/core/modules/comment/comment.install
   @@ -11,6 +11,28 @@
   +      'description' => t('The comment 8701 update requires that the entity_type and field_name fields have values for all comment entities. See the relevant <a href="https://www.drupal.org/node/3053046">the change record</a> for more information.'),
   

   We need to emphasize the field names somehow, how about this?

   t('The comment_update_8701() function requires that the %field_name_1 and %field_name_2 fields have values for all comment entities. See the <a href="https://www.drupal.org/node/3053046">change record</a> for more information.', [
     '%field_name_1' => 'entity_type',
     '%field_name_2' => 'field_name',
   ])
   

4. +++ b/core/modules/comment/comment.install
   @@ -11,6 +11,28 @@
   +      'severity' => $has_empty_columns ? REQUIREMENT_ERROR : REQUIREMENT_OK,
   

   Maybe we should only return the requirement if $has_empty_columns is TRUE (i.e. wrap the whole thing in an if). What do you think?

1. Good point, adding that.
2. Have previously used count to get something closer to a boolean, but an empty array is just as falsey, so fine by me.
3. Good point, neither the field names nor the change record URL are pertinent to translations either, so extracting them makes sense too.
4. Makes sense, this shouldn't effect most installs so hiding it entirely seems appropriate.

@Sam152, note that for point 3. I also tweaked the text a little in my suggestion by including the name of the update function and removing an extra "the" from the change record link :)

Whoops, missed that.

Great! This looks ready to me.

Hello, I downloaded the 3052147-21.patch to the core module "comment". I tried git apply -v 3052147-21.patch using the command line in the comment core module folder and it didn't work. Instead, I got the following:

Skipped patch 'core/modules/comment/comment.install'.
Skipped patch 'core/modules/comment/tests/fixtures/update/drupal-8.empty-comment-fields.3052147.php'.
Skipped patch 'core/modules/comment/tests/src/Functional/Update/CommentUpdateTest.php'.
Skipped patch 'core/tests/Drupal/FunctionalTests/Update/UpdatePathTestBase.php'.

I am not sure what am I doing wrong, I just need some guidance on applying this patch.

Thank you,

@absbennyboy2753, the patch from this issue does not fix the underlying problem, it just prevents the update if the problem is detected.

The only way to fix it is still to manually update all the comments that don't have a field_name or entity_value value in the database.

+++ b/core/modules/comment/tests/src/Functional/Update/CommentUpdateTest.php
@@ -118,4 +118,18 @@ public function testCommentEntityTypeAndFieldNameRequired() {
+    $this->prepareRunUpdates();

+++ b/core/tests/Drupal/FunctionalTests/Update/UpdatePathTestBase.php
@@ -390,6 +380,22 @@ protected function runUpdates() {
+  /**
+   * Prepare the environment for running updates.
+   */
+  protected function prepareRunUpdates() {

I think it is a bit risky to add a protected method to such a well used base class. Let's do the drupal_rewrite_settings in the test instead. I think you might be able to use the $this->writeSettings() to do this.

the -21 patch failed for me

[~/# ./drush updb
 [error]  The comment_update_8701() function requires that the <em class="placeholder">entity_type</em> and <em class="placeholder">field_name</em> fields have values for all comment entities. See the <a href="https://www.drupal.org/node/3053046">change record</a> for more information.

>  [notice] Update started: comment_update_8701
>  [error]  Exception thrown while performing a schema update. SQLSTATE[22004]: Null value not allowed: 1138 Invalid use of NULL value: ALTER TABLE {comment_field_data} CHANGE `field_name` `field_name` VARCHAR(32) CHARACTER SET ascii COLLATE ascii_general_ci NOT NULL; Array
> (
> )
>
>  [error]  Update failed: comment_update_8701
 [error]  Update aborted by: comment_update_8701
 [error]  Finished performing updates.

@tjtj, see #24, this patch does not fix the corrupted data in the database, it has to be done manually.

Addressing feedback. $this->writeSettings seems to work for me.

Hoping the testbot agrees as well :)

I think that before we commit this https://www.drupal.org/node/3053046 needs some information as we're pointing people there for more information are there is none.

I've updated https://www.drupal.org/node/3053046 with a description of the problem and some instructions.

@catch thanks for filling out the CR. I think it'd be great if there was at least some non-destructive path that we could inform people of. As per #14 - some idea of how to provide a default value might be nice.

@Dropa the change record can be updated by everyone. I think @catch's solution is important because many people do not have access to drush so that's at least a way around it. But also you're right that updating thousands of comments that way is not practical.

Setting back to needs work so that the change record can be updated to at least suggest some non-destructive operations.

I deliberately didn't add non-destructive instructions for the following reasons:

1. If there's no entity ID, you can't just assign comments to a random entity ID - they could show up where they're not supposed to.

2. Even if you have an entity ID, you can't easily discover which entity type it is.

3. Adding a default value means running an UPDATE {comment_field_data} SET field_name = :foo, entity_type = :bar WHERE field_name IS NULL AND entity_type IS NULL. If someone leaves off the WHERE clause they could destroy their entire comments table making things worse, and if they need to put different entity types and field names then they'd need to modify the query.

So the option of deleting comments, that already can't be seen anywhere, seems less destructive for someone that doesn't understand Drupal core schema and comment architecture, than the non-destructive option which which is a lot more complex.

Someone who wants to keep the comments can of course decide to do something else.

So the option of deleting comments, that already can't be seen anywhere, seems less destructive for someone that doesn't understand Drupal core schema and comment architecture, than the non-destructive option which which is a lot more complex.

Ahh I didn't realise that these comments could not be seen anywhere.

I tested this assumption on 8.6.x and yeah if either field_name or entity_type are NULL in comment_field_data then the comment is not displayed on the entity.

They do however appear on the recent comments block and the link to the entity works but as above the comments are not displayed. Hmmm tricky. @catch do you feel it is worth at least mentioning some of this complexity in the CR?

@catch do you feel it is worth at least mentioning some of this complexity in the CR?

I can see something like:

If you discover a lot of comments with invalid data or would like to preserve the comments for any other reason, you will need to do further investigation and may need to write custom SQL updates or PHP. See [the issue] for some discussion about approaches.

Then if people develop generic workarounds we could update the CR later.

The comments will indeed show in 'recent comments' and probably on the comment admin page, but since those fields don't have widgets there's nothing much you can do with them via the UI.

@catch thanks for the wording. I've updated https://www.drupal.org/node/3053046 with your suggestion and a little bit of html. How does that look for you?

That looks good to me. I'm not opposed to adding more specifics but I think we could do it after commit too.

Ouch. :\ The only way out of this situation being to destroy data (maybe dozens or hundreds of entries) is a pretty sobering situation to be in...

This might be a silly question, but why can we not do a query like the one in #34 in the update if we detect this condition? Set field_name to "unknown" and programmatically create a stub entity with a machine name of "unknown" or something and assign entity_type there, flag an error in the logs and tell people to look at their comments and manually sort them out?

Committed and pushed e52604b4d2 to 8.8.x and c35a821e96 to 8.7.x. Thanks!

diff --git a/core/modules/comment/tests/fixtures/update/drupal-8.empty-comment-fields.3052147.php b/core/modules/comment/tests/fixtures/update/drupal-8.empty-comment-fields.3052147.php
index 2d648629f0..b57a784b1a 100644
--- a/core/modules/comment/tests/fixtures/update/drupal-8.empty-comment-fields.3052147.php
+++ b/core/modules/comment/tests/fixtures/update/drupal-8.empty-comment-fields.3052147.php
@@ -11,28 +11,28 @@
 $connection = Database::getConnection();
 
 $connection->insert('comment')
-  ->fields(array(
+  ->fields([
     'cid',
     'comment_type',
     'uuid',
     'langcode',
-  ))
-  ->values(array(
+  ])
+  ->values([
     'cid' => '5',
     'comment_type' => 'comment',
     'uuid' => '2f0505ad-fdc7-49fc-9d39-571bfc3e0f88',
     'langcode' => 'en',
-  ))
-  ->values(array(
+  ])
+  ->values([
     'cid' => '6',
     'comment_type' => 'comment',
     'uuid' => '3be94e6b-4506-488a-a861-9742a18f0507',
     'langcode' => 'en',
-  ))
+  ])
   ->execute();
 
 $connection->insert('comment__comment_body')
-  ->fields(array(
+  ->fields([
     'bundle',
     'deleted',
     'entity_id',
@@ -41,8 +41,8 @@
     'delta',
     'comment_body_value',
     'comment_body_format',
-  ))
-  ->values(array(
+  ])
+  ->values([
     'bundle' => 'comment',
     'deleted' => '0',
     'entity_id' => '5',
@@ -51,8 +51,8 @@
     'delta' => '0',
     'comment_body_value' => "<p>Comment body</p>\r\n",
     'comment_body_format' => 'basic_html',
-  ))
-  ->values(array(
+  ])
+  ->values([
     'bundle' => 'comment',
     'deleted' => '0',
     'entity_id' => '6',
@@ -61,11 +61,11 @@
     'delta' => '0',
     'comment_body_value' => "<p>Comment body</p>\r\n",
     'comment_body_format' => 'basic_html',
-  ))
+  ])
   ->execute();
 
 $connection->insert('comment_field_data')
-  ->fields(array(
+  ->fields([
     'cid',
     'comment_type',
     'langcode',
@@ -84,8 +84,8 @@
     'entity_type',
     'field_name',
     'default_langcode',
-  ))
-  ->values(array(
+  ])
+  ->values([
     'cid' => '5',
     'comment_type' => 'comment',
     'langcode' => 'en',
@@ -104,8 +104,8 @@
     'entity_type' => NULL,
     'field_name' => 'field_test_2',
     'default_langcode' => '1',
-  ))
-  ->values(array(
+  ])
+  ->values([
     'cid' => '6',
     'comment_type' => 'comment',
     'langcode' => 'en',
@@ -124,5 +124,5 @@
     'entity_type' => 'node',
     'field_name' => NULL,
     'default_langcode' => '1',
-  ))
+  ])
   ->execute();

Fixed the array syntax on commit - well I used PHPStorm too.

This might be a silly question, but why can we not do a query like the one in #34 in the update if we detect this condition? Set field_name to "unknown" and programmatically create a stub entity with a machine name of "unknown" or something and assign entity_type there, flag an error in the logs and tell people to look at their comments and manually sort them out?

We should check what happens if you set 'entity_type' and 'field_name' to something arbitrary like 'unknown' and then load the comment edit form or view it in the admin or recent comments view. If it doesn't error out that might be an option here. It will still require manually fixing this data in order for those comments to work.

I've opened #3054746: Alternate fix for comment_update_8701 fails if there are comments without field_name to explore #39

@Dropa @absbennyboy2753 @tjtj @floown Thanks for taking part in this issue. If you'd like to help us make sure the 8.8.0 update is as smooth as possible, please consider signing up for the beta testing program at https://goo.gl/forms/bMBTMRSY3sKEscUJ3

This is still not fixed for me in 8.7.2.

How do I manually fix every comment? trying to edit comments gives me the site has encountered a problem error. I had to delete all my comments!

And I do not understand why this problem suddenly came up. I had no problems before 8.7. The comments were made properly.

error] The comment_update_8701() function requires that the entity_type and field_name fields have values for all comment entities. See the change record for more information.

You need to review the change record and follow the steps there before entering 'yes' to continue