Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Experiencing following error while updating from 8.6.15 to 8.7.0.
> [notice] Update started: comment_update_8701
> [error] Exception thrown while performing a schema update. SQLSTATE[01000]: Warning: 1265 Data truncated for column 'field_name' at row 1: 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
This is caused by empty values in field_name
fields in database
Comment | File | Size | Author |
---|---|---|---|
#28 | 3052147-28.patch | 5.91 KB | Sam152 |
#28 | interdiff.txt | 2.67 KB | Sam152 |
#21 | 3052147-21.patch | 7.25 KB | Sam152 |
#21 | interdiff.txt | 2.09 KB | Sam152 |
#19 | 3052147-19.patch | 7.24 KB | Sam152 |
Comments
Comment #2
Dropa CreditAttribution: Dropa at Mediamaisteri Oy commentedWas able to narrow the problem down. We have comments that do not have anything inserted into
field_name
field.I believe this should be noticed in the update hook, since the field previously was not required either.
Comment #3
Dropa CreditAttribution: Dropa at Mediamaisteri Oy commentedComment #4
xjmComment #5
xjmSo 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?
Comment #6
larowlanAs per #5
Comment #7
larowlanIn 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.
Comment #8
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedWe could try a "best guess" approach and fill the missing data if there's only one comment field available.
Comment #9
Dropa CreditAttribution: Dropa at Mediamaisteri Oy commentedI'm fairly aware that in our case we've to manually solve this, since we're not using comment entity as a direct comment to some other entity.
In our case we have trainings as one entity, and those trainings has editable questions, which one could be like food allergies, then there's second entity that indicates the enrollment to that training, where user gets to answer in those questions. These questions are now comments that do point to enrollment entity, but enrollment has no field for the comments.
However, since the field previously was not marked as required, it did not count as error in validation, so if there is even one comment that passed that, it's going to be problem for others as well, and they might not be able to even tell why that happened.
Comment #10
xjmMarked #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.
Comment #11
xjmThe error from the other issue was:
Comment #12
tjtj CreditAttribution: tjtj commentedI 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.
Comment #13
floown CreditAttribution: floown commentedI 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.
Comment #14
larowlanIn 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.
Comment #15
larowlan@tjtj - do you have multiple comment fields or just one? If just one, you should be able to set the value based on that.
Comment #16
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedI 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 theupdate
phase, and actively prevent the update if we find NULL values for thefield_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 :)
Comment #17
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedHere is an implementation of #16, which might be helpful to evaluate if this is worth doing.
How this looks in the verbose test output:
Comment #18
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedThis looks great! Thanks @Sam152 :)
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.
The count query might be expensive, but I think we can remove it since we're using a range already.
We need to emphasize the field names somehow, how about this?
Maybe we should only return the requirement if
$has_empty_columns
is TRUE (i.e. wrap the whole thing in anif
). What do you think?Comment #19
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commented1. 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.
Comment #20
amateescu CreditAttribution: amateescu for Pfizer, Inc. commented@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 :)
Comment #21
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedWhoops, missed that.
Comment #22
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedGreat! This looks ready to me.
Comment #23
absbennyboy2753 CreditAttribution: absbennyboy2753 as a volunteer commentedHello, 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,
Comment #24
amateescu CreditAttribution: amateescu for Pfizer, Inc. commented@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
orentity_value
value in the database.Comment #25
alexpottI 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.Comment #26
tjtj CreditAttribution: tjtj commentedthe -21 patch failed for me
Comment #27
amateescu CreditAttribution: amateescu for Pfizer, Inc. commented@tjtj, see #24, this patch does not fix the corrupted data in the database, it has to be done manually.
Comment #28
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedAddressing feedback.
$this->writeSettings
seems to work for me.Comment #29
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedHoping the testbot agrees as well :)
Comment #30
alexpottI 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.
Comment #31
catchI've updated https://www.drupal.org/node/3053046 with a description of the problem and some instructions.
Comment #32
Dropa CreditAttribution: Dropa at Mediamaisteri Oy commentedI believe the description should not directly tell you to remove your comments. (#3052147-31: comment_update_8701 fails if there are comments without field_name) In my opinion that should be the "if nothing else" -case.
Instead I'd point out that the code that actually generates these faulty comments should be fixed, and when doing so, it should also lead you towards the correct way of fixing the database.
Also, it's comments, there's good chance there's a ***ton of 'em in database, guide that tells you to individually go to delete link for each is going to be nightmare.
Instead the (if nothing else) operation can be done easily by following:
Copy-paste that code into some file, and run
drush scr <path_to_file>
. Just make sure that someone will fix the actual reason you've these comments in first place.Comment #33
alexpott@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.
Comment #34
catchI 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.
Comment #35
alexpottAhh 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?
Comment #36
catchI can see something like:
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.
Comment #37
alexpott@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?
Comment #38
catchThat looks good to me. I'm not opposed to adding more specifics but I think we could do it after commit too.
Comment #39
webchickOuch. :\ 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?
Comment #40
alexpottCommitted and pushed e52604b4d2 to 8.8.x and c35a821e96 to 8.7.x. Thanks!
Fixed the array syntax on commit - well I used PHPStorm too.
Comment #43
catchWe 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.
Comment #44
alexpottI've opened #3054746: Alternate fix for comment_update_8701 fails if there are comments without field_name to explore #39
Comment #45
pameeela CreditAttribution: pameeela as a volunteer commented@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
Comment #46
tjtj CreditAttribution: tjtj commentedThis 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.
Comment #47
larowlanYou need to review the change record and follow the steps there before entering 'yes' to continue