Tests for: $info['fields'], not always set?

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().

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.

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.

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.

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.

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

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 [:]

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.

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!

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.

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.

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.

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.

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.

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

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.

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 ?

Or rather, that one.

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.

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.

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 ?

#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.

#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 ?

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

Related issue: #1802074: Undefined index: fields field.info.inc:596

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).

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

+++ 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.

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

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.

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!

+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.

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

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

Thank You!

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

+  // 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!

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

Thank You!

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.

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

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

lol you can do that?

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

Awesome, thanks for sticking with this, folks!

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