String offset cast notice in field_invoke_method_multiple()

Patch #2 worked for me in 7.16

Patch worked successfully in 7.17. Thank you!

The patch should be updated for Drupal 8

(It's the workflow to fix bugs that happen in both 7 and 8 versions)

Welcome crevillo. Yes, it's standard policy to fix things in the development version (Drupal 8) first, then backport to the current version (Drupal 7). If you can provide a patch for Drupal 8 that would be great. Thanks!

Yes, and this is why the current patch cannot be applied: it's a D7 patch and the issue is now marked for 8.x-dev.

I think the patch is wrong.

If $options is not an array, then $langcode is set to $options['langcode'], which does not exist.

Ok, I assumed that $options may not be an array because what your patch does is check if it's an array.
Maybe you wanted to put:
is_array($options['langcode'])
instead of:
is_array($options)
?

Seems like test's fault ?

if ($language_widget && is_array($form_langcode)) {
...

Looks good to me.

Hard to say for sure, but this very much looks like some contrib module doing something wrong, which then triggers errors down the line in a core function. The core policy is to not add opportunistic validity checks to swallow errors coming from contrib.

In order to get anything done here, precise (and if possible simple) steps to reproduce the bug should be provided.
Does this happen on a Drupal Commerce site ? Is it based on Commerce Kickstart ?
If so, I'd suggest moving the issue over there so that their team can try to refine this.

We still don't know where this is coming from, so keeping the status.

So, looking at this a little closer :
_field_invoke_multiple() accepts $options['langcode'] ($options['language'] in D7) either as a langcode or as a 2-level array of langcodes keyed by entity id and field name.
Contrary to what I wrote in #26, It might well be true that the current check is a bit sloppy on that side - using isset($options['langcode'][$id]) to determine whether $options['langcode'] is one form or the other.

Still, we'd really need some detailed steps to reproduce to determine what's happening exactly, and add a proper test in our test suite so that this doesn't happen again.

Crosspost :-) See my #29.

#20: string-offset-cast-1824820-20.patch queued for re-testing.

#2: string-offset-cast-1824820-2.patch queued for re-testing.

cross post to http://drupal.org/node/1900456 where I just posted this bug with some perhaps helpful debug info.

Patch worked successfully in 7.17 (Commerce Kickstart). Thank you!

Ok @Tinnka but please don't change this as we still need that tag, and the version the patches are applied to is 'dev'

This patch at http://drupal.org/node/1824820#comment-6746564 also working with 7.x-dev.
Thanks. :)

I'm using EntityWrapper to programmatically create a node. I confirm this notice occurs in PHP 5.4, but not in PHP 5.3.

I run into this notice when using the Automatic Nodetitles module.

When I leave the Pattern for the title empty, no notice.
When I insert a token for a field of the content type to be used as the title, I get the notice.

On D7, PHP 5.4.4

*Edit* and: no issues when using PHP 5.3.14

Anything I can do to move this forward? Client project with auto nodetitle is affected.

Right, sorry for the delay :-(
Thanks a lot @earline for the explanations in #33, makes total sense.
Patch #2 is the correct fix, then.

Doesn't need to be fixed in D8, _field_invoke_multiple() is gone. and D8's EntityNG + Entity translation API makes the corresponding code completely different and not affected by this.

In Drupal 8 I see the following code in field_invoke_method_multiple():

  $langcode = !empty($options['langcode'][$id]) ? $options['langcode'][$id] : $options['langcode'];

Are we sure Drupal 8 isn't affected?

And could this use a test?

Doesn't patch #2 hack the core? Should I do it or wait for this to be ported to a new version of D7 core?
Any progress on this issue? If this is fixed, why not simply commit it?

It seems that the problem is caused by line 303 that calls entity_extract_ids for a node that has not been saved yet. Such node has it's nid set to NULL. Hence the $id in line 303 is also set to NULL.

// Determine the list of instances to iterate on.
list($id, $vid, $bundle) = entity_extract_ids($entity_type, $entity);

Now take a look at this:

$options['language'] = 'und';

// For PHP < 5.4 
print empty($options['language'][NULL]); // silently prints 'u'.

// For PHP >= 5.4
print empty($options['language'][NULL]); // raises a notice AND prints 'u'.

This is new for PHP 5.4

The problem is not the fact that $options['language'] is not always an array, but the way the check is performed. Current code clearly allows $options['language'] to be a string, so the patch from #2 is not a hack and should be applied.

Do we need to add tags for STR and tests to this? Are they blocking this issue?

I'm totally not an expert on automated patch testing, but it looks to me like field.test has unit tests that should catch any problems with this patch. And the patch passed testing. Am I missing something?

According #50

I've changed

$langcode = !empty($options['langcode'][$id]) ? $options['langcode'][$id] : $options['langcode'];
to
$langcode = $id != NULL &&  !empty($options['langcode'][$id]) ? $options['langcode'][$id] : $options['langcode'];

And alert is gone.

I have been able to trace this problem from a contrib module to core and it looks like SiliconMind in #50 is correct about entity_extract_ids().

I am currently setting up a Commerce site running PHP5.4 and that the inline_entity_form module is previewing an entity before it is saved with an ID so the ID is returned as NULL when creating new product variants.

I was able to work around the problem by overriding the theme function theme_inline_entity_form_entity_table($variables) and added the following.

  // Build an array of entity rows for the table.
  $rows = array();
  foreach (element_children($form) as $key) {    
    $entity = $form[$key]['#entity'];
    
+    // Workaround for core bug #1824820
+    // entity_extract_ids returns null when entity does not exist and _field_invoke_multiple tries to 
+    // check options for null key index for language that throws a notice.
+    $info = entity_get_info($entity_type);    
+    $entity->{$info['entity keys']['id']} = 0;
+    $entity->language = array(0 => $entity->language);
    
    list($entity_id) = entity_extract_ids($entity_type, $entity);        
    // Many field formatters (such as the ones for files and images) need
    // certain data that might be missing on unsaved entities because the field
    // load hooks haven't run yet. Because of that, those hooks are invoked
    // explicitly. This is the same trick used by node_preview().
    if ($form[$key]['#needs_save']) {
      _field_invoke_multiple('load', $entity_type, array($entity_id => $entity));
    }

This make the language options index an array rather than a string and by making the ID property on the entity object a ZERO it is easily to reference in the options array.

I hope that makes sense and is helpful.

I think #50 is right on. Like yoroy's #44, I ran into this with the Automatic Node Title module. If I set the node title to use a field token and then create a new node, the result is that this method gets called with an unsaved entity. Thus, the $id value is NULL, and the !empty($options['langcode'][$id]) check produces the "string offset cast" notice. Adding a !empty($id) condition to that statement fixes the problem. See attached patch.

Also: updating the issue title to reflect the D8 function name and downgrading priority to normal (since this is only a PHP notice message, not a warning or error).

And for those of us encountering this issue on D7, here's the backported patch.

Patch from #56 worked for my d7 installation. Thanks!

The problem persists with drupal's update to 7.25. Patch from #56 solves that too.

I confirm that patch from #56 works in Drupal 7.25

Thanks to muriqui we now have a Drupal 7 and 8 patch that is confirmed to work.

Field language system has been reworked completely in D8 - this patch probably became redundant for D8.

#56 work for me on D7, thank you

Looks good in D7.26. I say it's ready to be committed.

Re-upload of #56 to go through testing for D7.

#65 worked for me in my D7 panopoly-based distro.

I had the notice using rules with a content type which title is managed by auto_nodetitle.

Thanks

#65 takes care of this issue and works

re: #1824820-47: String offset cast notice in field_invoke_method_multiple() & #1824820-61: String offset cast notice in field_invoke_method_multiple()

I did a quick grep through D8 code base. There's no mention of anything closely resembling the code from #1824820-65: String offset cast notice in field_invoke_method_multiple(). RTBCing again.

Yes, that API has been completely revamped in D8, iterating on fields and handling of field translation has nothing in common with D7 anymore.

#65 worked for me as well

#65 works for me also.

#65 works for me also.

# 65 works well on D 7.26

65: string-offset-cast-1824820-65-D7.patch queued for re-testing.

The patch was tested without errors! It fixes the problem - no notices.

65: string-offset-cast-1824820-65-D7.patch queued for re-testing.

All tests are passed again.

#65 works well with current release version of D7.

#65 is working for D7.

65: string-offset-cast-1824820-65-D7.patch queued for re-testing.

All tests are passed again.

The original RTBC patch was #2 (which looks good to me), but this was changed in recent patches without much explanation. If the bug is that the code treats $options['language'] as an array even when it's not, why would we only fix this when $id is NULL (rather than actually checking that it's an array like #2 did)?

I guess we can live without tests if this is an obscure bug (and was presumably fixed as a side effect in Drupal 8 without any tests)...

Due to the way that empty() functions in PHP 5.4, simply checking for is_array isn't sufficient. Checking for empty on $id is important. It could be an array but the warning still displays. #1824820-35: String offset cast notice in field_invoke_method_multiple() does a good job explaining.

For those wondering if the patch in #65 works with the field.attach.inc file included with Drupal 7.28, it does. There were no changes in field.attach.inc between 7.27 and 7.28, so you can use the same patch on 7.28.

Yes, I can confirm that the patch works also on 7.28

OK, I read #35 again but I'm still not getting it. Can someone explain what I'm missing?

1. The bug occurs when code tries to access $options['language'][$id] but $options['language'] isn't an array.
2. #35 contains two examples of the problem, one where $id is NULL and one where $id is TRUE.
3. The approach in #2 checks is_array($options['language']) before trying to access $options['language'][$id] and therefore should catch all cases, including the two above.
4. The approach in #65 checks !empty($id) before trying to access $options['language'][$id] and therefore only catches one of those two cases (when $id is NULL). When $id evaluates to TRUE there will still be PHP notices.

So why isn't #2 the correct approach?

So, can we finally make this RTBC?

65: string-offset-cast-1824820-65-D7.patch queued for re-testing.

2: string-offset-cast-1824820-2.patch queued for re-testing.

Which patch are you saying should be RTBC?

Patch #2 should be RTBC.

Re-rolled patch #65 without extra brackets

It will be confusing for new people arriving at this thread to figure out which patch is recommended by the community now. The patch in #2? Or #65, or now #99? I understood that the consensus was building for recommending the patch in #2 because it covers the more potential instances of the problem, as explained by David_Rothstein in post #91 above. Is that right?

@pkiff, the consensus of those in the know is that #2 be applied. It still applies cleanly and the tests pass.

Patch #2 should be RTBC. Applied the patch and tested.

For me patch 2 as well as 99 work.

Patch #2 works for me too - PHP 5.4 issue
Thanks

Nice - patch#2 worked for me too. Thanks a lot!

I'm also using the patch in #2

+1 for #1824820-2: String offset cast notice in field_invoke_method_multiple()

Can we finally commit this? Please, the patch is ready for over a year now.

Patch from #2 is worked for me. Got this error when work with field collections and now it's gone!

I would also like to see this patch committed.

I was used to use patch in #65, but I switched to patch in #2 too, and it's working perfectly.

Removing unnecessary tag.

#87 says that what was done in patch #2 isn't sufficient, so it should be used patch #65, others say that patch #2 should be used... So which patch should I apply, waiting for the official commit?

Thanks.

Patch #2 is currently the recommended patch. The comments in #87 are based on a misunderstanding of how #2 addresses the root problem of the bug, and this is explained in post #91 by David_Rothstein. The person who posted #87 (heddn), subsequently posted an endorsement of the patch in #2 (see #108).

And in #100 above, I asked the same question as you, and the answer then also recommended the patch in #2.

Thanks! There was so much confusion that even reading the thread two times I was still puzzled.

I'm hopefully waiting for a quick commit!

Please note I have no opinion on which patch is correct. I was simply resetting in bulk the multitude of RTBC issues which failed last week due to a problem with Testbot.

When will this be commited?

Patch #2 seems to work.

Patch #2 work fine.

Works nicely with D7, thanks!

Patch #2 passed (again) the test, it's the recommended patch and works fine for everyone, I think that it should be RTBC (again).

Agreed. RTBC++. Patch in #2 solved my problem (getting this error on node/add/typename).

Patch in #2 works like a charm and fixed an issue I had width validation of a date field and date popup-calendar.

This is required when using Behat testing and the Drupal Extension to provide custom field values.

Committed #2 to 7.x - thanks!

And thanks for all the testing, etc., to verify that the fix is working correctly.

Hm...