Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
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.
Comment | File | Size | Author |
---|---|---|---|
#59 | 985642-notice-func-stack.txt | 13.04 KB | drzraf |
#58 | field-attach-load-entity-original-985642-58.patch | 1.09 KB | David_Rothstein |
#41 | core-7.15-remove-file-attach-load-985642-41.patch | 668 bytes | galooph |
#36 | core-7.15-remove-file-attach-load-985642-36.patch | 672 bytes | B-Prod |
#31 | load-original-entity-985642-31.patch | 635 bytes | apaderno |
Comments
Comment #1
sunAwesome. RTBC if bot passes.
Comment #3
yched CreditAttribution: yched commentedlooks like there's a whole in populating $entity->original somewhere ?
No time to investigate tonight, anyone welcome to beat me to it.
Comment #4
fagoOh, 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().
Comment #5
yched CreditAttribution: yched commentedOuch. user_save() fail indeed.
Comment #6
fagoSee #999004: user_save() relies on $edit instead of $account.
Comment #7
yched CreditAttribution: yched commentedreviving this, now that #999004: user_save() relies on $edit instead of $account is in.
Comment #8
fagofile_field_update.patch queued for re-testing.
Comment #9
apadernoComment #10
BerdirReviving 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.
Comment #11
Berdirfile_field_update.patch queued for re-testing.
Comment #13
BerdirObviously won't apply anymore due to the core/ change. Here is a re-roll.
Comment #14
Damien Tournoud CreditAttribution: Damien Tournoud commentedThis should have been committed about a year ago :)
Comment #15
Dries CreditAttribution: Dries commentedCommitted to 8.x. Moving to 7.x.
Comment #16
Damien Tournoud CreditAttribution: Damien Tournoud commentedReroll for Drupal 7.
Comment #17
apadernoThis is the patch for Drupal 7.
Comment #18
Damien Tournoud CreditAttribution: Damien Tournoud commentedComment #19
webchickNice clean-up!
Committed and pushed to 7.x, thanks!
Comment #20
cweagansFixing tags per http://drupal.org/node/1517250
Comment #22
Deciphered CreditAttribution: Deciphered commentedJust 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.
Comment #23
sunFor 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?
Comment #24
Deciphered CreditAttribution: Deciphered commentedPatch attached
Comment #25
David_Rothstein CreditAttribution: David_Rothstein commentedIf $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.)
Comment #26
sunyeah, @David_Rothstein is right, the original code basically loaded the original entity.
Unless I'm mistaken, should be as simple as:
Comment #27
apadernoThis is the patch containing sun's suggestion, which I think it is the correct one.
Comment #29
apadernoThat was definitively the wrong Drupal version.
:-(
Comment #30
Kristen PolI 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.
Comment #31
apadernoComment #32
Kristen PolLooks good to me... thanks!
Comment #33
erikhopp CreditAttribution: erikhopp commentedThe patch in #31 also seemed to work for me.
Comment #34
eggthing CreditAttribution: eggthing commentedIs 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 .
Comment #35
apadernoSearch 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.Right before them, add the following lines.
Comment #36
B-Prod CreditAttribution: B-Prod commentedFor those interested, here is a patch against 7.15 tag, according to kiamlaluno comment #35.
Comment #37
webchickSince I obviously screwed this up to begin with, assigning to David for a more careful look.
Comment #38
galooph CreditAttribution: galooph commentedThe patch in #36 worked nicely for me.
Comment #39
eggthing CreditAttribution: eggthing commentedPatch #36 perfect for me too and has solved my problems. Thanks ever so much!
Comment #40
stella CreditAttribution: stella commentedPatch in #36 worked for me, though it contains trailing whitespace on lines 10 and 15
Comment #41
galooph CreditAttribution: galooph commentedHere's the patch from #36 with the trailing whitespace removed.
Comment #42
apaderno#31: load-original-entity-985642-31.patch queued for re-testing.
Comment #43
Ralf Eisler CreditAttribution: Ralf Eisler commentedPatch in #35 solved the problem … thanks!
Comment #44
firfin CreditAttribution: firfin commentedI had the same error messages (while deleting content using Views Bulk Operations).
Patch 41 fixed it for me.
Comment #45
forssto CreditAttribution: forssto commentedAm 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?
Comment #46
webchickWell, that certainly sounds like a "needs review." :)
Comment #47
forssto CreditAttribution: forssto commentedIf anybody needs more thorough descriptions of potentially dangerous scenarios, get in touch at tommi@kollabora.com and I can provide more details.
Comment #48
magtak CreditAttribution: magtak commentedEDIT: removed original post, was misleading, see post below.
Comment #49
magtak CreditAttribution: magtak commentedThis 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.
Comment #50
Damien Tournoud CreditAttribution: Damien Tournoud commentedThis 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.).
Comment #51
forssto CreditAttribution: forssto commentedForcing 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.
Comment #52
drzraf CreditAttribution: drzraf commentedInstall latest 7.x (5e3c77b607ed), activate
ET
, installtitle
, add a new language, add a newarticle
content (here fr) => translate in english => save :Notice: Undefined property: stdClass::$original in file_field_update() (line 265 of /modules/file/file.field.inc).
Comment #53
tbenice CreditAttribution: tbenice commentedAlso, 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.
Comment #54
firfin CreditAttribution: firfin commentedAs 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.
Comment #55
apaderno#31: load-original-entity-985642-31.patch queued for re-testing.
Comment #56
castawaybcn CreditAttribution: castawaybcn commentedpatch in #41 worked for me with image field
Comment #57
David_Rothstein CreditAttribution: David_Rothstein commentedHm, 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: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.
Comment #58
David_Rothstein CreditAttribution: David_Rothstein commentedI 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.
Comment #59
drzraf CreditAttribution: drzraf commentedI 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.Comment #60
anjjriit CreditAttribution: anjjriit commentedPatch #41 works fine for me too.
Comment #61
jazzitup CreditAttribution: jazzitup commentedConfirmed #41 works for me at D7.18.
Comment #62
DD 85 CreditAttribution: DD 85 commentedConfirmed # 58 works for version Drupal 7.18
Comment #63
Toshiro CreditAttribution: Toshiro commentedI 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
Comment #64
apadernoCode is always changed in the development snapshot.
Comment #65
pav CreditAttribution: pav commented#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?
Comment #66
David_Rothstein CreditAttribution: David_Rothstein commentedComment #67
apadernoPatches are always written for the development snapshot, not for a release.
Comment #68
c3rberus CreditAttribution: c3rberus commentedpatch 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".
Comment #69
Jaypan CreditAttribution: Jaypan commentedOnce 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.
Comment #70
apadernoWhat 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.
Comment #71
Deciphered CreditAttribution: Deciphered commentedexxoid,
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.
Comment #72
Murat CreditAttribution: Murat commented#58: field-attach-load-entity-original-985642-58.patch queued for re-testing.
Comment #73
Exploratus CreditAttribution: Exploratus commentedPatch works for me. I was getting the error using DRealty Module, and was referenced here via other tickets.
Thanks!
Comment #74
jitse CreditAttribution: jitse commentedI 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.
Comment #75
willvincent CreditAttribution: willvincent commentedThe patches #31, #41, and #58 all is work for me on 7.20.. so, take your pick :)
Comment #76
robmalon CreditAttribution: robmalon commentedI'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.
Comment #77
firfin CreditAttribution: firfin commentedI 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 ?
Comment #78
bsevere CreditAttribution: bsevere commented#58 is working for me on 7.19.
Comment #79
eliosh#58 is working for me on 7.21
Comment #80
David_Rothstein CreditAttribution: David_Rothstein commentedThanks 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...