Get rid of file_field_load() - shouldn't load all file object data with field load - only file ID

Yeah the right place to load referenced entities is in entity/field_prepare_view() - you can do multiple load in there so it's cheap, and saves duplicating information in the cache.

assigning this to myself. Patch forthcoming.

and here we go

I manually tested this: uploaded image, attached it to a node, removed it from the node. One minor nit, the $fids array initialization was removed in the above patch in #4; I added that back in.

Since $item no longer has the full file object metadata payload, we now need to use file_load($item['fid']) to get the file info rather than type casting the $item array. There are almost certainly more such cases in the file, but we can address them as problems arise.

An NOW without a trailing whitespace patch error.

Patch applies cleanly. Single file (and multiple files) can be added and removed without error. Files are unlinked and also deleted from the file system.

#7: load-files-on-view-1446464-7.patch queued for re-testing.

Awesome, this probably doesn't apply anymore because the presave hook is gone :)

This would also conflict with #1361226: Make the file entity a classed object, so let's wait with a re-roll until that one is in. Also let's have another look at how and what we're passing to theme_file_link().

Bumping to major bug, I bet we don't clear the field cache when those file objects get updated either. This is probably too big a change to backport to D7 though unfortunately.

Nodes and comments load user properties and add it to the main object, that results in exactly the same problem when using persistent entity caches. Not sure if there is already a bug report and if it should be major (because core doesn't do persistent entity caching, but supports pluggable caching) but it's something I want to fix.

Completely untested re-roll...

I'm not sure if the tests passed before, but I don't think this could have been caused by the file entity change.

Obviously, after this change, we can't do an (object) cast anymore of the field item and expect a file object, so I converted all of them to a file_load().

That we are loosing some properties is for the tests not relevant, unlike the similar, reverted changes in the file entity patch. We know that file_link isn't going to be using them and most checks are just about the uri and filename.

There is also one instance that needs to be changed in file_file_download(). I made the change there as simple as possible, it will conflict with #1245220: file_file_download() passed bogus $field to field_access() anyway. Could be improved to do a single load although that has the downside that we might load more than necessary.

And now with patch.

More similar test fixes for the image tests. This one should pass.

Needs reroll for the PSR-0-ification of test classes.

Rerolled.

Ugh.

Doing this as part of core office hours, with help from tim.plunkett.
http://drupalofficehours.org/task/198

* Added an image to an article.
* Created a very simple module, which implements hook_node_load().

/**
 * Implementation of hook_node_load().
 */
function mymodule_node_load($nodes, $types) {
  print_r('<pre>');
  print_r($nodes);
  print_r('</pre>');
}

See attachments for before and after the patch has been applied.

field_image output is now like this:

            [field_image] => Array
                (
                    [und] => Array
                        (
                            [0] => Array
                                (
                                    [fid] => 1
                                    [alt] => 
                                    [title] => 
                                    [width] => 128
                                    [height] => 128
                                )

                        )
                )

This looks good to me, I'm getting the same result with node_load() as the other reviewers. The contents of e.g. {field_data_field_image} are loaded, but not the file entity. The change is also consistent with the docs for hook_field_load() which say "never use this hook to load fieldable entities... use hook_field_formatter_prepare_view() instead".

Patch looks great. Gets rid of lots of nasty casting etc. committed/pushed to 8.x.

Needs backport to D7: image fields on 7.x still carry the whole set of information. cf screen copy.

In addition, although not completely the same topic, from profiling an actual site where this information was taken, it looks like we could just save significant PHP time by not calling file_field_load() if empty($items), like:

/**
 * Implements hook_field_load().
 */
function image_field_load($entity_type, $entities, $field, $instances, $langcode, &$items, $age) {
  if ($items) {
    file_field_load($entity_type, $entities, $field, $instances, $langcode, $items, $age);
   }
}

Just a quick patch to see what breaks if we add this optimization.

Missing patch..

Previous patch was wrong: $items is indexed per-entity. Rerolled.

+++ b/modules/image/image.field.inc
@@ -192,7 +192,20 @@ function _image_field_resolution_validate($element, &$form_state) {
+    if (count($entity_items) > 0) {

I think this should be changed to something like:

!empty($entity_items)

which is a more common pattern in Drupal, I think it's also more efficient since you don't need to know how many items are in the array, just that it's not empty.

The formulations are not strictly equivalent, for example count(FALSE) > 0 is TRUE, while !empty(FALSE) is FALSE.

This being said, is normal use cases of the field structure, they /should/ always be equivalent : content is always expected to be NULL (equivalent) or an array (always equivalent ?).

Meh...