Ran into a very interesting edge case when using Paragraphs with comments in Open Atrium on the oa_worktracker Task which has comment_alter enabled.

Basically in Atrium we have added a "related content" paragraphs field to both the task node and to the comment. We have comment_alter turned OFF for these fields, but comment_alter is enabled for the normal Status, Priority, etc fields on the Task.

When a new comment is posted, the value of its "related content" field is saved with the parent node even though comment_alter is turned off for that field.

I debugged this further and found that the call to field_attach_form_validate() within _comment_alter_validate_node_fields() caused the current $form_state['values'] of ALL FIELDS to be updated in the entity, essentially ignoring the 'field_name' option. It seems that 'field_name' is respected for only performing the actual validation check on the indicated fields, but it still has the side-effect of setting ALL the fields in the passed entity.

Since the $form_state is for the COMMENT, if the comment and parent node have matching field names (like the related-content field), this causes the comment's field to overwrite the parent node's field.

Patch coming to fix this.

CommentFileSizeAuthor
#1 comment_alter_overwrites_fields-2540782-1.patch679 bytesmpotter
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

mpotter’s picture

mpotter CreditAttribution: mpotter commented
Status: Active » Needs review
FileSize
comment_alter_overwrites_fields-2540782-1.patch679 bytes

Here is the patch. In this case, since we just want to run validation hooks, we just need to pass a copy of the node entity so it doesn't actually change the real node what is saved later when the comment is posted.

It might be nice if Drupal Core actually respected the 'field_name' in $options when calling _field_invoke_default('extract_form_values' ...), so if somebody wants to propose a core patch to deal with this, please link it here. But for now let's just fix this issue in comment_alter which can cause data-loss on the original node.

dsnopek’s picture

dsnopek
he/him
Primary language English
Location USA
CreditAttribution: dsnopek commented
Status: Needs review » Reviewed & tested by the community

Ugh, yeah, looking at the code in field_attach_form_validate() it's calling:

_field_invoke_default('extract_form_values', $entity_type, $entity, $form, $form_state);

... without passing the $options as the 6th argument. :-/

The patch looks good to me! RTBC'ing. :-)

FranCarstens’s picture

FranCarstens CreditAttribution: FranCarstens commented

This patch was not included in the latest dev (7.x-1.0-rc4+28-dev, 2016-Mar-03) but applied without problem and fixed the issue.

Can we get it committed?

  • Boobaa committed b5b80eb on 7.x-1.x authored by mpotter 
    Issue #2540782 by mpotter: Overwrites fields with same name on original...
Boobaa’s picture

Boobaa
he/him
CreditAttribution: Boobaa at Pronovix commented
Status: Reviewed & tested by the community » Fixed

Looks like I overlooked this one, sorry.

Status: Fixed » Closed (fixed)

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