Support from Acquia helps fund testing for Drupal Acquia logo

Comments

sun’s picture

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.

yched’s picture

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

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

fago’s picture

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().

yched’s picture

    // 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.

fago’s picture

yched’s picture

Version: 7.x-dev » 8.x-dev
Status: Needs work » Needs review
fago’s picture

file_field_update.patch queued for re-testing.

apaderno’s picture

Title: remove f_a_load() from file_field_update() » Remove file_attach_load() from file_field_update()
Berdir’s picture

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.

Berdir’s picture

file_field_update.patch queued for re-testing.

Status: Needs review » Needs work

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

Berdir’s picture

Status: Needs work » Needs review
FileSize
891 bytes

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

Damien Tournoud’s picture

Status: Needs review » Reviewed & tested by the community

This should have been committed about a year ago :)

Dries’s picture

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

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

Damien Tournoud’s picture

Reroll for Drupal 7.

apaderno’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
871 bytes

This is the patch for Drupal 7.

Damien Tournoud’s picture

Status: Needs review » Reviewed & tested by the community
webchick’s picture

Status: Reviewed & tested by the community » Fixed

Nice clean-up!

Committed and pushed to 7.x, thanks!

cweagans’s picture

Status: Fixed » Closed (fixed)

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

Deciphered’s picture

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.

sun’s picture

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?

Deciphered’s picture

Status: Active » Needs review
FileSize
1.38 KB

Patch attached

David_Rothstein’s picture

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.)

sun’s picture

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);
  }
apaderno’s picture

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.

apaderno’s picture

Status: Needs work » Needs review
FileSize
637 bytes

That was definitively the wrong Drupal version. :-(

Kristen Pol’s picture

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.

apaderno’s picture

Status: Needs work » Needs review
FileSize
635 bytes
Kristen Pol’s picture

Status: Needs review » Reviewed & tested by the community

Looks good to me... thanks!

erikhopp’s picture

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

eggthing’s picture

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 .

apaderno’s picture

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);
  }
B-Prod’s picture

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

webchick’s picture

Assigned: Unassigned » David_Rothstein

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

galooph’s picture

The patch in #36 worked nicely for me.

eggthing’s picture

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

stella’s picture

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

galooph’s picture

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

apaderno’s picture

Ralf Eisler’s picture

Patch in #35 solved the problem … thanks!

firfin’s picture

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

forssto’s picture

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?

webchick’s picture

Status: Reviewed & tested by the community » Needs review

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

forssto’s picture

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

magtak’s picture

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

magtak’s picture

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.

Damien Tournoud’s picture

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.).

forssto’s picture

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.

drzraf’s picture

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).

tbenice’s picture

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.

firfin’s picture

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.

apaderno’s picture

castawaybcn’s picture

patch in #41 worked for me with image field

David_Rothstein’s picture

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.

David_Rothstein’s picture

Status: Needs work » Needs review
FileSize
1.09 KB

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.

drzraf’s picture

FileSize
13.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.

anjjriit’s picture

Patch #41 works fine for me too.

jazzitup’s picture

Confirmed #41 works for me at D7.18.

DD 85’s picture

Version: 7.x-dev » 7.18

Confirmed # 58 works for version Drupal 7.18

Toshiro’s picture

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

apaderno’s picture

Version: 7.18 » 7.x-dev

Code is always changed in the development snapshot.

pav’s picture

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?

David_Rothstein’s picture

Version: 7.18 » 7.x-dev
apaderno’s picture

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

c3rberus’s picture

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".

Jaypan’s picture

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.

apaderno’s picture

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.

Deciphered’s picture

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.

Murat’s picture

Exploratus’s picture

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

Thanks!

jitse’s picture

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.

willvincent’s picture

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

robmalon’s picture

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.

firfin’s picture

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 ?

bsevere’s picture

#58 is working for me on 7.19.

eliosh’s picture

#58 is working for me on 7.21

David_Rothstein’s picture

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.