array_flip() warning in entity.inc

let's wake up the testbot

This seems to make sense to me and I can at least say that it made the error I saw within another module go away. Keeping an eye on this as it seems like a very simple solution....

This is not caused by anything added in Drupal 7.37 - it's an issue caused by code that is using the entity API incorrectly (see discussion in #1102570: array_flip() [function.array-flip] issue in DrupalDefaultEntityController / entity.inc). Usually something like entity_load('node', array(NULL)) or maybe entity_load('node', 1) (rather than entity_load('node', array(1))).

The best solution is to find the offending code and fix it.

Since this indicates an actual problem with the calling code we don't necessarily want to hide the error completely... although clearly it would be better to have a more meaningful error message :)

Looking at the patch itself, a possible issue is that in moving this code up it changes the meaning of the $passed_ids variable (defined in between the old and new locations); it no longer necessarily represents the IDs that were actually passed in. Since that variable is used several other times in the function it seems like that could cause problems, but I haven't checked.

+++ b/modules/simpletest/tests/entity_crud.test
@@ -45,5 +45,13 @@ class EntityLoadTestCase extends DrupalWebTestCase {
+      $pass = ($exception->getMessage() == t('The second argument passed to entity_load must be FALSE or an array.'));

Aren't exception messages not supposed to use t()?

Well I copied from entity_query.test...

RTBC. This only adds error docs and tests cover the exception call. The exception used is appropriate as well i.e. InvalidArgumentException.

So currently (before the patch), this gives me a fatal error:

  entity_load('node', array(1));
  entity_load('node', 1);

But this by itself just gives me a PHP warning:

  entity_load('node', 1);

(The difference seems to have to do with whether the static cache is populated or not.)

So we probably don't want to throw an exception and break the whole page here, rather do something like trigger_error() (which should have a similar effect as a PHP warning, just with a more meaningful error message).

Also this patch won't do anything for the entity_load('node', array(NULL)) case (which anecdotally I think is more common?).

+  if ($ids != FALSE && !is_array($ids)) {

Should probably be !== FALSE?

+    throw new \InvalidArgumentException("The second argument passed to entity_load() must be FALSE or an array.");

Doesn't matter if we wind up removing this anyway, but I don't think the leading slash works in PHP 5.2?

+  if (is_array($ids)) {
+    foreach ($ids as $key => $id) {
+      if (!is_numeric($id)) {
+        trigger_error("If an array is passed as the second argument to entity_load(), it must contain integers only.");
+        unset($ids[$key]);
+      }

So this sort of duplicates some of what the cleanIds() method does, but does it differently... I don't think we want the error to display when a user visits node/1.string, rather only when it's a non-string and non-integer (such as NULL). And we definitely shouldn't unset() just because it's non-numeric because the goal of the original issue was to still support some entity types that have non-numeric IDs (apparently they exist out there).

It seems to me this part could all be handled by:
1. Moving the existing cleanIds() call up (as the original patch here did) but making sure to think about the $passed_ids question I mentioned above.
2. After cleanIDs runs, compare the output to the original (with array_diff()?) and if anything was removed, and if the thing that was removed is neither a string nor integer, that's when the warning should be displayed.
3. cleanIds() itself could also be updated to do some cleaning even in the case of an entity type that allows non-numeric keys (in that case it should still remove things that are neither strings nor integers). Less important in practice, but I'm mentioning it for completeness.

I'm also surprised the tests pass with the trigger_error() - maybe that's because trigger_error does a debug-level notice by default? We might want E_USER_WARNING instead here...

I'm also surprised the tests pass with the trigger_error() - maybe that's because trigger_error does a debug-level notice by default? We might want E_USER_WARNING instead here...

In that case I believe the tests would naturally fail, but there is some way to make them pass (I think it involves making the test use a different error handler that looks for the expected warnings rather than failing when warnings appear)... there is already test code for that somewhere I think, not sure where offhand.

 function entity_load($entity_type, $ids = FALSE, $conditions = array(), $reset = FALSE) {
....
+    trigger_error("The second argument passed to entity_load() must be FALSE or an array.");
....
+        trigger_error("If an array is passed as the second argument to entity_load(), it must contain integers only.");

So if we're going through the trouble of triggering custom messages would it make sense to put some more information in (e.g. about the $entity_type and $ids that were passed in)? Not sure if it's actually worth it - probably someone debugging this would need to do a backtrace anyway to figure out what the calling code was doing. So we can feel free to defer this question.

I'm not sure if anyone has looked at the issue I had reported over here: https://www.drupal.org/node/2476821 I saw that someone was saying the calling code was to blame, not core... but I don't think that is quite the case in the example I have reported. Again, limited core/entity knowledge here.. but just wanted to point it out because book module comes with core and it was part of the issue I ran into.

**EDIT** actually perhaps you are already solving for that issue as well since book is guilty of the "-1" setting.

Any update on this issue or anything I can do to help test/debug?

I tested the patch in #21 and the debugging error is not a good solution. Perhaps that was intended to be temporary, but I can't use something like that on a production server. Especially with the use case that I outlined in my comment #24.

Found another issue with patch #21, When I use that patch and I am also using the taxonomy deep index module, I am getting the following errors when I try and access the module listing page:

    PDOException: SQLSTATE[23000]: Integrity constraint violation: 1062 Duplicate entry 'taxonomy_deep_index_rebuild' for key 'name': INSERT INTO {rules_config} (name, label, plugin, weight, status, module, owner, data) VALUES (:db_insert_placeholder_0, :db_insert_placeholder_1, :db_insert_placeholder_2, :db_insert_placeholder_3, :db_insert_placeholder_4, :db_insert_placeholder_5, :db_insert_placeholder_6, :db_insert_placeholder_7); Array ( [:db_insert_placeholder_0] => taxonomy_deep_index_rebuild [:db_insert_placeholder_1] => Rebuild taxonomy deep index [:db_insert_placeholder_2] => action set [:db_insert_placeholder_3] => 0 [:db_insert_placeholder_4] => 2 [:db_insert_placeholder_5] => taxonomy_deep_index_admin [:db_insert_placeholder_6] => rules [:db_insert_placeholder_7] => O:14:"RulesActionSet":14:{s:9:"*parent";N;s:2:"id";N;s:12:"*elementId";N;s:6:"weight";i:0;s:8:"settings";a:0:{}s:4:"name";s:27:"taxonomy_deep_index_rebuild";s:5:"label";s:27:"Rebuild taxonomy deep index";s:6:"plugin";s:10:"action set";s:6:"status";i:2;s:6:"module";s:25:"taxonomy_deep_index_admin";s:5:"owner";s:5:"rules";s:4:"data";r:1;s:11:"*children";a:1:{i:0;O:11:"RulesAction":6:{s:9:"*parent";r:1;s:2:"id";N;s:12:"*elementId";N;s:6:"weight";i:0;s:8:"settings";a:2:{s:11:"node:select";s:4:"node";s:18:"#_needs_processing";b:1;}s:14:"*elementName";s:27:"taxonomy_deep_index_rebuild";}}s:7:"*info";a:1:{s:9:"variables";a:1:{s:4:"node";a:2:{s:4:"type";s:4:"node";s:5:"label";s:7:"Content";}}}} ) in drupal_write_record() (line 7316 of C:\inetpub\wwwroot\drupal_test\includes\common.inc).

Status message

    Debug: The second argument passed to entity_load() must be FALSE or an array. in entity_load() (line 7925 of C:\inetpub\wwwroot\drupal_test\includes\common.inc).

The entire module list page is broken until I roll the patch back out.

Re #24

I saw that someone was saying the calling code was to blame, not core...

This is correct. If the entity_load() function is called with invalid arguments, then that is definitely a problem in the calling code.

but I don't think that is quite the case in the example I have reported. Again, limited core/entity knowledge here.. but just wanted to point it out because book module comes with core and it was part of the issue I ran into.

This is a bit more complicated. This is not actually a bug in the book module, but in the integration code for the book module in the contrib Entity API module.

Thanks for clearing up some more details znerol!

My array_flip error can from a default image in a cck field that was missing inside the cck field after a d6 to d7 upgrade
https://www.drupal.org/node/1102570#comment-10978037

This worked for me:

open /includes/entity.inc

uncomment $passed_ids = !empty($ids) ? array_flip($ids) : FALSE; i.e. // $passed_ids = !empty($ids) ? array_flip($ids) : FALSE;

just before if ($this->cache && !$revision_id) {

comment $passed_ids = count($ids)>1 ? array_flip($ids) : FALSE;

save and clear cache via drush if you are having difficulty accessing the main site

Since I stumbled on the question with a search on google for my own problem what was causing this issue and not being able to find a solution.
I found a way to get track of this error. It is indeed a problem that is caused by the callee of the function.
I used the example of David_Rothstein to get a backtrace to through some of the custom code.
Since print_debug_backtrace gave me a memory exhaustion i'd use this:

// add this in the function load of includes/entity.inc
if (is_array($ids)) {
      foreach ($ids as $key => $id) {
      if (!is_numeric($id)) {
          throw new ErrorException('No integer used in entity_load function');
}}}


// alter the index.php like this:
<?php

/**
 * @file
 * The PHP page that serves all page requests on a Drupal installation.
 *
 * The routines here dispatch control to the appropriate handler, which then
 * prints the appropriate page.
 *
 * All Drupal code is released under the GNU General Public License.
 * See COPYRIGHT.txt and LICENSE.txt.
 */

/**
 * Root directory of Drupal installation.
 */
define('DRUPAL_ROOT', getcwd());

require_once DRUPAL_ROOT . '/includes/bootstrap.inc';

try {
    drupal_bootstrap(DRUPAL_BOOTSTRAP_FULL);
    menu_execute_active_handler();
} catch (ErrorException $e ){
    echo '<pre>';
    print_r( $e->getTraceAsString());
}

Basicly I used a try catch in the drupal init to get the actual caller of the function without memory exhaustion en threw the Error where it came from.

#32 fixed my issue, it helps me nail down where the problems come from. Thanks, @herrwalter

Hi!

In my case, the problem was the array passed via a View is Array ([] => ), so it is not empty for the empty function. I have added a check to validate that all values are set.

@dtamajon Your patch in #34 is a patch to includes/entity.inc whereas previous patches are to includes/common.inc

Can you explain if your patch is a substitute for the others or an addition?

Thanks

@izmeez, the #34 solves the problem directly in the entity.inc, without need of modifying any other file... at least when the list comes with values like Array ([] => ).

As the final load is deletaged to the includes/entity.inc file, that fix should be in there. The given solution on #34 solves the malformed array given by some modules, like Views... and for other cases, as pointed in #8, the best solution is to find the offending code and fix it.

#34 no working for me, make more errors with this patch
Warning: array_flip(): Can only flip STRING and INTEGER values! en DrupalDefaultEntityController->cacheGet() (línea 388 de /includes/entity.inc).
Warning: array_flip(): Can only flip STRING and INTEGER values! en EntityAPIController->load() (línea 219 de /home/corcolco/public_html/sites/all/modules/contrib/entity/includes/entity.controller.inc).

any solutions?

https://www.drupal.org/project/drupal/issues/1102570

   if(!empty($ids)) {
      if(count($ids)==1 and is_array($ids[0])) {
        $ids = reset($ids);
        $passed_ids = array_keys($ids);
        $ids = $passed_ids;
        $passed_ids = array_flip($passed_ids);
      }else {
        $passed_ids = array_flip($ids);
      }
    }else {
      $passed_ids = FALSE;
    }