Field invoke multiple does not handle field language correctly

Subscribing

We need tests for this condition, because they should've caught this.

By attachments, do you mean filefield uploads?

Got the same issue. Only happens if a file field is left empty and the node is displayed in a list. Looks like the $items array is not populated with all entity-ids occuring in $entities.
Same problem with taxonomy terms.

Same issue here, but in my case it also happens when I have nodes of different languages in the list even if all filefields have a file.

Notice: Undefined offset: 1 in file_field_prepare_view() (line 201 of [...]/modules/file/file.field.inc).
Warning: Invalid argument supplied for foreach() in file_field_prepare_view() (line 201 of [...]/modules/file/file.field.inc).
Notice: Undefined offset: 1 in file_field_prepare_view() (line 207 of [...]/modules/file/file.field.inc).
Warning: array_values() expects parameter 1 to be array, null given in file_field_prepare_view() (line 207 of [...]/modules/file/file.field.inc).
Notice: Undefined offset: 3 in file_field_prepare_view() (line 201 of [...]/modules/file/file.field.inc).
Warning: Invalid argument supplied for foreach() in file_field_prepare_view() (line 201 of [...]/modules/file/file.field.inc).
Notice: Undefined offset: 3 in file_field_prepare_view() (line 207 of [...]/modules/file/file.field.inc).
Warning: array_values() expects parameter 1 to be array, null given in file_field_prepare_view() (line 207 of [...]/modules/file/file.field.inc).

The attached patch should fix the issue, tests are still missing.

Thanks for getting on this so quickly, plach! :D

I should stop fixing core issues at 3:00 AM, see what happened the last time :(

Hi, when I try a dry run of the patch, I get this:

patch -p1 --dry-run < ./tf-1169426-7.patch

patching file modules/field/field.attach.inc
Hunk #1 FAILED at 303.
Hunk #2 FAILED at 312.
Hunk #3 FAILED at 329.
Hunk #4 FAILED at 347.

any ideas what I might be doing wrong?

Be sure to checkout the latest code from the 7.x branch, this applies cleanly to 7.2 however.

#7: tf-1169426-7.patch queued for re-testing.

suscribe

Patch #7 seems to fix the issue (or at least remove error messages ;) ).

Patch #7 removed the error messages for me to :)

Patch #7 solves #1164230: Invalid argument supplied for foreach() in taxonomy_field_formatter_prepare_view() line 1429 of taxonomy.module for me! Marking that one as duplicate.

Perhaps using the $items from the foreach is more clear (although the PHP manual states that foreach operates on a copy, the old version seems to work anyway):

    // Iterate over all the field translations.
    foreach ($grouped_items[$field_id] as $langcode => $items) {
      $results = $function($entity_type, $grouped_entities[$field_id][$langcode], $field, $grouped_instances[$field_id][$langcode], $langcode, $grouped_items[$field_id][$langcode], $a, $b);

    // Iterate over all the field translations.
    foreach ($grouped_items[$field_id] as $langcode => &$items) {
      $results = $function($entity_type, $grouped_entities[$field_id][$langcode], $field, $grouped_instances[$field_id][$langcode], $langcode, $items, $a, $b);

subscribe

Patch applied properly and errors are gone on 7.2 multisite site

I'd like to make sure this is part of 7.3.

Any takers on writing that test?

I'll do

Here are tests. The test-only patch is supposed to fail to show that tests capture the bug.

Stop that, bot. :)

eee...how to install this patch?

take the most recent patch (not the test only one) and remove the test part out of it before applying

sorry, I'm new to drupal could you explain it to me step by step?
thx for your replay

Try http://drupal.org/node/707484

#22 worked for me!

#22: tf-1169426-22.patch queued for re-testing.

#7: tf-1169426-7.patch queued for re-testing.

Sub

Sub

Ok, it seems the patch fixes the issue: a feedback from @yched and @sun would be welcome...

+++ b/modules/field/field.attach.inc
@@ -303,9 +303,6 @@ function _field_invoke_multiple($op, $entity_type, $entities, &$a = NULL, &$b =
-        $grouped_entities[$field_id][$id] = $entities[$id];

@@ -315,6 +312,10 @@ function _field_invoke_multiple($op, $entity_type, $entities, &$a = NULL, &$b =
+          $grouped_entities[$field_id][$langcode][$id] = $entities[$id];

@@ -327,8 +328,10 @@ function _field_invoke_multiple($op, $entity_type, $entities, &$a = NULL, &$b =
+      $entities = $grouped_entities[$field_id][$langcode];

@@ -346,9 +349,9 @@ function _field_invoke_multiple($op, $entity_type, $entities, &$a = NULL, &$b =
     // Populate field values back in the entities, but avoid replacing missing
     // fields with an empty array (those are not equivalent on update).
...
+    foreach ($grouped_entities[$field_id] as $langcode => $entities) {
+      foreach ($entities as $id => $entity) {
+        if ($grouped_items[$field_id][$langcode][$id] !== array() || isset($entity->{$field_name}[$langcode])) {
           $entity->{$field_name}[$langcode] = $grouped_items[$field_id][$langcode][$id];
         }
       }

I'm not really sure whether we also need to stack all entities by language. For monolingual sites, this change makes no difference. But on multilingual sites, $grouped_entities will contain {fields} x {languages} "copies" of each entity. Whereas it looks like we.......

well, scratch that - the overall processing logic already exists. Looks like _field_invoke_multiple() could use some @chx magic and @sun clean-up in a follow-up issue.

+++ b/modules/field/field.attach.inc
@@ -327,8 +328,10 @@ function _field_invoke_multiple($op, $entity_type, $entities, &$a = NULL, &$b =
-    foreach ($grouped_items[$field_id] as $langcode => $items) {
...
+    foreach ($grouped_items[$field_id] as $langcode => &$items) {

Is there a reason for allowing the callbacks to change $items in $grouped_items[$field_id] by reference?

Powered by Dreditor.

@sun:

I'm not really sure whether we also need to stack all entities by language. For monolingual sites, this change makes no difference. But on multilingual sites, $grouped_entities will contain {fields} x {languages} "copies" of each entity.

If I'm not mistaken objects are always passed by reference, so what get copied are object references not object data structures.

Is there a reason for allowing the callbacks to change $items in $grouped_items[$field_id] by reference?

Well, this is only a readability improvement: the old code already lets items be passed as reference, see for instance: http://api.drupal.org/api/drupal/modules--field--field.api.php/function/....

the old code already lets items be passed as reference

While that is true, the foreach ($items[$id] as $delta => $item) { generates a copy of &$items[$id][$delta] as $item. Only the results of the callback are put back to the &$items passed by reference, but the callback only gets the copy in $item.

I'd suggest to revert that change to the foreach for now. Deciding on whether field callbacks should be allowed to manipulate $items by reference looks like a different question to me that should be discussed in a separate issue.

I cannot parse your latest comment: the current _field_invoke_multiple() code passes $grouped_items[$field_id][$langcode] as the $items parameter of hook_field_X() (multiple) implementations. Implementing hooks may decide to get a reference or a copy depending on their signature.

Do you mean that the current code always passes a reference and so hooks altering for their private reasons the $items array may see their changes unintentionally propagated?

+++ b/modules/field/field.attach.inc
@@ -327,8 +328,10 @@ function _field_invoke_multiple($op, $entity_type, $entities, &$a = NULL, &$b =
-    foreach ($grouped_items[$field_id] as $langcode => $items) {
-      $results = $function($entity_type, $grouped_entities[$field_id], $field, $grouped_instances[$field_id], $langcode, $grouped_items[$field_id][$langcode], $a, $b);
+    foreach ($grouped_items[$field_id] as $langcode => &$items) {
...
+      $results = $function($entity_type, $entities, $field, $instances, $langcode, $items, $a, $b);

ah, ok, now I get it -- you also replaced $grouped_items[$field_id][$langcode] with $items in the arguments passed to the field callback.

Alright, makes sense.

Powered by Dreditor.

subscribe

subscribe

subscribe

subscribe

subscribe

Using view's frontpage, you don't get this warnings.

Awesome work! Thanks folks!!

Committed to 8.x and 7.x.

Hello !

I have a production site impacted by this issue.

Is the patch [tf-1169426-22.patch] reliable ?
Do we have to manually apply that patch, or is there a new core release planned soon ?

Many thanks in advance for your reply ! :-)

The patch in #22 got committed: a development snapshot is coming in 4 hours. If I understood the current plans correctly, the next stable release should be available by the end of June.

Everyone here is encouraged to test the patch by manually applying it or downloading the development snapshot, because the code in it will be part of D7.3. We have more than one week to fix possible side effects and ensure D7.3 behaves well, let's make this happen ;)

patch in #22 is working for me.

patch in #22 is working for me too :-)

Sub

For information, this patch is now included in Drupal 7.4 release.

Thanks to everybody ! :-)

i am running windows and cygwin doesnt work properly for me.
*pulling my hair out and crying*
cant somone upload the patched files instead of the patch.
thanks