Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
Problem/Motivation
I migrated my D7 site to 8.4.x-dev. When migrating forum content, I noticed that the Replies and Last reply columns of the forum thread list view are not taking the migrated comments into account.
Please see the attached screenshot:
- The first thread in the screenshot has actually 32 migrated comments and 1 comment added in D8. The view is showing that the thread would only have 1 comment instead of 33. This one comment is the one added in D8.
- The second thread has only 1 comment which was added in D8. This comment is shown correctly in the comment count.
- The third thread is created in D8 and it has 1 comment which was added in D8. This comment is shown correctly in the comment count.
- All other threads are migrated ones and they all have comments but the Replies and Last reply columns are not showing them.
- The comments can be seen if you drill into the thread.
- The issue persists even if I clear all caches.
The root cause for this bug is that the database table comment_entity_statistics is populated incorrectly when migrating comments.
- Drupal 8 forum reads the comment statistics from database table comment_entity_statistics with field_name comment_forum. See comments #4 and #5 for more details.
- The migrate process currently creates an incorrect entry to this table with field_name comment_node_forum
- See attached screesnshot on how the entries look like:
When investigating further, this same bug has also another symptom. The built-in content types Article and Forum have two comment types after migration:
- See attached screenshot from article field settings:
- D8 Standard installation profile creates content type Article which ships with comment type comment but the D7-D8 migration adds also comment type comment_node_article. As a result, the content type has now two commen types (note that this only applies to D8 Standard installation profile). When comments are migrated, the comment_entity_statistics table gets incorrectly populated.
- See attached screenshot from forum field settings:
- D8 Forum module uses comment type comment_forum. This comment type is created when D8 Forum module is installed. D7-D8 migration adds also comment type comment_node_forum. As a result, the content type has now two comment types. When comments are migrated, the comment_entity_statistics table gets incorrectly populated as illustrated above in the earlier screenshot.
Proposed resolution
Proposed resolution for Forum
- Do not create comment type comment_node_forum for Forum because Forum uses hard coded comment type comment_forum. Several comment / field migrations need to be updated.
- Use static_map process plugin to map the D7 comment / field migrations to the D8 standard comment_forum
Proposed resolution for Article
- D8 Standard is the only installation profile that creates the comment type comment for Article. In other words, it is not guaranteed that D8 destination already has comment type comment for Article.
- Conclusion in #68-71 is that migration will migrate Article comments to comment_node_article and site builder needs to manually delete the comment field and comment type comment if the D8 site was installed using D8 Standard installation profile.
- Regarding the deletion of the comment field: there is another bug related to it, see #2906470: Orphan comments and entries in comment_entity_statistics after comment field instance has been deleted. However, #2906470 is not a blocker for this migration issue. Before #2906470 lands, the site builder can avoid all problems by manually deleting the comment comment field and comment type before running the migrations.
Remaining tasks
write patch- review it
User interface changes
None.
API changes
None.
Data model changes
None.
Comment | File | Size | Author |
---|---|---|---|
#114 | interdiff-2853872-83-114.txt | 4.58 KB | maxocub |
#114 | 2853872-114.patch | 21.88 KB | maxocub |
#83 | interdiff_79-83.txt | 1.09 KB | heddn |
#83 | 2853872-83.patch | 21.62 KB | heddn |
Comments
Comment #2
masipila CreditAttribution: masipila as a volunteer commentedComment #3
masipila CreditAttribution: masipila as a volunteer commentedI just reproduced this with a vanilla D7. I can now confirm that this has nothing to do with the text formatting issue I mentioned in the issue summary earlier so I removed that part of the issue summary.
Comment #4
masipila CreditAttribution: masipila as a volunteer commentedI was troubleshooting this further. The topic list is returned by ForumManager::getTopics.
The comment count and last comment information is read from the database table comment_entity_statistics:
I had a look at the database table comment_entity_statistics for one of the forum nodes that I migrated. We have two entries in this table, please have a look at the attached screenshot comment_entity_statistics.PNG.
field_name = 'comment_forum'
Markus
Comment #5
masipila CreditAttribution: masipila as a volunteer commentedNotes from further investigation. I installed a vanilla D8 (no migrations) and checked how the comment_entity_statistics table behaves.
Tests and findings:
Conclusion: The D7-D8 comment migration is incorrectly creating entries to comment_entity_statistics with field_name comment_node_forum. It should create the entries with field_name comment_forum.
Additional note: the comment migration seems to create separate comment types for each D7 content type. There is probably some conceptual reason for this behavior but that logic is NOT OK for forum comments according to the analysis above. I'll create a patch that handles forum comments correctly.
Cheers,
Markus
Comment #6
masipila CreditAttribution: masipila as a volunteer commentedThis turned out to be a bit bigger topic than I first thought and I'm not able to create the patch for now. Issue summary updated with a proposed resolution.
Comment #7
masipila CreditAttribution: masipila as a volunteer commentedComment #8
masipila CreditAttribution: masipila as a volunteer commentedI tested this further. Article and Forum content types have duplicate comment fields after the migration. This is because of the same issue than comment_entity_statistics described earlier.
Issue summary updated with the detailed findings.
Comment #9
masipila CreditAttribution: masipila as a volunteer commentedI'll try to get a patch for this during this week.
Comment #10
masipila CreditAttribution: masipila as a volunteer commentedHere's the initial patch for the first round of review. No tests have been updated yet, I'd like to get feedback on the approach first.
Comment #12
masipila CreditAttribution: masipila as a volunteer commentedIssue summary updated to help reviewers. I was previously wondering if we should try to write a hook_update to fix previously migrated (corrupted) data but that goes very difficult very quickly and I don't think that will be realistic.
Comment #13
quietone CreditAttribution: quietone as a volunteer commentedI wonder if we can use the existing cck/field plugins to do the mapping that is needed.
Comment #14
alayham CreditAttribution: alayham commentedI think the root cause of this issue is that comment_forum is hardcoded in core. I created a feature request #2874487: Hardcoded field names in Core should be assigned/returned dynamically to remove hardcoded field names in future versions while we work on solving this problem with the current core hardcoded fields.
Comment #15
phenaproximaApparently this is the flip side of #2669030: D6 Forum vocabulary is migrated to the wrong D8 field name, which is Migrate critical. So this one joins the family too.
Comment #16
phenaproximaThis will certainly need tests.
Comment #17
masipila CreditAttribution: masipila as a volunteer commented@phenaproxima, do you have any input on the proposed approach? See the Remaining Tasks of the issue summary.
Cheers,
Markus
Comment #18
phenaproximaI think my main problem with the approach here is that we are altering source plugins to make special exceptions for certain cases. And I think that is a dangerous path for us to take.
I'd rather we skipped rows, or made the required changes, in process pipelines where things are more visible and explicit. If needed, we can write specialized process plugins to do what's needed.
Granted, I'm saying this off the top of my head, without having read through the issue, so maybe that approach has already been tried and abandoned...but if not, I'd prefer approaching it that way.
Comment #19
masipila CreditAttribution: masipila as a volunteer commentedHi, that approach has not been evaluated and abandonded.
When I analyzed this issue some months back, I had zero experience with the whole migration API and as I wrote with the first quick & dirty patch I started to think that I'm not doing the right thing. That's why I wrote to the issue summary that the very first thing is to evaluate the whole approach.
@quietone was planning to check this issue and provide some guidance on the direction but unfortunately he has not had time to do that.
I've now written some custom migrations for my own sites so I'm now more familiar with the migration API but as I'm still fresh on the topic I would still appreciate some guidance. That is, if you wish me to help with the actual fix in addition to the root cause analysis I've already done. If some of the module maintainers / more experienced regular contributors want to take this I'm of course ok with that as well.
Long story short: I'm offering my help forvthis but I would need some mentoring. I'm on the #drupal-migrate IRC channel so you can also pijg me there.
Cheers,
Markus
Comment #20
masipila CreditAttribution: masipila commentedHi again,
I was educating myself a bit on the process pipeline. Would fhe more elegant approach be as follows, I'll use the migration of comment types as an example:
Problem that we are trying to solve, using comment type migration as an example
Currently d7_comment_type is creating duplicate / unwanted comment types for forum and article. These content types already have the out-of-the-box comment types and we want to use these instead of creating second comment types. See #8 for the details on this.
Introduce process pipeline, still using comment type migration as an example
1. Write a new process plugin which would be called something like skip_on_forum_and_article
2. Modify migration template d7:comment_type so that we first pass the row through the skip_on_forum_and_article. If the row represents forum / article, we skip the row. For all others we continue to the second part of the pipeline which would be like today i.e. we create the new comment type for the current row
As mentioned, I'm still a migration newbie so I'd appreciate some guidance if I'm on the correct / incorrect path with this.
Cheers,
Markus
Comment #21
masipila CreditAttribution: masipila commentedI was thinking this a bit further. What I propose is as follows:
Let's create a new process plugin called skip_if_in_array which takes an array of values as an input parameter. Using this new plugin, we could check for example in comment_type migration if the Row is comment / article and skip these rows.
If you like this approach, I can give this a shot and try to write a patch with this approach.
Cheers,
Markus
Comment #22
masipila CreditAttribution: masipila commentedI managed to write a process plugin skip_row_if_in_blacklist. I'll try to find time to test it tomorrow with these comment types.
Markus
Comment #23
masipila CreditAttribution: masipila commentedUpdate: I'm almost done with this. I'll still do some manual testing and provide the patch hopefully this evening.
Comment #24
masipila CreditAttribution: masipila as a volunteer commentedAlright, here we go finally.
First disclaimer: The patch does not include updates to automated tests. This is a remaining tasks, I've mentioned this also in the issue summary.
The approach has now been changed to what phenaproxima suggested in #18. Instead of hacking and hardcoding the special handling for comment types in the source layer, we handle this now in the process layer. For this, a new process plugin skip_row_if_in_blacklist has been introduced. I wrote this as a generic plugin so that we avoid hard coding and so that this plugin can be re-used for other purposes as well.
As mentioned above, this process plugin allows us to skip comment_node_forum and comment_node_article in multiple comment / field migrations as we will use the out-of-the-box comment_forum and comment instead. Quite many comment and field migrations were affected by this, see a list below.
Updated comment migrations
The following comment migrations are now skipping comment_node_forum and comment_node_article
d7_comment_type
d7_comment_field
d7_comment_field_instance
d7_comment_entity_display
d7_comment_entity_form_display_subject
Remark, pay attenttion when reviewing.
d7_comment_entity_form_display: This is NOT modified. The migration goes through without errors. Please have a second look at this when reviewing!
Finally, d7_comment: This is mapping to the out-of-the-box comment types for forum and article comments using static_map.
Field migrations
The follwing migrations are now skipping comment_node_forum and comment_node_article
d7_field_instance
d7_field_instance_widget_settings
d7_field_formatter_settings
Remark, pay attention when reviewing.
d7_field: I did not add an explicit skipping for the two comment types as the existing code seems to skip the 2 comment types already when the other migration mofidications listed above are applied. When reviewing this, please consider if we want to explicitly skip these two comment types like we do for the other comment/field migrations.
Offtopic: I need to say that despite the last bugs that we have in the D7-D8 migrations, I'm still more than impressed about the migration API and D7-D8 migrations. Hats off for all individuals who have contributed to this piece of art.
Cheers,
Markus
Comment #25
masipila CreditAttribution: masipila as a volunteer commentedScrewed up with the comment # in the file name, sorry :/ should have used 24 instead of 22. Time to get some sleep...
Comment #27
masipila CreditAttribution: masipila as a volunteer commentedI checked the Kernel test failures. I'll fix most of them this evening. There seem to be some bit more complex tests that will probably need input from someone else but the trivial ones I can update.
Markus
Comment #28
masipila CreditAttribution: masipila as a volunteer commentedSetting to Needs review to trigger testbot. One Kernel test has been updated to see the behavior of testbot. No interdiff here, I'll create a second new patch soon after this one after I've seen the testbot results for this one.
Comment #30
masipila CreditAttribution: masipila as a volunteer commentedHmm... The one kernel test that I tried to update is still failing, probably because the patch is intentionally skipping the migration of article comments but the test is still checking that it is migrating.
I'm unassigning this from myself now and let others pick this up. I don't want to just comment out the failing tests - somebody with more conceptual understanding of what each migration is doing should review the patch and then update the tests accordingly.
When reviewing, the latest patch is the only one that makes sense to check.
Cheers,
Markus
Comment #31
maxocub CreditAttribution: maxocub commentedThanks @masipila for working on this. I'll take a look at those failing tests.
Comment #32
maxocub CreditAttribution: maxocub commented@masipila: Great work with the skip_row_if_in_blacklist process plugin, but personally I think we can achieve the same thing with only the static_map plugin.
If instead of skipping rows we map the the correct bundles/field_names with static_map, they would not be duplicated after the migration.
Also, if someone would migrate D7 article comments without having installed the D8 standard profile (where the default comment type comes from), the D7 article comments won't migrate since the default comment type would not exists and the migration would have skip them.
So here's a new patch using only static_map.
Comment #34
maxocub CreditAttribution: maxocub commentedFix the failing test.
Comment #35
masipila CreditAttribution: masipila as a volunteer commentedHi, I didn't test your patch yet but on conceptual level I believe your static_map approach is better than the skip_row_if_in_blacklist approach.
Did you already manually test that the comment/field migrations are actually migrating the D7 settings correctly to D8? The skip_row_if_in_blacklist approach would have simply skipped these and the D8 standard settings would be left as they are out of the box but your patch is probably also taking care of this.
I'll update the issue summary next with the static_map appoach.
Markus
Comment #36
masipila CreditAttribution: masipila as a volunteer commentedComment #37
masipila CreditAttribution: masipila as a volunteer commented@maxocub, I changed this to unassigned since your patch is now passing the tests and this is now ready for the review of other members of the community.
Comment #38
maxocub CreditAttribution: maxocub commented@masipila: Yes, I did test it manually, but only with limited content, I'd be curious to see if it works well with a real site with a ton of articles, forums and comments.
@phenaproxima: I think that the existing tests are sufficient, they were already testing the right stuff, but with the wrong names for the article and forum's comment types.
Comment #39
phenaproximaThis patch raises a lot of questions for me, mostly because comment migration is already incredibly confusing. But this is the main thing that raises my eyebrow:
Why are we doing this for articles, but all other content types (except for forum) are untouched?
Comment #40
phenaproximaI was reviewing this and honestly, I found it incredibly confusing. However! That is 100% my fault, not @masipila's or @maxocub's, or anyone else who worked on this patch.
#2532534: Migration Files for Drupal 7 Comments, which introduced the d7_comment_type migration, is janky and was hacked together in less than a day, and having a comment type for each node is incredibly illogical and makes further work on comments very, very confusing.
So I've opened #2886839: D7 comment type migration should emulate Standard in the hope of clearing that up, and ideally I'd like to postpone this issue on that one. Once that lands, this can be rerolled much, much more simply. In fact, it might not be necessary at all. But in any event, the comment type migration stuff desperately needs to be untangled, and I'd rather fix that underlying problem before we deal with more specific issues, like this one.
Comment #41
phenaproximaAdding #2886839: D7 comment type migration should emulate Standard as a related issue.
Comment #42
phenaproximaI think I finally grok the issue, and the general fix (make an exception for the Forum comment type) makes sense to me. However, I have a few problems with the approach taken in the patch:
I think this should be using the migration_lookup process plugin to get the migrated comment type ID from the d7_comment_type migration, rather than static mapping.
We can just map this to the migrated comment_type value, since they seem to be identical:
field_name: '@comment_type'
As with d7_comment, this should use migration_lookup to get the migrated comment type ID.
Same here.
And here.
This could just reuse the migrated field_name value:
'settings/comment_type': '@field_name'
This should also use migration_lookup.
I don't understand why we're making a specific exception for articles. IMHO, we should only do the mapping for comment_forum, and everything else should be left untouched.
As per my previous comment, I'd rather we only made an exception for forum posts, but not for any other node type.
Ditto.
Ditto.
Comment #43
masipila CreditAttribution: masipila as a volunteer commentedSummary of the discussions with phenaproxima today in IRC:
Comment #44
phenaproxima+1 what @masipila said in #43. There is basically no elegant way to do this properly, so the approach in #34 is what we will do. However, we acknowledge that it's a confusing workaround, so we're going to thoroughly document it by adding comments to all of the migrations that are affected by this change.
Comment #45
phenaproximaThis addresses my feedback from #42 except for points 8-11, as per @masipila's comment in #43. It doesn't add documentation, but I'll do that once this passes all tests.
Comment #46
masipila CreditAttribution: masipila as a volunteer commentedHi,
I can do manual testing for this hopefully later this evening.
Did you have a chance to check if we need / if we want to have almost identical migrations d7_comment_field_instance and d7_field_instance? If I remember correctly by heart, they seemed to do the same thing except they had one difference which was related to translatability. I think the question is that do we need d7_comment_field_instance migration at all?
Cheers,
Markus
Comment #47
phenaproximaAdded a bit of documentation. All opinions welcome! I'd like it to be crystal-clear, since this is such a confusing and thorny issue.
Comment #48
maxocub CreditAttribution: maxocub commentedI forgot to change the doc block here to account for the new arguments.
Same here.
Comment #49
phenaproximaFixed!
Comment #50
phenaproximaComment #51
masipila CreditAttribution: masipila commentedHi,
I have the latest D8.4.x from git + patch #49 applied cleanly.
Unfortunately the test results are not OK. Results from the selected migrations below. Pay attenttion to the following:
Comment #52
masipila CreditAttribution: masipila commentedComment #53
phenaproximaThe failures most likely have to do with the use of migration_lookup (as I requested), which probably failed and caused the rows to be skipped/ignored. I'll have to fix this and expand the test coverage to catch those cases.
Comment #54
heddnPotentially resolved with #2887142: NodeType source plugin should include comment information.
Comment #55
maxocub CreditAttribution: maxocub commentedPostponed on #2887142: NodeType source plugin should include comment information.
Comment #57
maxocub CreditAttribution: maxocub commentedI don't know why I postponed this issue on #2887142: NodeType source plugin should include comment information, this seems unrelated.
We need to address #51 and add tests.
Comment #58
maxocub CreditAttribution: maxocub commentedThis needed a re-roll, so here it is.
@masipila: I don't understand why it's not working for you. I did test it with a test site and everything was migrated. Also, I find the test coverage to be adequate. Could you provide more information about the context of the failures you mention in #51? Thanks!
Comment #59
masipila CreditAttribution: masipila commentedHi, I'll try to find time to re-test my migration with a clean 8.5.x-dev install and patch #58. I'm quite busy for the rest of the week but I'll try to find time to do this either this evening (Finnish time) or tomorrow.
Cheers,
Markus
Comment #60
masipila CreditAttribution: masipila commentedI did a full manual re-test with D8.5.x-dev with path #58 applied. Patch #58 seems to be OK with only one remark. All forum threads are closed after the migration. Unortunately I don't have time to test this without the patch so I propose that somebody gives this a second pair of eyeballs to check if this is related to patch #58 or not.
Testing notes.
Manual inspection in the UI. Result: OK. We have comment, comment_forum. We don't have comment_node_article, we don't have comment_node_forum. For all other content types we have comment_node_foo.
Manual inspection in the UI. Result: OK. Article has a comment field comment, forum has comment_forum and those content types which had comments on D7, have now comment_node_foo
The two failed ones are related to my custom module on D7. So these are OK.
The error about the select plugin seems unrelated.
Migrated all users, taxonomy vocabularies and nodes. Had to manually tweak the filter formats but that is unrelated to this issue.
Then, finally
Had to manually tweak the filter formats for comment body but that is unrelated to this issue.
The comment statistics are now OK in the Forums. Yay.
Now some sleep...
Cheers,
Markus
Comment #61
masipila CreditAttribution: masipila commentedOh, forgot to comment about the problems I reported in #51. I tried to be very systematic both in #51 and #60 but I can't explain what went wrong in #51. The migrations seem to work now (except the closed forum threads is suspectious) so I can't figure out anything else then some human error on my side when testing #51.
Summa summarum:
@maxocub did test #58 manually and so did I but @maxocub is the key contributor of the patch so I don't think his test result count here. To me, everything seems to be OK except the closed forum threads. Having said this, I'm quite tired at the moment so I would highly recommend that somebody will give patch #58 another full manual test review before this will be RTBC.
Cheers,
Markus
Comment #62
maxocub CreditAttribution: maxocub commentedThanks a lot @masipila for taking time to test this again.
About the closed forum threads, there is already an issue open about this: #2887043: D7 comment field instance migration: incorrect mapping for open/closed/hidden default value.
This problem was not apparent before applying the patch from #58 because the forums were still using the comment type provided by the forum module, not the migrated comment type. I think we can ignore this problem and deal with it in the other issue.
Comment #63
masipila CreditAttribution: masipila as a volunteer commentedHmm... I don't completely get this. The scope of #2887043: D7 comment field instance migration: incorrect mapping for open/closed/hidden default value is that the default value of open/closed/hidden was migrated incorrectly for a given content / comment type.
The problem that I was wondering here was the open/closed/hidden status of an individual forum node. This should have nothing to do with #2887043: D7 comment field instance migration: incorrect mapping for open/closed/hidden default value.
However, thinking this further, it should have nothing to do with with the issue that we are trying to fix here either because the open/closed/hidden status is a property of the node and not a comment of a node. Which suggests that patch #58 did not break this but something else did. Which suggests that we should create a separate issue for that one and investigate and fix it separately.
I'll create a separate issue for that.
Cheers,
Markus
Comment #64
masipila CreditAttribution: masipila as a volunteer commentedI created #2902766: Node "comment open / closed" status not migrated.
Cheers,
Markus
Comment #65
heddnInventing a tag to mark this as pertaining to migrate_drupal and therefore blocking its stable release.
Comment #66
heddnBlocker for #2905736: Mark Migrate Drupal as stable
Comment #67
heddnUpdated IS.
Comment #68
heddnUntil now, we've assumed that the destination site in D8 was vacant. This is the first (that I'm aware of) case where we try to work around any install profile naming of fields. I hate to start doing that now. But the process plugin shouldn't know anything about the destination either, so I don't think we have much option. We should just migrate, without concern of what the D8 site already has built in any install profile.
Can we migrate to the semi-standard migration location for nodes (barring forum, which adds its field in the module install, not an install profile)? We do in all other cases. If someone is migrating from d6 and has an article node type there that has the body field removed and a field_description added, we'd result in a duplicate "body-like" field on articles. This isn't any different.
What data integrity issues do we create if we have two comment fields on the article content type? Can someone speak to that? Beside the oddity of it, do we actually break/loose data?
Let's just use the filename instead of the full path.
Can we move this to a trait? Seems generally useful enough. And the default for the comment name should then be comment_node_ . $node_type, unless $comment_type is non-null.
Comment #69
masipila CreditAttribution: masipila as a volunteer commented@heddn, regarding this:
If I remember correctly by heart from the previous investigations I had with comment_entity_statistics, we had duplicate entries there when Article had the two comment fields / types present. This lead to duplicate results in views that were using data originated from comment_entity_statistics.
I believe (without testing this) that these duplicate views result issue could be resolved by deleting the comment commenti field / type from the Article (assuming that Migrate populated the statistics to comment_node_article, which I don't remember by heart).
Other than that, I don't see any other breaks / data losses.
I believe we have a clear consensus that Forum must have comment_forum because that has been hard coded to Forum module?
Markus
Comment #70
heddnYes, no problem with forum. We've heard from the forum/comment maintainer it won't be changing. The issue we need to decide about is what to do with article, which comes from standard install profile. I'd like to propose we assume the destination is empty and if it isn't then user will have to delete the comment field from article to fix things.
Comment #71
heddnReviewing in weekly migrate maintainers meeting. This should be marked as a major at least. Still looking to gain some insights around #68.1.
We discussed in the meeting and we would like to not have one-off logic for articles since it comes from an install profile, not the node module. As long as deleting the duplicate comment field fixes the statistics bug that started this issue.
Comment #72
masipila CreditAttribution: masipila as a volunteer commented@heddn, your proposed approach in #68-71 affects #2902766: Node "comment open / closed" status not migrated. Please see my comment https://www.drupal.org/node/2902766#comment-12243133
Short version: if we choose the approach I proposed in my comment linked above, I don't see unrecoverable data losses / breaks with your approach #68-71 here. We need to clearly document that site builders using D8 need to manually delete the extra comment field, though.
Cheers,
Markus
Comment #73
heddnre #72: we can mention the issue with comment fields in https://www.drupal.org/docs/8/upgrade/known-issues-when-upgrading-from-d....
Comment #74
heddnFrom #68
fixed 1 and 2 (I think, let's run some tests). There's a lot of places to change so I might have missed a few.
While tests run, working on cleanup from point 3.
Comment #75
heddnComment #77
heddnRe #74 failure. Counts increased because we don't re-use the article node's comment type. Fixed here.
Comment #79
heddnFixes code style issues caught by CI.
Comment #82
dillix CreditAttribution: dillix commented@heddn, is there a chance that we see a fix for this issue at 8.4?
Comment #83
heddnSince migrate is still experimental, changes will almost always get backported to 8.4. In the case of this, it is a bug and should make its way into 8.4.
And now for those counts. It came back green locally. Let's see if it does on testbot too.
Comment #84
masipila CreditAttribution: masipila as a volunteer commentedI was testing patch #83 but run into the same weird behavior that I encountered in #51.
For some reason the comment fields and comment field instances are ignored. I'll start to debug this and will post my findings here.
Markus
Comment #85
masipila CreditAttribution: masipila as a volunteer commentedComment #86
masipila CreditAttribution: masipila as a volunteer commentedThe weird behavior here seems to be that migration_lookup process plugin is fragile.
About my test site:
1. Latest D8.5.x installed with git
2. Patch 83 from this issue applied. Should not make a difference since this patch is not changing the migration_lookup process plugin.
Migration steps performed:
Observation: We can see that all 5 comment fields were ignored when upgrade_d7_comment_field was run.
Looking at the migration process, we have this:
Observation: The skip_on_empty will cause the row to be skipped if migration_lookup does not return a value.
Debug step: I added a debug message to SkipOnEmpty process plugin as follows:
Executed the upgrade_d7_comment_field migration again with Drush:
Indeed, the migration_lookup is passing an empty value to the skip_on_empty plugin in the migration pipeline.
Looking at the comment types in the UI at admin/structure/comment: I can see that the 5 comment types are present. So upgrade_d7_comment_type migration did it's job just like the migration output said earlier. I'm referring to this:
Let's look at the migration map in the database next, table migrate_map_upgrade_d7_comment_type
Observation: the mapping is present in the mapping table:
This starts to suggest that we have some weird stuff going on with the migration_lookup process plugin. Let's debug that next.
Let's add a debug info to the end of MigrationLookup::transform().
Let's run the migration again:
Conclusion: we have some underlying bug in migration_lookup process plugin which is interfering here.
Proposed next steps: I'll open a follow-up issue for migration lookup being fragile.
Cheers,
Markus
Comment #87
masipila CreditAttribution: masipila as a volunteer commentedThe issue with #84-86 was that Migrate Upgrade 8.x-3.0-rc1 does not work properly with migrate_lookup process plugin. This has been fixed as #2875405: Rewrite references in 'migration_lookup' plugins as well as 'migration' but that is only in Migrate Upgrade dev.
Comment #88
masipila CreditAttribution: masipila as a volunteer commentedNgggh...
Either I screwed up my test environment when messing up with the #84-87 or the patch #83 needs to be re-rolled.
Comment #89
heddnNew release provided for migrate_upgrade. And the patch still applies cleanly against 8.4.x & 8.5.x.
Comment #90
masipila CreditAttribution: masipila as a volunteer commentedI set up my test environment from scratch and was now able to test patch #83. We're almost there, but...
1. Handling for Forum is correct now
2. Unfortunately the suggested approach from #71 for Article is not ok.
The proposed approach is this:
I did this, but unfortunately the duplicate entries in comment_entity_statistics are not removed when comment comment field / type is deleted. See the screenshot below.
I see four possible approaches to this:
a) We return back to the drawing board and use the special handling for Article comments.
b) We instruct the site builders to remove the D8 Standard comment field *before* they start the migrations. I don't think that too many site builders will notice this instruction, though.
c) If s#it is already in the fan when the site builder notices the duplicate entries in comment_entity_statistics, then I guess it *most probably* is safe to delete the entries from comment_entity_statistics where field_name = comment. This is too dangerous instruction to write IMO because the site builder might have re-used the standard comment comment type by the time he/she realizes the issue.
d) D8 Core would be changed so that when comment field is removed from a content type, also associated comment_entity_statistics would be deleted.
3. Disclaimer: I did not test / pay attention to migrations d7_comment_entity_display, d7_comment_entity_form_display, d7_comment_entity_form_display_subject, d7_view_modes
Cheers,
Markus
Edit: added 2d, 3.
Comment #91
dillix CreditAttribution: dillix commented@masipila can we make patch that will do 2d automatically?
Comment #92
masipila CreditAttribution: masipila as a volunteer commented@dillix, I think that 2d is the way to go but I'm tempted to say that the behavior I tested and described in my previous comment is a bug in Core Comment module that should be fixed regardless of this migration issue.
Consider the following:
1. Content type has field foo. Nodes exist and we have content in this field.
2. Field foo is deleted from the content type.
I didn't test yet but I'm quite sure that all content of field foo from this instance will be deleted i.e. we don't leave orphan entries to the database. If this is correct as general Core behavior, then it should be the responsibility of the Comment module to clean up its data from the database, including related entries from comment_entity_statistics.
I'll test this later today but any thoughts to this anyone?
Markus
Comment #93
dillix CreditAttribution: dillix commentedI think you are correct. But related issue #2874487: Hardcoded field names in Core should be assigned/returned dynamically addressed to 8.5, may be we should change that to 8.4?
Comment #94
masipila CreditAttribution: masipila as a volunteer commentedI don't see how #2874487: Hardcoded field names in Core should be assigned/returned dynamically would be needed in order to get this issue resolved. According to heddn, the Forum module maintainers had informed that Forum is not goint to get rid of hard coded comment_forum. Patch #83 already handles comment_forum properly. The only remaining headache from migration poijt of view is the Article and D8 Standard comment comment type. If we can get the 2d approach I suggested, then we're done.
Markus
Comment #95
masipila CreditAttribution: masipila as a volunteer commentedI created #2906470: Orphan comments and entries in comment_entity_statistics after comment field instance has been deleted.
@heddn, could you please discuss that issue with Comment module maintainers?
Cheers,
Markus
Comment #96
heddnI don't see #2906470: Orphan comments and entries in comment_entity_statistics after comment field instance has been deleted being a hard-blocker. Maybe a soft one. Let's add to https://www.drupal.org/docs/8/upgrade/known-issues-when-upgrading-from-d... what we know about comments (right now) and link the issue that is planning to fix things.
Putting back to NR.
Comment #97
masipila CreditAttribution: masipila as a volunteer commentedI agree that #2906470: Orphan comments and entries in comment_entity_statistics after comment field instance has been deleted is only a soft blocker. Especially now that it became crystal clear there that the current core behavior upon comment field instance deletion is a bug and not a feature.
Then about this issue and remaining tasks. As I mentioned in #90, I did not test all the migrations that this patch changed as I don't even understand all the things that those migrations are supposed to migrate.
What my manual test covered was d7_node_type, d7_comment_type, d7_comment_field, d7_comment_field_instance, d7_field, d7_field_instance, d7_node_{node_type}, d7_comment.
The automated test coverage for the remaining affected migrations seems to be good but it would not hurt if somebody could give still another pair of eyeballs for them.
Cheers,
Markus
Markus
Comment #98
sbogner@insightcp.com CreditAttribution: sbogner@insightcp.com commentedI'm upgrading a D6 forum to D8 (8.4 specifically) - are there plans to make this correction apply also to D6 migrations?
Comment #99
heddnThis is the first report of an issue on D6. Can you elaborate if does effect d6 as well? The solution would need to be duplicated for d6.
Comment #100
sbogner@insightcp.com CreditAttribution: sbogner@insightcp.com commentedYes, I'm seeing the exact same symptoms in my D6 site when I do the forum migration.
Comment #101
heddnBack to NW to pick up D6.
Comment #102
andypostIMO mapping is a right way for standard profile provided types, I think forum case should also be covered with mapping
I like that idea!
Comment #103
maxocub CreditAttribution: maxocub commentedThe problem when upgrading from D6 is that, right now, it's only creating two comment types, however how many node types there is: comment & comment_no_subject.
Before we can implement the solution from this issue for D6, we need to fix the D6 migration of comment types, and that's what we're doing in #2887142: NodeType source plugin should include comment information.
I also like that idea, but I think that @heddn and @masipila agreed that we shouldn't add mapping for the standard profile.
Comment #104
masipila CreditAttribution: masipila as a volunteer commentedRe #102-103:
The original approach of this issue was that the comment type for Article nodes would be the D8 Standard comment type called comment. I.e. this:
However, heddn and other maintainers of the migrate concluded that we don't want to do installation profile specific migrations.
So we have two possible approaches for the migration of Article comments:
There is at least one other comment migration related issue that are depending on the decision here. I'm referring to #2902766: Node "comment open / closed" status not migrated. Currently that issue assumes that we use comment_node_article. If we choose to use comment, then that new approach needs to be reflected in #2902766.
I personally don't have a strong opinion to either directions. I do understand the point from Migrate maintainers (see #71) that we would like to avoid installation profile specific approaches in migrations.
Proposed next steps: I would propose that @heddn and @andypost will agree on the approach so that we have a clear guidance on the approach for this spiderweb.
Markus
Comment #105
Morbus IffThrowing in another "has problems on D6" - tried a migration this morning.
I'd personally prefer "comment" myself, too. If I'm having to start a D8 site "from scratch" to perform a migration, than I want the migration to migrate me as "D8-y default" as possible. I don't want to start in D8 with weirdness already ("comment_node_article" vs. "comment"), and no real ability to unweird it. I don't want half my "article" content using one Comment type (via the migration) and then any other content type I create using the default "comment" (I also wouldn't want to set new content types to use "comment_node_article", to standardize on one content type, cos it's clearly namespaced to article, even if that wasn't my choice),
Comment #106
heddnArticle and Forum are the weird ones here. If you add a comment manually to page or blog, or migrate content over from a page or blog, then they are going to use comment_node_page and comment_node_blog.
Comment #107
Morbus IffGotcha. In which case, due to prior art, I support comment-node-article. Blame ignorance.
Comment #108
heddnRather than slow down this patch, let's move the D6 stuff into a follow: #2909752: [D6] Migration for comments: duplicate comment types and incorrect comment_entity_statistics. That means this needs review.
Comment #109
andypostI think it depends on #2887142: NodeType source plugin should include comment information
About naming - maybe file new issue and discus about how machine names should migrate?
It's like naming for content type - d6 "story" migrate to d7/8 "article"
Comment #110
heddnI've opened #2910600: [policy, no patch] How machine names should be migrated.
Comment #111
larowlanOne code comment:
As an Australian I endorse the shortening of Combination to Combo, but I think our standards in core would dictate that we should use the full word. So I think this needs to be Combination instead of Combo
And one question:
I don't understand what this patch does to resolve comment_node_article - it may be that it does nothing and we're going to advise people to delete the comment field from article if they migrated - is that the case? If so, then I understand, if not, then I'm not qualified to review this (migrate isn't my speciality).
I note the issue summary still says
But I don't see where in the patch we're mapping to
comment
- so perhaps the IS needs an update? Tagging for that.Comment #112
masipila CreditAttribution: masipila as a volunteer commentedThanks @larowlan!
Issue summary updated.
Cheers,
Markus
Comment #113
masipila CreditAttribution: masipila as a volunteer commentedComment #114
maxocub CreditAttribution: maxocub as a volunteer and commentedThis also needed a re-roll, so here it is with the Combo -> Combination change.
Comment #115
heddnPreferably #2887142: NodeType source plugin should include comment information lands before this one. If we can, let's get that one committed first.
Comment #116
phenaproximaAssigning for review.
Comment #117
xjmGiven the importance of Migrate to the Drupal 8 ecosystem at this point, @catch and I agreed to promote the "Migrate critical" issues to critical priority.
Comment #118
quietone CreditAttribution: quietone as a volunteer commentedI have through the issue and looks like the task is done and everyone agrees with the solution in the IS. What I would like to see is this documented on the Known issues when upgrading from Drupal 6 or 7 to Drupal 8 before this is committed.
What I have not done is review the code, that will have to be later.
Setting to NW for the doc page, just to ensure we document this.
Comment #119
phenaproximaUn-assigning from myself, since @quietone kicked it back.
Comment #120
masipila CreditAttribution: masipila as a volunteer commentedI'll do the documentation update.
Comment #121
masipila CreditAttribution: masipila as a volunteer commentedKnown issues docs updated:
https://www.drupal.org/docs/8/upgrade/known-issues-when-upgrading-from-d...
According to #108, the D6-D8 part will be handled as a separate follow-up issue. I'm not 100% familiar with the D6-D8 status so I didn't update the Known Issues for D6-D8 part. But that is anyway out of scope of this issue.
Back to Needs Review.
Comment #122
maxocub CreditAttribution: maxocub for Acquia commentedAdding a tag so we can see this issue on our Vienna migrate sprint kanban.
Comment #123
quietone CreditAttribution: quietone as a volunteer commented@masipila, thanks. That documentation is very thorough and clear! I made one small change, due to an extra 'do' in a sentence. On reading the whole thing, the only concern I have is that is isn't clear if the comment field on Article should be removed for all migrations with Article content type of for only migrations using Forum that have Article content type. Does it matter? Either way, I think this should be made clear what situations the recommendation is addressing.
Comment #124
masipila CreditAttribution: masipila as a volunteer commentedI don't completely understand this question so you'll get a bit longer answer.
1. For Forum, no special actions are needed from the site builder after this issue has been committed. The only thing that need to be understood is that we are *not* creating comment_node_forum like we are doing for all other content types. Reason being that Forum uses a hard coded comment type so we are migrating the comments to that comment type.
2. The Article topic has nothing to do with Forum.
The migrations are creating comment type comment_node_article and migrating all Article comments to this comment field. This works exactly like for all other content types (except Forum which has the special handling described above).
The thing here is that if D8 site was installed using Standard installation profile, then Article also has another comment field called comment which was there out of the box. If the site builder does not want to have two comment fields for Article content type, then the site builder needs to remove the not-wanted comment field comment. See the third screenshot in the issue summary.
Hope this helps,
Markus
Comment #125
masipila CreditAttribution: masipila as a volunteer commentedBack to needs review.
Comment #126
quietone CreditAttribution: quietone as a volunteer commented@masipila, thank you very much. I have read your comment and re-read the known issue page. It is all clear to me now and I don't think any changes are needed to known issues. I was just a bit muddled.
Comment #127
quietone CreditAttribution: quietone as a volunteer commentedI have applied the patch, read the code and can't find any problems not even a nit. I did have a problem understanding the entity counts changes, but I was reading it as an increase instead of a decrease in the entity counts. maxocub kindly showed me the error of my ways. I still blame it all on the northern hemisphere air!
masipila tested the patch in #90 and reported that the 'Handling for Forum is correct now' and 'Unfortunately the suggested approach from #71 for Article is not ok.' Following discussion resolved how to handle the Article node type.
After that test, the code changes was simply a name change on a method from 'CreateNodeCommentCombo' to 'CreateNodeCommentCombination'. So, I think the manual testing done in #90 still applies.
The Known issues handbook page has been updated.
masipila, does suggest in #90 that another pair of eyes on the other migrations changed in this patch is a good idea. Those migrations are
The final question, as far as I am concerned, is are the existing test sufficient for the changes to these migrations.
And I need to take a break now.
Comment #128
maxocub CreditAttribution: maxocub for Acquia commentedd7_comment_entity_display.yml
The change in this file is to get the field name from the comment type migration. Beside the forum comment field name, the other field names did not change, as shown by this test:
d7_comment_entity_form_display_subject.yml
Same as above, but with the comment bundle. And only the forum bundle is changed in the test:
And the same is true for the 3 other migrations:
Comment #129
quietone CreditAttribution: quietone as a volunteer commented@maxocub, Absolutely! Thanks for answering while I took a break. Yes, it is obvious that those other migrations are better now that are using migration_lookup.
Woohoo!
Comment #130
maxocub CreditAttribution: maxocub for Acquia commented@quietone: Thank you! They are better in the way that we don't need to add this mapping in all of them:
We do the mapping only in d7_comment_type and get the same value everywhere with the migration lookup.
Comment #131
webchick@maxocub and @phenaproxima stepped me through this one at DrupalCon Vienna. Looks like @larowlan's feedback from #111 has been addressed. The issue with Articles having two comment types is already existing, regardless of this patch. There's some talk about this in #2906470: Orphan comments and entries in comment_entity_statistics after comment field instance has been deleted, but I think we need a dedicated follow-up critical to this (since it sounds like it's a bit contentious among members of the Migrate team), because nearly everyone installs with Standard (I can find some DA stats to back this up if need be).
My only concern was whether existing D7 migrations would need to make changes here, and it sounds like existing migrations should work fine. Forum module is doing something truly wacky, so we're trying to support it properly here.
Committed and pushed to 8.5.x and back-ported to 8.4.x. Thanks!
Comment #134
masipila CreditAttribution: masipila as a volunteer commented@webchick, as @maxocub and @phenaproxima most probably explained (and which is clearly documented on the migrate known issues page), the current approach where this issue concluded was that the comment comment field needs to be manually deleted from the Article content type because we are migrating comments to comment_node_article comment field.
The issue #2906470: Orphan comments and entries in comment_entity_statistics after comment field instance has been deleted that you were referring to has nothing to do with migration per se. It's just that this assumption that the redundant comment field is deleted does not currently work because the deletion of the comment field leaves orphan rows in the database at the moment. Based on your comment above, I raised that issue from major to critical.
----
If you feel that we should revisit the decision that D7-D8 migration path (and D6-D8 if D6 had a content type called Article by default, I don't remember by heart) relies on the manual deletion of comment field of Article, then that should indeed be handled as a separate follow-up issue.
Cheers,
Markus
Comment #135
maxocub CreditAttribution: maxocub as a volunteer commentedHere is the issue for the discussion about mapping machine names to things provided by the standard installation profile: #2910600: [policy, no patch] How machine names should be migrated.