Node "comment open / closed" status not migrated

First quick glance to figure out the root cause.

We are talking about this D8 node field in UI:

When this setting is open for an individual node, the forum node has this when the node is checked with Devel.

  [comment_forum] => Array
                (
                    [x-default] => Array
                        (
                            [0] => Array
                                (
                                    [status] => 2
                                    [cid] => 0
                                    [last_comment_timestamp] => 1470864261
                                    [last_comment_name] => 
                                    [last_comment_uid] => 174
                                    [comment_count] => 0
                                )

                        )

                )

When the setting is closed, we the status is 1.

I looked at the migration upgrade_d7_node_forum. This migration process does not have any reference to the comment type field. As a result, the default value of the comment type is used for every node.

In plain English:

• In Drupal 7 we have the option to define for each node separately if the comments are open or closed
• Forum moderator can for example close "lock" a topic
• The node migrations are not migrating this setting at all and as a result D8 falls back to the default value of the comment type.
• I would consider this as Major. Module maintainers to evaluate and set the priority accordingly.

Minor updates to issue summary.

Reading the API, it looks to me that this mapping needs to be added to D7NodeDeriver::getDerivativeDefinitions.

Assinging to myself. I'll try to write the first draft of the patch in the next couple of days.

Cheers,
Markus

Patch attached.

Apparently the patch didn't break any existing tests but I guess we should add tests to cover the additional mapping. I'll leave that for someone who is more experienced with automated tests.

Issue summary / remaining tasks updated accoringly.

Cheers,
Markus

Good morning,

The pattern of using pattern of using comment_node_{node type} is indeed coming from other migrations. Phenaproxima was trying to get rid of this back in June. See #2886839: D7 comment type migration should emulate Standard.

Summary:
1. D8 Standard installation profile uses comment comment type for Article.

2. D8 Forum must have its own comment type comment_forum. This is hard coded in Forum module just like it was mentioned also above in #10.

3. Phenaproxima was trying to get rid of the comment_node_{node_type} pattern in #2886839: D7 comment type migration should emulate Standard but that fell short. Conclusion was that we need to have separate comment types for each node types because the D7 comments might have additional fields in addition to the comment subject, body and author. These additional fields may be different for each node type in D7.

The patch in #7 assumes that 1-3 are valid. Do we have alternative approaches for this specific issue?

Cheers,
Markus

Hmm...

One thing we could consider would be as follows:

In D7NodeDeriver, we could check if the D8 node type has a comment field. If yes, then we add the mapping foo/0/status: comment where foo is the comment field we detected. If we find no comment fields, we don't add this mapping.

Worth thinking: it is possible that D8 site has multiple comment fields. Should we apply the mapping to all of them or just the first? If first, do we mean first what happens to come first from the D8 config check? Or first based on the weight in form settings? Or first in Display settings? If first in display settings, which view mode? Note that I'm not suggesting this heavy determination logic, I'm just making a point that if we can't assume comment_node_{node_type}, we need to think this through.

Markus

@heddn,

You proposed in https://www.drupal.org/node/2853872#comment-12242655 that we should let the other migrations create comment type comment_node_article . Now if the D8 site was installed with Standard installation profile, the Article has two comment types / comment fields.

Hence: the concern of multiple comment fields per content type I raised in #12 above is valid if we let Article to have two comment fields.

Proposed solution for this issue about node comment statuses:
1. In D7NodeDeriver we check from the D8 configuration if the content type has any comment fields. The result of this check can be 0-N.
2. If we find any comment fields in D8, we map foo/0/status: comment for each D8 comment field.

This should make your proposed approach in https://www.drupal.org/node/2853872#comment-12242655 feasible.

Issue summary updated with the proposed approach.

Cheers,
Markus

To verify that I have not misunderstood anything:

1. Regarding Articles
We have now concluded both in this issue and in #2853872: Migration for forum and article comments: duplicate comment types and incorrect comment_entity_statistics that we should not have special handling for Articles because of D8 Standard installation profile. If the site builder is using D8 Standard, he/she needs to manually remove the comment field comment from Article node type. This will be added to the known issues documenation heddn linked above.

2. Regarding the comment fields comment_node_{node_type}
These comment types / fields are created by the other D7-D8 migrations. I'm not quite sure if comments 9-10 by heddn and larowlan were arguing against mapping to these comment fields. That is what we need to achieve if we want to preserve the node level comment statuses.

3. About the new patch

• This patch uses the same approach as patch #7 except it removes the special handling for Articles.
• It also adds the optional dependency to comment field instance migration

Setting this to Needs Review to get feedback on the approach. This is still not complete as we need to add tests so once you provide feedback on the general approach, please set back to Needs work for at least tests.

Issue summary is also updated.

Thanks for adding the test Jo Fitzgerald and maxocub!

I updated the issue summary to help reviewers. Remaining tasks at the moment are:
- Review by maintainers
- Commit
- Dance all night

Cheers,
Markus