Updated: Comment #14

Problem/Motivation

  1. comment_field_instance_create() contains logic to ensure that the default value of comment fields at least contains entries for each of the sub-values of a comment field, many of which are calculated (ie not present in comment_field_schema()). The logic incorrectly sets the value without respecting any existing default values the calling code may have set.
  2. Also it creates a body field on entity create, but should only do so on insert - which causes sync issues.
  3. Finally, there is some code referencing #731724: Convert comment settings into a field to make them work with CMI and non-node entities in field_purge_batch() that can be removed.

Proposed resolution

  1. Don't blindly set the default value, instead check for its presence first and only append the missing values..
  2. Don't create the body field in hook_field_instance_create, use hook_field_instance_insert instead.
  3. Remove chunk from field_purge_batch().

Remaining tasks

Reviews

User interface changes

None

API changes

None

Original report by @chx

CommentFileSizeAuthor
#14 comment-default-2172123.14.patch3.1 KBlarowlan
#14 interdiff.txt1.7 KBlarowlan
#10 2172123-10.patch1.99 KBandypost
#10 interdiff.txt632 bytesandypost
#2 comment-default-2172123.pass_.patch1.99 KBlarowlan
#2 comment-default-2172123.fail_.patch1.06 KBlarowlan
comment_instance_default_clobbered.patch952 byteschx

Comments

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented
Title: Comment settings can not be migrated » comment_field_instance_create() overwrites default values
Issue summary: View changes
larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented
Status: Active » Needs review
StatusFileSize
new comment-default-2172123.fail_.patch1.06 KB
new comment-default-2172123.pass_.patch1.99 KB

should be red/green

The last submitted patch, 2: comment-default-2172123.fail_.patch, failed testing.

The last submitted patch, 2: comment-default-2172123.fail_.patch, failed testing.

chx’s picture

chx commented
Title: comment_field_instance_create() overwrites default values » comment_field_instance_create() overwrites default values and breaks the migration

Status: Needs review » Needs work

The last submitted patch, 2: comment-default-2172123.pass_.patch, failed testing.

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented
Status: Needs work » Needs review

2: comment-default-2172123.pass_.patch queued for re-testing.

benjy’s picture

benjy commented
Status: Needs review » Reviewed & tested by the community

Looks good.

andypost’s picture

andypost
he/him
Primary language Russian
commented 
+++ b/core/modules/comment/comment.module
@@ -232,14 +232,18 @@ function comment_field_instance_create(FieldInstanceInterface $instance) {
+    $instance->default_value += array(array());
+    $instance->default_value[0] += array(

comment field cardinality is 1. so first line is useless

andypost’s picture

andypost
he/him
Primary language Russian
commented
StatusFileSize
new interdiff.txt632 bytes
new 2172123-10.patch1.99 KB

the fix, still rtbc

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 10: 2172123-10.patch, failed testing.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Priority: Major » Critical
Issue tags: +beta blocker
Related issues: +#1808248: Add a separate module install/uninstall step to the config import process

comment_field_instance_create() also causes issues for syncing configuration and properly removing comment fields (currently excluded from field_purge_batch())

This blocks #1808248: Add a separate module install/uninstall step to the config import process

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented
Title: comment_field_instance_create() overwrites default values and breaks the migration » Fix various issues with comment_field_instance_create()
Issue summary: View changes

Updated issue summary/title
Working on this

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented
Assigned: larowlan » Unassigned
Issue summary: View changes
Status: Needs work » Needs review
StatusFileSize
new interdiff.txt1.7 KB
new comment-default-2172123.14.patch3.1 KB

Have at it bot.

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented
Issue summary: View changes
larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented
Issue summary: View changes
andypost’s picture

andypost
he/him
Primary language Russian
commented

Not sure we still need hook create implementation because it was a workaround for fields created via Field UI that does not set default values. Suppose now Entity field api should set this defaults

chx’s picture

chx commented

So what's up with this critical now?

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented

#17 default stuff is #2175017: FieldDefinition::create() doesn't populate default 'settings' for the field type

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
commented
Status: Needs review » Reviewed & tested by the community

This was RTBC before, and @larowlan linked to the defaults issue.
The code looks good to me!

berdir’s picture

berdir
Primary language German
Location Switzerland
commented

The referenced issue is only about FieldDefinition, meaning base fields *and* it's about settings, this is about the default value. Not related.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented
Assigned: Unassigned » alexpott

This looks straight-forward to me, but shooting it over to alexpott since he can better evaluate whether the fixes here unblock that other issue.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Assigned: alexpott » Unassigned
Status: Reviewed & tested by the community » Fixed

Committed 854c866 and pushed to 8.x. Thanks!

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.