While using field_attach_update() to update certain fields in a node object I noticed that file fields are always cleared if not included in the passed entity object. This doesn't seem to be a problem for any other field type.

To reproduce:

  1. Install Drupal with the standard modules.
  2. Add the existing field_image field to the "Basic Page" content type.
  3. Add a standard text field called field_text
  4. Add another standard text field called field_text2
  5. Create a basic page, uploading an image to field_image and placing text in all the other fields

Try to update the text field only with field_attach_update(). In the code below I'm assuming the content created above is nid #1.

  $node = node_load(1);
  $update_node = new stdClass();
  $update_node->nid = $node->nid;
  $update_node->vid = $node->vid;
  $update_node->type = $node->type;
  $update_node->field_text['und'][0]['value']       = 'Some new text!';
  field_attach_update('node', $update_node);

Result:
field_text field is successfully updated but the image is deleted and the field is set to null. field_text2 is left alone as expected.

Desired result:
Only field_text is updated. All other fields are left alone.

CommentFileSizeAuthor
#19 drupal7-1483736-11-testonly.patch1.01 KBstefan.r
FAILED: [[SimpleTest]]: [MySQL] 41,360 pass(es), 10 fail(s), and 5 exception(s). View
#19 drupal7-1483736-11-withtest.patch1.77 KBstefan.r
PASSED: [[SimpleTest]]: [MySQL] 41,372 pass(es). View
#11 drupal7-1483736-11.patch774 bytesstefan.r
PASSED: [[SimpleTest]]: [MySQL] 41,362 pass(es). View
Members fund testing for the Drupal project. Drupal Association Learn more

Comments

derek.deraps’s picture

Status: Active » Closed (works as designed)

I can confirm that the steps posted in this issue do lead to the reported data loss (field_image). However, I am marking this issue "closed (works as designed)" because the posted code does not use function field_attach_update as it was intended. See this code that uses field_attach_update as intended and works correctly:

  $update_node = node_load(1);
  $update_node->field_text['und'][0]['value'] = 'Some new text!';
  field_attach_update('node', $update_node);
emilymoi’s picture

I was about to post the same bug report and I can across this one describing the problem I am running into.

My understanding is that drupal should allow for saving of partial nodes. There is code that handles partial nodes in node_save and also in field_attach_update. An empty array != a missing field. This is handled all over the place.

Now who is at fault here? file_field_update() gets passed an empty array of $items so it really has no idea whether the entity had the field set or not. If we look upstream we find that in _field_invoke() does a check to see if the field is empty and if it is uses an empty array placeholder. That empty array is then passed in to the field hooks. Now, what I don't understand is why does _field_invoke() pass an empty $items array to the field_hooks on update? Why does it even need to call the hook_field_update() when the field does not exist? Is there something that relies on this? Seems like a bug to me.

Here is what I am currently using as a work-around for this problem. No idea what sort of side effects this will cause but IMO it makes no sense to update when the field does not exist.

--- field.attach.inc.old	2012-05-17 12:06:18.183243000 -0300
+++ field.attach.inc	2012-05-17 11:26:57.531243001 -0300
@@ -205,24 +205,30 @@
       $languages = _field_language_suggestion($available_languages, $options['language'], $field_name);
 
       foreach ($languages as $langcode) {
-        $items = isset($entity->{$field_name}[$langcode]) ? $entity->{$field_name}[$langcode] : array();
-        $result = $function($entity_type, $entity, $field, $instance, $langcode, $items, $a, $b);
-        if (isset($result)) {
-          // For hooks with array results, we merge results together.
-          // For hooks with scalar results, we collect results in an array.
-          if (is_array($result)) {
-            $return = array_merge($return, $result);
-          }
-          else {
-            $return[] = $result;
-          }
-        }
-
-        // Populate $items back in the field values, but avoid replacing missing
-        // fields with an empty array (those are not equivalent on update).
-        if ($items !== array() || isset($entity->{$field_name}[$langcode])) {
-          $entity->{$field_name}[$langcode] = $items;
-        }
+      	// XXX-CL don't invoke field operation if it does not exist on update
+      	$items = isset($entity->{$field_name}[$langcode]) ? $entity->{$field_name}[$langcode] :
+      		($op != 'update' ? array() : null);
+			
+      	if ($items !== null) {
+        	$result = $function($entity_type, $entity, $field, $instance, $langcode, $items, $a, $b);
+		
+	        if (isset($result)) {
+	          // For hooks with array results, we merge results together.
+	          // For hooks with scalar results, we collect results in an array.
+	          if (is_array($result)) {
+	            $return = array_merge($return, $result);
+	          }
+	          else {
+	            $return[] = $result;
+	          }
+	        }
+	
+	        // Populate $items back in the field values, but avoid replacing missing
+	        // fields with an empty array (those are not equivalent on update).
+	        if ($items !== array() || isset($entity->{$field_name}[$langcode])) {
+	          $entity->{$field_name}[$langcode] = $items;
+	        }
+		 }
       }
     }
   }
emilymoi’s picture

Status: Closed (works as designed) » Active
marcingy’s picture

Status: Active » Closed (works as designed)

This works as designed

j0rd’s picture

I just got bitten by a couple weeks of this on a user content generated site where I'm using back referencing entity references. Started to lose media from those who ended up touching my field_attach_update() code on a slimmed down object.

With this all said, has anyone found a proper way to update a single field for a node with out doing superfluous queries for fields which never get touched.

In my case, I allow my users to associate their content with other peoples content. Each of these content items have 50 or so fields. They can easily create as many associations as they want via a custom widget I've created. Upon save, I update a single field in the associated nodes, which links back to the node which the original author was creating these associations from. It's not uncommon for a user to associate themselves with 20 or so other nodes, so saving each field for each node creates hundreds of updates / insert queries which I'd like to avoid as it won't scale.

Any assistance would be appreciated.

hedley’s picture

Issue summary: View changes

I just got stung by this issue. There doesn't seem to be any information in the field_attach_update() documentation on how to use it properly, it might be a good idea to add something in?

stefan.r’s picture

Version: 7.12 » 7.x-dev
Status: Closed (works as designed) » Active

I got stung by this as well while implementing https://www.urbaninsight.com/2011/10/24/saving-nodes-fields-without-savi...

Regardless of how this was "designed", it seems a lot of people believe you can use field_attach_update() to update a single field using a partial node.

stefan.r’s picture

Other pages that suggest using field_attach_update() is safe:

https://drupal.stackexchange.com/questions/60713/update-a-single-field-w...

https://drupal.stackexchange.com/questions/64833/fast-saving-single-fiel...

As well as the comments in the documentation for field_attach_update():

https://api.drupal.org/api/drupal/modules!field!field.attach.inc/functio...

So we can only use field_sql_storage_field_storage_write() instead?


// Get the numeric id of your field by passing field name
$info = field_info_field('field_product_description');
$fields = array($info['id']);
 
// Execute the storage function
field_sql_storage_field_storage_write('node', $node, 'update', $fields);
 
// Clear field cache
cache_clear_all("field:node:$node->nid", 'cache_field');
mikeytown2’s picture

If this function shouldn't be called directly like this it should have been named _field_attach_update(). Since we can't rename it we need to fix this. While doing a bulk drush operation it nuked less than 1% of the files but still over 1,000 on our setup.

Other modules use it directly as well #2353491: File Usage count for revisions out of sync/incorrect not realizing how this might cause data loss.

Even the documentation for this acknowledges that this might be called outside of the normal way of doing things https://api.drupal.org/api/drupal/modules!field!field.attach.inc/functio...

Related:
#1141912: Changing node language by moving down language list deletes files in file field
#1399846: Make unused file 'cleanup' configurable
#1968442: RFC: How to prevent files from being deleted by file/media fields when unused

stefan.r’s picture

Priority: Normal » Major

Data loss so upgrading priority

stefan.r’s picture

Status: Active » Needs review
FileSize
774 bytes
PASSED: [[SimpleTest]]: [MySQL] 41,362 pass(es). View

Just to explain what happens here, field_attach_update() does _field_invoke('update', $entity_type, $entity);, which iterates through all the defined field instances of the bundle, doing the following:

        $items = isset($entity->{$field_name}[$langcode]) ? $entity->{$field_name}[$langcode] : array();
        $result = $function($entity_type, $entity, $field, $instance, $langcode, $items, $a, $b);

So this calls file_field_update() in order to "check for files that have been removed from the object", regardless of whether $entity->{$field_name} is even defined. So the value of $items will be an empty array regardless of whether the field is defined but empty/"file-less" or whether the field property is not even defined.

I guess we could either change file_field_update() (as in this patch), or field_invoke() directly?

mikeytown2’s picture

Status: Needs review » Reviewed & tested by the community

Just tested the patch in #11 and this fixes the issue.

ShaneOnABike’s picture

Priority: Major » Critical

I can also confirm that this works as expected. I agree that this is a fairly serious problem since it means that anyone not using node_save and directly saving fields unrelated to a file results in lost data.

jtsnow’s picture

I work on a large Drupal project that was recently affected by this. I can confirm that the patch fixes the problem.

Fabianx’s picture

Issue tags: +Needs tests

This will need a test and can anyone confirm that this is no longer an issue in 8.0.x?

Fabianx’s picture

Fabianx’s picture

Status: Reviewed & tested by the community » Needs work
stefan.r’s picture

Status: Needs work » Needs review
FileSize
1.77 KB
PASSED: [[SimpleTest]]: [MySQL] 41,372 pass(es). View
1.01 KB
FAILED: [[SimpleTest]]: [MySQL] 41,360 pass(es), 10 fail(s), and 5 exception(s). View

Status: Needs review » Needs work

The last submitted patch, 19: drupal7-1483736-11-testonly.patch, failed testing.

Fabianx’s picture

Status: Needs work » Reviewed & tested by the community
Issue tags: -Needs tests

Back to RTBC, thanks for the tests!

David_Rothstein’s picture

Priority: Critical » Normal
Status: Reviewed & tested by the community » Fixed

This is OK as a sane fallback behavior, but I really don't see how passing in anything other than a complete entity is supposed to be supported by the field API, so I'm lowering the priority...

Committed to 7.x - thanks!

  • David_Rothstein committed 277ae50 on 7.x
    Issue #1483736 by stefan.r, bfcam, jrigby: field_attach_update deletes...

Status: Fixed » Closed (fixed)

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

kikecastillo’s picture

I would like to share with you some issues you can have if you don't use db_transaction using field_attach_update function:

field_attach_update function is normally used within node_save function and then it is done "transactional". If this function is used without using db_transaction function before it is possible to lose content because field_attach_update calls field_sql_storage_field_storage_write function that will delete and insert fields in the database. If this function is not transactional then it could happen a wrong database state during the execution and lose content.

Example:
$transaction = db_transaction();
try {
$node = node_load(1); // where 1 is the nid you want to change
$update_node = new stdClass();
$update_node->nid = $node->nid;
$update_node->vid = $node->vid;
$update_node->type = $node->type;
$update_node->field_FIELD_NAME['und'][0]['value'] = VALUE_YOU_WANT_TO_CHANGE;
field_attach_update('node', $update_node);
} catch (Exception $e) {
$transaction->rollback();
throw $e;
}