file_field_update() currently calls field_attach_load() to compare old values and new values take action on files that were removed (media module does the same in contrib)

#651240: Allow modules to react to changes to an entity allows us to clear that up.

Files: 
CommentFileSizeAuthor
#59 985642-notice-func-stack.txt13.04 KBdrzraf
#58 field-attach-load-entity-original-985642-58.patch1.09 KBDavid_Rothstein
PASSED: [[SimpleTest]]: [MySQL] 39,723 pass(es).
[ View ]
#41 core-7.15-remove-file-attach-load-985642-41.patch668 bytesgalooph
PASSED: [[SimpleTest]]: [MySQL] 39,379 pass(es).
[ View ]
#36 core-7.15-remove-file-attach-load-985642-36.patch672 bytesB-Prod
PASSED: [[SimpleTest]]: [MySQL] 39,338 pass(es).
[ View ]
#31 load-original-entity-985642-31.patch635 byteskiamlaluno
PASSED: [[SimpleTest]]: [MySQL] 39,544 pass(es).
[ View ]
#29 load-original-entity-985642-29.patch637 byteskiamlaluno
PASSED: [[SimpleTest]]: [MySQL] 39,340 pass(es).
[ View ]
#27 load-original-entity-985642-27.patch657 byteskiamlaluno
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch load-original-entity-985642-27.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#24 no_original-985642-24.patch1.38 KBDeciphered
PASSED: [[SimpleTest]]: [MySQL] 39,353 pass(es).
[ View ]
#17 remove-file-attach-call-in-file-update-985642-16.patch871 byteskiamlaluno
PASSED: [[SimpleTest]]: [MySQL] 39,126 pass(es).
[ View ]
#16 remove-file-attach-call-in-file-update-985642-12.patch871 bytesDamien Tournoud
PASSED: [[SimpleTest]]: [MySQL] 39,111 pass(es).
[ View ]
#13 remove-file-attach-call-in-file-update-985642-12.patch891 bytesBerdir
PASSED: [[SimpleTest]]: [MySQL] 36,854 pass(es).
[ View ]
file_field_update.patch863 bytesyched
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch file_field_update.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Comments

Status:Needs review» Reviewed & tested by the community

Awesome. RTBC if bot passes.

Status:Reviewed & tested by the community» Needs work

The last submitted patch, file_field_update.patch, failed testing.

looks like there's a whole in populating $entity->original somewhere ?

No time to investigate tonight, anyone welcome to beat me to it.

Oh, the lovely user.module! It just passes the field API a fake entity, thus there is no $entity->original.

Perhaps we can streamline that, similar as we try in #968458: Missing hook_entity_presave().

    // Load the stored entity, if any.
    if (!empty($account->uid) && !isset($account->original)) {
      $account->original = entity_load_unchanged('user', $account->uid);
    }
    // Presave field allowing changing of $edit.
    $edit = (object) $edit;
    field_attach_presave('user', $edit);
    $edit = (array) $edit;

Ouch. user_save() fail indeed.

Version:7.x-dev» 8.x-dev
Status:Needs work» Needs review

file_field_update.patch queued for re-testing.

Title:remove f_a_load() from file_field_update()Remove file_attach_load() from file_field_update()

Reviving again, we need this to remove the concept of stub entities completely in #1551140: Remove stub entities, replace entity_create_stub_entity().

Will trigger a re-test and if it passes, RTBC. Changes look fine to me.

file_field_update.patch queued for re-testing.

Status:Needs review» Needs work

The last submitted patch, file_field_update.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new891 bytes
PASSED: [[SimpleTest]]: [MySQL] 36,854 pass(es).
[ View ]

Obviously won't apply anymore due to the core/ change. Here is a re-roll.

Status:Needs review» Reviewed & tested by the community

This should have been committed about a year ago :)

Version:8.x-dev» 7.x-dev

Committed to 8.x. Moving to 7.x.

StatusFileSize
new871 bytes
PASSED: [[SimpleTest]]: [MySQL] 39,111 pass(es).
[ View ]

Reroll for Drupal 7.

Status:Reviewed & tested by the community» Needs review
StatusFileSize
new871 bytes
PASSED: [[SimpleTest]]: [MySQL] 39,126 pass(es).
[ View ]

This is the patch for Drupal 7.

Status:Needs review» Reviewed & tested by the community

Status:Reviewed & tested by the community» Fixed

Nice clean-up!

Committed and pushed to 7.x, thanks!

Status:Fixed» Closed (fixed)

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

Just a note, not sure if it's worth re-opening or creating a new issue for, but this caused an issue with File (Field) Paths, which calls field_attach_update() to save newly created and moved files even when the Entity doesn't have an $original value.

Simple fix would be to wrap this condition to check if an $original value exists, in the case of File (Field) Paths the fix was just to force a $value to exist.

Cheers,
Deciphered.

Status:Closed (fixed)» Active

For D8, I guess we want to rely on ::$original being set.

For D7, wrapping this code into a condition might make sense. It's indeed a slight change in behavior.

@Deciphered: Can you roll a patch with your suggestion?

Status:Active» Needs review
StatusFileSize
new1.38 KB
PASSED: [[SimpleTest]]: [MySQL] 39,353 pass(es).
[ View ]

Patch attached

If $entity->original isn't set, do we really want to skip the next block of code?

Or do we want to call entity_load_unchanged() and get the original entity from that instead? (Unless by the time this code is called, we're too late in the process for entity_load_unchanged() to actually do what it advertises... not sure about that offhand.)

yeah, @David_Rothstein is right, the original code basically loaded the original entity.

Unless I'm mistaken, should be as simple as:

  if (empty($entity->original)) {
    $entity->original = entity_load_unchanged($entity_type, $id);
  }

StatusFileSize
new657 bytes
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch load-original-entity-985642-27.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

This is the patch containing sun's suggestion, which I think it is the correct one.

Status:Needs review» Needs work

The last submitted patch, load-original-entity-985642-27.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new637 bytes
PASSED: [[SimpleTest]]: [MySQL] 39,340 pass(es).
[ View ]

That was definitively the wrong Drupal version. :-(

Status:Needs review» Needs work

+++ b/modules/file/file.field.inc
@@ -261,6 +261,11 @@ function file_field_update($entity_type, $entity, $field, $instance, $langcode,
+  // Load the original entity, if $entity->original is not already set.
+  if (empty($entity->original)) {
+    $entity->original = entity_load_unchanged($entity_type, $id);
+  }
+  ¶

I think there is an extraneous space or hidden character on the new line.

I applied the patch and am no longer getting the error:

Undefined property: stdClass::$original in file_field_update()

So it's RTBC for me except the extra space / hidden character on the new line.

Status:Needs work» Needs review
StatusFileSize
new635 bytes
PASSED: [[SimpleTest]]: [MySQL] 39,544 pass(es).
[ View ]

Status:Needs review» Reviewed & tested by the community

Looks good to me... thanks!

The patch in #31 also seemed to work for me.

Version:7.x-dev» 7.15

Is there a workaround to get it working in 7.15 rather than waiting for the fix in 7.16?

#27 #29 and #31 patches all failed for me in 7.15 .

Version:7.15» 7.x-dev

Search the modules/file/file.field.inc file; in that file, search for the file_field_update($entity_type, $entity, $field, $instance, $langcode, &$items) function, and in that function look for the following code lines.

  // Compare the original field values with the ones that are being saved.
  $original = $entity->original;
  $original_fids = array();

Right before them, add the following lines.

  // Load the original entity, if $entity->original is not already set.
  if (empty($entity->original)) {
    $entity->original = entity_load_unchanged($entity_type, $id);
  }

StatusFileSize
new672 bytes
PASSED: [[SimpleTest]]: [MySQL] 39,338 pass(es).
[ View ]

For those interested, here is a patch against 7.15 tag, according to kiamlaluno comment #35.

Assigned:Unassigned» David_Rothstein

Since I obviously screwed this up to begin with, assigning to David for a more careful look.

The patch in #36 worked nicely for me.

Patch #36 perfect for me too and has solved my problems. Thanks ever so much!

Patch in #36 worked for me, though it contains trailing whitespace on lines 10 and 15

StatusFileSize
new668 bytes
PASSED: [[SimpleTest]]: [MySQL] 39,379 pass(es).
[ View ]

Here's the patch from #36 with the trailing whitespace removed.

#31: load-original-entity-985642-31.patch queued for re-testing.

Patch in #35 solved the problem … thanks!

I had the same error messages (while deleting content using Views Bulk Operations).
Patch 41 fixed it for me.

Am I completely wrong, but isn't the proposed fix kind of dangerous?

My specific use case is from the File module's File field.

I use the mechanism of only running field_attach_presave and field_attach_update with dummy objects outlined here:
http://blog.urbaninsight.com/2011/10/24/saving-nodes-fields-without-savi...

It works great other than the fact that it seems hitting field_attach_update also triggers hook_field_update on all of the fields of the entity.

If in the scenarios where the $entity->original is empty we load the full entity with entity_load_unchanged, what we get is the complete entity with all fields.

In the case of file_field_update for example, that creates the situation where the $entity object that the function gets, might only contain one property, the one we're updating with field_attach_update.

Then when file_field_update goes through the check of what has changed, all the original's values will be missing from the current item thus creating a scenario where it'll interpret ALL of the values as having been deleted, thus running file_field_delete_file on them. Essentially deleting files.

Am I in the wrong here, or is the proposed fix outright dangerous and might lead to massive loss of data?

Status:Reviewed & tested by the community» Needs review

Well, that certainly sounds like a "needs review." :)

If anybody needs more thorough descriptions of potentially dangerous scenarios, get in touch at tommi@kollabora.com and I can provide more details.

EDIT: removed original post, was misleading, see post below.

This patch, creates problems in modules (like entityreference_count http://drupal.org/project/entityreference_count) that need to perform an entity_load() during the presaving and updating hook cycle.

entity_load_unchanged resets the cache (unavoidably) thus providing the modules in question with the wrong version of the entity. Took a lot of debugging to figure this out.

Maybe, entity_load_unchanged should be replaced with a query or something that wouldn't reset the cache thus allowing additional entity_load() calls during the page callback that's being run.

One can argue that the module's shouldn't collect data or anything similar during that cycle and that it's bad practice. I don't know, just reporting the issue.

Also, maybe the $entity->original should have been passed into the $entity object way before the hook_field_presave cycles. I do not think it's valid for each module that needs the original to check if it's not there and set it, reseting the cache each time. Sounds risky.

I use the mechanism of only running field_attach_presave and field_attach_update with dummy objects [...]

This use case has never been supported. I strongly advice against trying to do this. The proper way of updating fields is to load the entity, modify it and save it back. Any other way is done under the feet of the Entity System and will cause issues with modules that control access to or track the life cycle of the entities (the list is long: Apache Solr, Search API, Drupal Commerce, etc.).

Forcing to use node_save every single time you save something related to the node is just plain overkill. It should be up to the architecture to decide whether a change warrants a "full" node_save or whether it's more a metadata, like a counter update or similar.

For example, for performance reasons we maintain a number of published commerce_product entities for product display nodes. Also, I recently added a field to our nodes for maintaining the timestamp when the node was published and I had to run a migrate script to 15k nodes. Running that script with full node_save vs. just doing a field_attach_update was an order of magnitude difference in performance. There's no way the update on all those nodes warranted the whole set of actions that a full node_save does because nothing in the node's content itself really changed, just metadata / precalculate-type fields.

...and yes, I know we could have custom tables for those things, but then we'd lose the niceness of using EntityFieldQuery for querying based on that data.

Install latest 7.x (5e3c77b607ed), activate ET, install title, add a new language, add a new article content (here fr) => translate in english => save :
Notice: Undefined property: stdClass::$original in file_field_update() (line 265 of /modules/file/file.field.inc).

Also, if you wish to update fields for some reason on node update, then calling node_save will trigger an infinite loop. field_attach_update is needed then.

I agree that triggering a full node_save for field updates is not preferred for performance reasons.

As stated in #44 the patch from #41 removed the errors while using VBO for one of my sites. Unfortunately the error re-appears when I try to import a content-type with image fields using the node_export module.

I will investigate further, check if undoing the patch resolves this issue, post in node_export issue queue linking here and try any recommendations.

#31: load-original-entity-985642-31.patch queued for re-testing.

patch in #41 worked for me with image field

Assigned:David_Rothstein» Unassigned
Status:Needs review» Needs work

Hm, I think this was only assigned to me back when it was marked RTBC... unassigning myself for now.

In any case, regardless of all the other recent comments, isn't there a more fundamental reason why we can't use entity_load_unchanged() here? (Or at the very least, why we can't do $entity->original = entity_load_unchanged($entity_type, $id).) In #25 I suggested we look at this:

Or do we want to call entity_load_unchanged() and get the original entity from that instead? (Unless by the time this code is called, we're too late in the process for entity_load_unchanged() to actually do what it advertises... not sure about that offhand.)

I'm not sure anyone looked into that, but I just did now and it's definitely a problem. For example, node_save() calls field_attach_update() after it has already written some node data to the database. The fields haven't been written yet, but the node properties have. So calling entity_load_unchanged() after that point will give you some kind of in-between node from the database (e.g. if the node title has been changed, you'll get an object with the new node title and but the original field values). We definitely shouldn't stick that in $entity->original and pass it along to whatever hooks run after this one.

Status:Needs work» Needs review
StatusFileSize
new1.09 KB
PASSED: [[SimpleTest]]: [MySQL] 39,723 pass(es).
[ View ]

I only tested this patch briefly, but for Drupal 7 I think the thing to do here might just be to go back to the original code we had whenever $entity->original is not available. As far as I know, it never caused anyone any problems (just was ugly)?

So this patch continues to use $entity->original if it's there, but falls back on the old method if it's not.

This could use testing especially in scenarios where the second code path (the one without $entity->original) gets used.

StatusFileSize
new13.04 KB

I tested it during a title -> title field replacement where I was experiencing this notice (#1801242: Drush Extension : Replace %field with a field instance).
Using this patch the process completed without notice.
I attached a log from the session throwing the notice, in case this could help.
Maybe related to the strange "File attachments" field (from D6) whose $instance[widget] has no "module" key, but that's probably another issue.

Patch #41 works fine for me too.

Confirmed #41 works for me at D7.18.

Version:7.x-dev» 7.18

Confirmed # 58 works for version Drupal 7.18

I have the following error message after applying #58 - "Notice: Array to string conversion in DrupalDefaultEntityController->cacheGet() (line 369 of /var/www/drupal/includes/entity.inc)."

Drupal 7.18

Version:7.18» 7.x-dev

Code is always changed in the development snapshot.

Version:7.x-dev» 7.18

#41 working on D7.18 for me. If the patch was originally written for D7.15, how come it was not included in the D7.18?

Version:7.18» 7.x-dev

Patches are always written for the development snapshot, not for a release.

patch worked for me as well, as regards to "patches always written for dev", when does this make it to release if ever?

Meaning, should I note this change in my docs so that I can re-apply it every drupal update (such as 7.18 to 7.19), or does dev get committed to release at some point? because it appears it did not with the recent .19 release.

Just want to make sure I have the appropriate process in place if this is not going to get applied to "release".

Once a patch is committed to dev, it becomes part of the next release. So if this patch is not committed before the next release, you will have to re-patch.

What I meant is that the version field should be set to 7.x-dev because patches are for the development snapshot. Setting it to 7.18 because the patch works for that version is not necessary, as the patch needs to work with the development snapshot.

exxoid,

The issue is still marked as 'needs review', which means 'This patch needs to be reviewed to determine if it's ready to be committed', if you have thoroughly reviewed and tested the patch and you are 100% positive it is ready you can mark it as 'reviewed & tested by the community', in which case it will then go into the queue to be considered for a commit.

While the issue sits in 'needs review' state it won't be put into core.

Patch works for me. I was getting the error using DRealty Module, and was referenced here via other tickets.

Thanks!

Status:Needs review» Reviewed & tested by the community

I had the same issues on 7.19. If I recall correctly I din't have this issue on a vanilla install but as it stands we seldomly end up with vanilla's after a project is finished. Patch works for me too.

The patches #31, #41, and #58 all is work for me on 7.20.. so, take your pick :)

I'm using Corresponding Entity References (CER) and have references to $replacements[$original] in a hook_tokens() function. I figured one of those caused this error for me. I put the patch #58 in place and all seems to be working well now.

I bumped my head on this notice for the third time today (see also comments 44 and 54) This time while using corresponding entity relation (CER).

Patch from #58 fixes all three occurences, everything seems to be working.

Besides which it really seems to make sense, use the old it-wasn't-broke-so-don't-fix-it way if we can't use entity ->original.
I say RTBC, may I ?

#58 is working for me on 7.19.

#58 is working for me on 7.21

Status:Reviewed & tested by the community» Fixed

Thanks for the testing, everyone!

Committed to 7.x - http://drupalcode.org/project/drupal.git/commit/e4037b0

Hopefully we've seen the last of this issue...

Status:Fixed» Closed (fixed)
Issue tags:-needs backport to D7

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