I have mongodb set as field storage, and when I do a site install with drush I get a warning (twice): Invalid argument supplied for foreach() field.info.inc:592. I think it has to do with the Mongodb setup, so I think there should be a check if ($info['fields'] is an array.

Files: 
CommentFileSizeAuthor
#50 1400256_50.patch2.54 KBDavid_Rothstein
PASSED: [[SimpleTest]]: [MySQL] 53,453 pass(es).
[ View ]
#50 interdiff-49-50.txt731 bytesDavid_Rothstein
#49 1400256_49.patch2.51 KBmahaprasad
PASSED: [[SimpleTest]]: [MySQL] 53,142 pass(es).
[ View ]
#49 interdiff.txt2.66 KBmahaprasad
#43 1400256_43.patch2.51 KBmahaprasad
PASSED: [[SimpleTest]]: [MySQL] 53,095 pass(es).
[ View ]
#39 1400256_39.patch2.53 KBmahaprasad
FAILED: [[SimpleTest]]: [MySQL] 53,088 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#35 1400256-8.patch2.7 KBamcgowanca
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1400256-8_0.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#33 1400256-8.patch2.71 KBamcgowanca
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]
#22 field_info_collate_fields_clash-1400256-22-D7.patch1.25 KByched
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch field_info_collate_fields_clash-1400256-22-D7.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#20 field_info_collate_fields_clash-1400256-20.patch1.02 KByched
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch field_info_collate_fields_clash-1400256-20.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#13 field-info-collate-fields-1400256-13-TESTS-ONLY.patch2.46 KBDavid_Rothstein
FAILED: [[SimpleTest]]: [MySQL] 37,265 pass(es), 1 fail(s), and 2 exception(s).
[ View ]
#13 field-info-collate-fields-1400256-13.patch3.41 KBDavid_Rothstein
PASSED: [[SimpleTest]]: [MySQL] 37,266 pass(es).
[ View ]
#8 1400256-field_info_collate_fields-7.patch1002 bytespopulist
PASSED: [[SimpleTest]]: [MySQL] 39,219 pass(es).
[ View ]
#6 field_info_collate_fields_1400256-6.patch1.01 KBvlkff
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch field_info_collate_fields_1400256-6.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#1 check_if_array-1400256-1.patch676 bytesMiSc
PASSED: [[SimpleTest]]: [MySQL] 35,912 pass(es).
[ View ]

Comments

MiSc’s picture

Status:Active» Needs review
StatusFileSize
new676 bytes
PASSED: [[SimpleTest]]: [MySQL] 35,912 pass(es).
[ View ]

Here is the patch. I really do not now if this fix is needed in core, but I need to have the patch online, to get the setup with a different field storage work without warnings.

Damien Tournoud’s picture

Status:Needs review» Postponed (maintainer needs more info)

The code is correct. There is just no way for $info['fields'] not to be an array here.

The only thing that could possibly happen is that you have a broken cache implementation, and cache_get() doesn't return the data that was saved by cache_set().

MiSc’s picture

Status:Postponed (maintainer needs more info)» Needs review

I do not see how cache can have something to do with it, the warning comes when I do a site install with a clean database. If I put the array check inside field_info_fields, I get no warnings and everything works, without it I get the warnings. Must it not have to be something with the field storage or the Mongodb setup?

dude4linux’s picture

I'm getting the same error "Invalid argument supplied for foreach() field.info.inc:592" when I try a drush site-install of OpenPublish. In my case, the error occurs three times. It occurs for both drush-4.5 and drush-5.0-dev. Also I'm using mysql, not mongodb.

Jon_B’s picture

I also get this when using the field_collection module. On install, it calls the field_info_fields() function. At the point this function is invoked there are no fields, which means the for loop causes warnings.

In my case, it is nothing to do with the cache as at this point, the cache is empty.

vlkff’s picture

StatusFileSize
new1.01 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch field_info_collate_fields_1400256-6.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Actually the problem happens in _field_info_collate_fields() function, when it is calls field_info_bundles().
Is some special cases, the field_info_bundles calls _field_info_collate_fields(TRUE) (whitch means cache clearing) and here is we got a problem:
_field_info_collate_fields(TRUE) doing
$info = NULL
and as the $info is static cached variable, that action clears the previous value was set on previous recursion step, and the result of code

      // Populate 'fields' with all fields, keyed by ID.
      $info['fields'] = array();
      foreach ($definitions['field_ids'] as $key => $field) {
        $info['fields'][$key] = $definitions['field_ids'][$key] = _field_info_prepare_field($field);
      }

on prev. recursion step replacing by NULL.

The patch with a fix is attached.

Status:Needs review» Needs work

The last submitted patch, field_info_collate_fields_1400256-6.patch, failed testing.

populist’s picture

StatusFileSize
new1002 bytes
PASSED: [[SimpleTest]]: [MySQL] 39,219 pass(es).
[ View ]

I am also seeing this error as part of the site installation process. It seems odd to me as per the logic in #2 and it was only introduced with the new Features RC2, but wanted to reroll the patch for review/testing.

JvE’s picture

Version:7.8» 7.x-dev
Status:Needs work» Needs review

I'm also seeing this problem when installing with an install profile containing features containing custom entities.

JvE’s picture

Here's a trace showing the behaviour vlkff describes:

$reset == TRUE
#0 _field_info_collate_fields() called at [/sites/all/modules/contrib/entity/entity.module:1227]
#1 entity_entity_info_alter() called at [/includes/module.inc:1022]
#2 drupal_alter() called at [/includes/common.inc:7493]
#3 entity_get_info() called at [/modules/field/field.info.inc:588]
#4 field_info_bundles() called at [/modules/field/field.info.inc:234]
#5 _field_info_collate_fields() called at [/modules/field/field.info.inc:611]
#6 field_info_fields() called at [/sites/all/modules/contrib/features/includes/features.field.inc:166]
#7 field_features_rebuild() called at [:]
JvE’s picture

Status:Needs review» Reviewed & tested by the community

The _field_info_collate_fields() function has been refactored in D8 already to use a better static caching mechanism.
The patch from #8 works and I see no problems being introduced so marking RTBC.

webchick’s picture

Status:Reviewed & tested by the community» Fixed

Seems like a reasonable fix. We should probably have test coverage for this, but I can't think of how to do that with something that mainly appears at install time.

Committed and pushed to 7.x. Thanks!

David_Rothstein’s picture

Version:7.x-dev» 8.x-dev
Status:Fixed» Needs review
Issue tags:+needs backport to D7
StatusFileSize
new3.41 KB
PASSED: [[SimpleTest]]: [MySQL] 37,266 pass(es).
[ View ]
new2.46 KB
FAILED: [[SimpleTest]]: [MySQL] 37,265 pass(es), 1 fail(s), and 2 exception(s).
[ View ]

I don't see how this bug would have been fixed in Drupal 8 just because of the switch to drupal_static()?

I think the bug exists in Drupal 8 too, and here is a patch with a test to prove it. The test is a bit contrived, but basically replicates the condition that the Entity module seems to be triggering, so hopefully good enough...

So, I guess we need to fix this in Drupal 8, then backport the test to Drupal 7.

rfay’s picture

Version:8.x-dev» 7.x-dev
Priority:Normal» Critical
Status:Needs review» Needs work

This commit causes crazy memory usage during simpletest, and I would imagine at other times as well.

Discovered using git bisect while investigating #1678192: Drupal Commerce tests suddenly fail due to memory limit

So a Drupal instance that normally uses less than 30M will run over a 256M limit with this commit.

rszrama’s picture

And I just confirmed this starting from a clean Drupal 7.14 and applying this patch, trying to perform the "Order Rules" tests from the Drupal Commerce test suite (in reality just a single test) failed due to hitting my memory limit at 256M. I upped the memory limit to 512M and got the same failure.

That said, on a simple scan of the patch, I'm at a loss for how we're compounding our memory usage here. I'm not sure it's anything particular to Commerce other than we define 5 entity types with various bundles and a variety of default fields, so our tests by nature have a lot of field makin' going on in them. On regular usage of Drupal Commerce, though, I don't have any problems - just when I run tests.

David_Rothstein’s picture

Version:7.x-dev» 8.x-dev
Priority:Critical» Normal

Well, that's not good. I went ahead and rolled this back for now: http://drupalcode.org/project/drupal.git/commit/34ac2b6

So, let's start over again with the Drupal 8 patch, figure out the memory issue, and backport to Drupal 7 once that's finished.

David_Rothstein’s picture

Status:Needs work» Needs review

Actually, I guess we don't know the memory problem exists in Drupal 8 (the difference between static and drupal_static could definitely have something to do with it?), although we certainly need to check.

So, setting back to "needs review" for now.

catch’s picture

Issue tags:+Needs profiling

Adding 'needs profiling' tag, also see #1040790: _field_info_collate_fields() memory usage - _field_info_collate_fields() is already a memory hog.

populist’s picture

To provide a bit more context for developers of install profiles, this problem will appear because of a call to _field_info_collate_fields(TRUE) that resets the static $info which happens in entity_entity_info_alter() which is invoked through drupal_flush_all_caches() in the install_finished() install task triggered at the end of the profile installation process.

As far as I can tell the symptoms of this problem are just a PHP notice and a PHP warning, but since it happens at the completion of the install profile install it is a less than ideal out of the box product experience. To help address this, the patch in #8 does solve the issue but considering the performance issues mentioned in #14 + #15 and the hassle of managing a Drupal core patch I implemented this sort of crazy solution (http://drupalcode.org/project/panopoly_core.git/commitdiff/37f2760) as part of panopoly_core.module. Not recommended as a best practice, but wanted to share with others who might find this interesting/helpful.

yched’s picture

StatusFileSize
new1.02 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch field_info_collate_fields_clash-1400256-20.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

So, according to #17, next step would be to switch _field_info_collate_fields() to drupal_static() in D7, and see if the fix in #2 (rename top-level cached data to $cached_info) works without making memory consumption explodes on tests suites.

That's what the attached D7 patch does.
@rfay, @rszrama : could you guys check the commerce test suite with this ?

Status:Needs review» Needs work

The last submitted patch, field_info_collate_fields_clash-1400256-20.patch, failed testing.

yched’s picture

StatusFileSize
new1.25 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch field_info_collate_fields_clash-1400256-22-D7.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Or rather, that one.

yched’s picture

Status:Needs work» Needs review

Status:Needs review» Needs work

The last submitted patch, field_info_collate_fields_clash-1400256-22-D7.patch, failed testing.

alfaguru’s picture

StatusFileSize
new1.06 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch field_info_collate_fields-1400256-25.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

I have the same issue but have created a simpler patch for 7.x which does not require two copies of the info variable, just a flag which indicates when the structure is being rebuilt and ignores the reset in that case. This may overcome the memory issue, though it doesn't address other possible recursion horrors where one instance of this function on the stack stomps on work done by another.

alfaguru’s picture

After I posted I realised that the original patch posted above does prevent overwrite of the local copy of $info but at the expense of any number of copies on the stack, which is presumably the root cause of the simpletest issues. The amount of memory used by this structure does call the design into question anyway. Perhaps it would be better to cache this data per content / entity type and lazy-load it instead.

yched’s picture

Actually, the code refactoring in #1040790: _field_info_collate_fields() memory usage does solve the bug (tested it by running @David_Rothstein's test in #13 on top of it).
That patch is totally targeted for D7 backport, so maybe let's focus on that one ?

yched’s picture

#1305362-38: Helper issue for #1040790 has an initial attempt at a D7 backport of #1040790: _field_info_collate_fields() memory usage, testbot is chewing on it right now.

yched’s picture

#1040790-96: _field_info_collate_fields() memory usage has a D7 backport - passes tests.
@rfay, @rszrama: since it seems the commerce test suite revealed memory issues with the initial fixes in this issue, would be good to give this one a try ?

rfay’s picture

I'll try to take a look at it. It requires setting up the testbot with an unusual setup.

alippai’s picture

David_Rothstein’s picture

Title:$info['fields'], not always set?» Tests for: $info['fields'], not always set?
Category:bug» task
Issue tags:-needs backport to D7, -Needs profiling+Novice

This issue got fixed as part of #1040790: _field_info_collate_fields() memory usage for both Drupal 7 and Drupal 8, but the tests only made it into the Drupal 7 version of the patch...

So this issue still needs to get the tests committed to Drupal 8 (see comment #13 above, and see also #1040790-241: _field_info_collate_fields() memory usage which was the final patch that got committed to Drupal 7 containing the tests).

amcgowanca’s picture

Status:Needs work» Needs review
StatusFileSize
new2.71 KB
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]

@David_Rothstein: Attached is a patch for d8.x to include specific tests that were missing from the d7.

amateescu’s picture

Status:Needs review» Needs work
+++ b/core/modules/field/lib/Drupal/field/Tests/FieldInfoTest.php
@@ -329,6 +328,31 @@ function testSettingsInfo() {
   }
+  ¶
+  /**
+   * Tests that the field info cache can be built correctly.
+   */
...
+    $this->assertTrue(isset($fields[$field_name]), 'The test field is initially found in the array returned by field_info_fields().');
+    ¶
+    // Now rebuild the field info cache, and set a variable which will cause

Please remove the whitespace in these two places.

amcgowanca’s picture

StatusFileSize
new2.7 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1400256-8_0.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

@amateescu: Thanks, the attached revision has the whitespace removed. My apologies.

amcgowanca’s picture

Status:Needs work» Needs review

Status:Needs review» Needs work

The last submitted patch, 1400256-8.patch, failed testing.

disasm’s picture

Great work on getting the ball rolling here amcgowanca. Here's a code review. Also, it looks like your patch is failing because it needs rerolled.

+++ b/core/modules/field/lib/Drupal/field/Tests/FieldInfoTest.phpundefined
@@ -329,6 +328,31 @@ function testSettingsInfo() {
+  ¶

remove trailing whitespace

+++ b/core/modules/field/lib/Drupal/field/Tests/FieldInfoTest.phpundefined
@@ -329,6 +328,31 @@ function testSettingsInfo() {
+    variable_set('field_test_clear_info_cache_in_hook_entity_info', TRUE);

We don't want to introduce new variable_set here. Please use CMI.

+++ b/core/modules/field/tests/modules/field_test/field_test.entity.incundefined
@@ -9,6 +9,17 @@
+  // If requested, clear the field cache while this is being called. See
+  // testFieldInfoCache().

Rather than referencing in the comemnt, add an @see block to the doc tag.

+++ b/core/modules/field/tests/modules/field_test/field_test.entity.incundefined
@@ -9,6 +9,17 @@
+  if (variable_get('field_test_clear_info_cache_in_hook_entity_info', FALSE)) {

variable_get should be CMI as well.

mahaprasad’s picture

Status:Needs work» Needs review
StatusFileSize
new2.53 KB
FAILED: [[SimpleTest]]: [MySQL] 53,088 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

Re-rolled patch & fixed #38

For variable_set() & variable_get () I have followed as per patch @ http://drupal.org/node/1843476

Please review attached patch.

Thank You!

Status:Needs review» Needs work

The last submitted patch, 1400256_39.patch, failed testing.

David_Rothstein’s picture

+function field_test_entity_info() {
+  /**
+   * If requested, clear the field cache while this is being called.
+   * @see testFieldInfoCache().
+   */
+  if (state()->get('field_test.clear_info_cache_in_hook_entity_info')) {

You had this right the first time :) For inline comments, use "See" rather than "@see", and "//" rather than "/**" for the comment syntax.

The test failure looks unrelated (and hopefully a random failure?), so my guess is if you upload a new patch it will be OK.

mahaprasad’s picture

@David_Rothstein thank you for review comments. I will do changes & upload new patch.

mahaprasad’s picture

StatusFileSize
new2.51 KB
PASSED: [[SimpleTest]]: [MySQL] 53,095 pass(es).
[ View ]

Changes done as per code review comment #41. Please review attached new patch.

Thank You!

mahaprasad’s picture

Status:Needs work» Needs review

Status:Needs review» Needs work
Issue tags:-Novice

The last submitted patch, 1400256_43.patch, failed testing.

Cottser’s picture

Status:Needs work» Needs review
Issue tags:+Novice

#43: 1400256_43.patch queued for re-testing.

David_Rothstein’s picture

+  // If requested, clear the field cache while this is being called.
+  // see testFieldInfoCache().

"see" should be "See" (and also on the previous line, since it fits within 80 characters). Otherwise this looks RTBC to me!

webchick’s picture

Status:Needs review» Needs work
mahaprasad’s picture

Status:Needs work» Needs review
StatusFileSize
new2.66 KB
new2.51 KB
PASSED: [[SimpleTest]]: [MySQL] 53,142 pass(es).
[ View ]

Changes done as per comment #47. Please review attached patch.

Thank You!

David_Rothstein’s picture

Status:Needs review» Reviewed & tested by the community
StatusFileSize
new731 bytes
new2.54 KB
PASSED: [[SimpleTest]]: [MySQL] 53,453 pass(es).
[ View ]

It still needs to be moved to the previous line, but I've done that in the attached patch.

It also occurred to me that since in Drupal 8 it's no longer obvious which file the testFieldInfoCache() method would be in, we should refer to the class specifically in the code comment.

Since the attached just contains those small code comment changes compared to the previous patch, I'm marking it RTBC.

Status:Reviewed & tested by the community» Needs work
Issue tags:-Novice

The last submitted patch, 1400256_50.patch, failed testing.

David_Rothstein’s picture

Status:Needs work» Needs review
Issue tags:+Novice

#50: 1400256_50.patch queued for re-testing.

David_Rothstein’s picture

Status:Needs review» Reviewed & tested by the community

I don't believe that test failure, so moving back to RTBC.

diwant’s picture

lol you can do that?

xjm’s picture

#50: 1400256_50.patch queued for re-testing.

xjm’s picture

Issue tags:+Quick fix
webchick’s picture

Status:Reviewed & tested by the community» Fixed

Awesome, thanks for sticking with this, folks!

Committed and pushed the tests to 8.x. I think we're done here now. :)

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