When a node is saved after editing both a CNR field and a non-CNR field, the CNR field values are appropriately saved, but the values of the fields that do not involve CNR are not saved. This happens on a clean install of Drupal 7.9, References, and CNR.

This may relate to this CNR 6.x issue: http://drupal.org/node/1292080

To replicate:

  1. Clean installation of Drupal 7.9 with References, and CNR 7.x-4.1 installed/enabled.
  2. Create content types "Content Type A" and "Content Type B".
  3. Create Content Type A fields: "B References" (noderef autocomplete) and "CTA Textfield" (text field).
  4. Create Content Type B fields: "A References" (noderef autocomplete) and "CTB Textfield" (text field).
  5. Bind Reference fields with CNR at admin/config/system/corresponding_node_references
  6. Create "Node A" of Content Type A.
  7. Create "Node B" of Content Type B.
  8. Edit "Node A" and assign the "B References" field with the value "Node B".
  9. View "Node B" to ensure the "A References" field was automatically given the value "Node A".
  10. Edit "Node A" by emptying the "B References" field. Also give "CTA Textfield" any value. Save the node.
  11. See that "Node A" now has no value for "B References" (good), but the value given to "CTA Textfield" was not saved (not good).
  12. Again, edit "Node A" and re-add "Node B" under "B References". Again, give "CTA Textfield" a value. Save the node.
  13. See that the reference was once again properly saved, but again the textfield was not.
  14. Edit "Node A" and give "CTA Textfield" a value. Save the node. See that it properly saves when CNR fields were not altered.

Any help with this would be great.

CommentFileSizeAuthor
#4 cnr_fields_not_saved_1334796_2.patch492 bytesczigor
#1 cck_fields_not_saved-1334796-1.patch1.81 KBguschilds
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

guschilds’s picture

guschilds CreditAttribution: guschilds commented
Status: Active » Needs review
FileSize
cck_fields_not_saved-1334796-1.patch1.81 KB

I believe I have discovered the source of the problem and created a patch for it.

When Node A was updated with new information in CNR fields (such as a reference to Node B), the module (surprisingly) seems to work like this:

  1. Node B is updated with a reference to Node A.
  2. This update triggered the same effect on Node A, so existing Node A (without CCK updates) was updated with a reference to Node B.
  3. The non-CNR content (CCK fields, etc.) that were also changed on Node A were never saved, and were lost.

The attached patch passes a reference to Node A to the function that was already being passed (and saving) Node B. This function now saves Node A before saving Node B (in step 1 of the previous process). This allows Node A to retain other changes, such as non-CNR CCK fields. Node A is only saved in this way when updating Node A, not when Node A is completely new or when Node A is being deleted. In those cases, the reference to Node A is not passed. This is because the problem was not happening then.

I believe CNR checks to ensure that references do not already exist before re-saving a node, so Node A should only be saving once (rather than being saved again in Step 2 outlined above after already saving in Step 1 with my patch).

I welcome any suggestions for improvement, but this problem surely needs to be addressed, solved, and eventually committed.

czigor’s picture

czigor CreditAttribution: czigor commented

Have you tried the dev version yet? There are many issues solved there.

guschilds’s picture

guschilds CreditAttribution: guschilds commented
Version: 7.x-4.1 » 7.x-4.x-dev

Yes, I was already working with the dev version because I was experiencing another issue with 7.x-4.1: http://drupal.org/node/1148948

My apologies, perhaps this was incorrectly marked on my part.

czigor’s picture

czigor CreditAttribution: czigor commented
FileSize
cnr_fields_not_saved_1334796_2.patch492 bytes

Thanks guschilds for the report and the patch! Great work!
Your description was very clear and I can confirm this behavior.

The patch in #1 would work I think (I have not tested), but there is a more straightforward way to do this. With the patch #1 node_save(A) is called from inside node_save(B) which in turn is also called from node_save(A). Brrr. Calling node_save from inside another node_save should be avoided whenever possible. I know that's what the module had done so far, but recently I have run into another solution posted here:

http://blog.urbaninsight.com/2011/10/24/saving-nodes-fields-without-savi...

I have attached a patch using this method, please test it!

guschilds’s picture

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

You are welcome! Glad my unideal solution brought attention to the problem and assisted in creating something more appropriate.

I have tested your patch in the original scenario(s) outlined above. It succeeds where the previous code failed.

  • When creating a new node with CNR-field and non-CNR-field content, CNR still functions as desired.
  • When deleting a node with an existing CNR-field reference, CNR still functions as desired.
  • When updating a node with both CNR-field and non-CNR-field content, all fields are properly saved (fails without patch).

The link you provided mentions calling field_attach_presave before field_attach_update, which gives other modules the opportunity to act if appropriate. Might this be worth adding? Details on that function are here: http://api.drupal.org/api/drupal/modules--field--field.attach.inc/functi...

Thanks for the more optimal and timely solution!

Gus

adamdicarlo’s picture

adamdicarlo CreditAttribution: adamdicarlo commented
Priority: Normal » Critical

This seems like a critical bug to me.

I've tried the patch at #4 (applied to 7.x-4.x head) and it fixes the problem for me, too!

czigor’s picture

czigor CreditAttribution: czigor commented
Status: Reviewed & tested by the community » Fixed

Committed.

Status: Fixed » Closed (fixed)

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

domidc’s picture

domidc CreditAttribution: domidc commented

The problem with this is that hooks are not triggered anymore. It is absolutely necessary that hook are triggered because when a ref is updated on an away node it needs to be notified. For example that node should be marked for reindexing for search. Updating the fields only is not the correct sollution.

The home node should be saved anyway. It just passes thru update/insert hooks to save references to the away nodes. The real reason for these unsaved fields must be found because changing node_save to field_attach_update is not correct since it doesnt trigger hooks.

guschilds’s picture

guschilds CreditAttribution: guschilds commented

domidc,

Are you then suggesting something similar to my patch in #1 rather than the currently committed patch from #4? That implementation saved the home_node.

Thanks,
Gus