- Patch #512236 by bjaspan: fixed design flaws in field_attach_query(). Well-documented and tested.

4 files changed, 92 insertions, 190 deletions

diff --git a/modules/field/field.attach.inc b/modules/field/field.attach.inc
index a7e301957..66574157a 100644
--- a/modules/field/field.attach.inc
+++ b/modules/field/field.attach.inc
@@ -778,10 +778,18 @@ function field_attach_delete_revision($obj_type, $object) {
  *     array('value', 12, '>'),
  *   );
  *   @endcode
- * @param $result_format
- *   - FIELD_QUERY_RETURN_IDS (default): return the ids of the objects matching the
- *     conditions.
- *   - FIELD_QUERY_RETURN_VALUES: return the values for the field.
+ * @param $count
+ *   The number of results that is requested. This is only a
+ *   hint to the storage engine(s); callers should be prepared to
+ *   handle fewer or more results. Specify FIELD_QUERY_NO_LIMIT to retrieve
+ *   all available objects. This parameter has no default value so
+ *   callers must make an explicit choice to potentially retrieve an
+ *   enormous result set.
+ * @param &$cursor
+ *   An opaque cursor that allows a caller to iterate through multiple
+ *   result sets. On the first call, pass 0; the correct value to pass
+ *   on the next call will be written into $cursor on return. If NULL,
+ *   the first result set is returned and no next cursor is returned.
  * @param $age
  *   Internal use only. Use field_attach_query_revisions() instead of passing
  *   FIELD_LOAD_REVISION.
@@ -791,26 +799,24 @@ function field_attach_delete_revision($obj_type, $object) {
  *     object type and object revision id.
  * @return
  *   An array keyed by object type (e.g. 'node', 'user'...), then by object id
- *   or revision id (depending of the value of the $age parameter), and whose
- *   values depend on the $result_format parameter:
- *   - FIELD_QUERY_RETURN_IDS: the object id.
- *   - FIELD_QUERY_RETURN_VALUES: a pseudo-object with values for the
- *     $field_name field. This only includes values matching the conditions,
- *     and thus might not contain all actual values and actual delta sequence
- *     (although values oprder is preserved).
- *     The pseudo-objects only include properties that the Field API knows
- *     about: bundle, id, revision id, and field values (no node title, user
- *     name...).
+ *   or revision id (depending of the value of the $age parameter).
+ *   The values are pseudo-objects with the bundle, id, and revision
+ *   id fields filled in.
+ *
  *   Throws a FieldQueryException if the field's storage doesn't support the
  *   specified conditions.
  */
-function field_attach_query($field_name, $conditions, $result_format = FIELD_QUERY_RETURN_IDS, $age = FIELD_LOAD_CURRENT) {
+function field_attach_query($field_name, $conditions, $count, &$cursor = NULL, $age = FIELD_LOAD_CURRENT) {
+  if (!isset($cursor)) {
+    $cursor = 0;
+  }
+
   // Give a chance to 3rd party modules that bypass the storage engine to
   // handle the query.
   $skip_field = FALSE;
   foreach (module_implements('field_attach_pre_query') as $module) {
     $function = $module . '_field_attach_pre_query';
-    $results = $function($field_name, $conditions, $result_format, $age, $skip_field);
+    $results = $function($field_name, $conditions, $count, $cursor, $age, $skip_field);
     // Stop as soon as a module claims it handled the query.
     if ($skip_field) {
       break;
@@ -818,43 +824,7 @@ function field_attach_query($field_name, $conditions, $result_format = FIELD_QUE
   }
   // If the request hasn't been handled, let the storage engine handle it.
   if (!$skip_field) {
-    $results = module_invoke(variable_get('field_storage_module', 'field_sql_storage'), 'field_storage_query', $field_name, $conditions, $result_format, $age);
-  }
-
-  if ($result_format == FIELD_QUERY_RETURN_VALUES) {
-    foreach ($results as $obj_type => $pseudo_objects) {
-      if ($age == FIELD_LOAD_CURRENT) {
-        // Invoke hook_field_load().
-        $b = NULL;
-        _field_invoke_multiple('load', $obj_type, $pseudo_objects, $age, $b, array('field_name' => $field_name));
-
-        // Invoke hook_field_attach_load().
-        foreach (module_implements('field_attach_load') as $module) {
-          $function = $module . '_field_attach_load';
-          $function($obj_type, $pseudo_objects, $age);
-        }
-      }
-      else {
-        // The 'multiple' hooks expect an array of objects keyed by object id,
-        // and thus cannot be used directly when querying revisions. The hooks
-        // are therefore called on each object separately, which might cause
-        // performance issues when large numbers of revisions are retrieved.
-        foreach ($pseudo_objects as $vid => $pseudo_object) {
-          list($id) = field_attach_extract_ids($obj_type, $pseudo_object);
-          $objects = array($id => $pseudo_object);
-
-          // Invoke hook_field_load().
-          $b = NULL;
-          _field_invoke_multiple('load', $obj_type, $objects, $age, $b, array('field_name' => $field_name));
-
-          // Invoke hook_field_attach_load().
-          foreach (module_implements('field_attach_load') as $module) {
-            $function = $module . '_field_attach_load';
-            $function($obj_type, $objects, $age);
-          }
-        }
-      }
-    }
+    $results = module_invoke(variable_get('field_storage_module', 'field_sql_storage'), 'field_storage_query', $field_name, $conditions, $count, $cursor, $age);
   }
 
   return $results;
@@ -869,15 +839,22 @@ function field_attach_query($field_name, $conditions, $result_format = FIELD_QUE
  *   The name of the field to query.
  * @param $conditions
  *   See field_attach_query().
- * @param $result_format
- *   See field_attach_query().
- *   Note that the FIELD_QUERY_RETURN_VALUES option might cause performance
- *   issues with field_attach_query_revisions().
+ * @param $count
+ *   The number of results that is requested. This is only a
+ *   hint to the storage engine(s); callers should be prepared to
+ *   handle fewer or more results. Specify FIELD_QUERY_NO_LIMIT to retrieve
+ *   all available objects. This parameter has no default value so
+ *   callers must make an explicit choice to potentially retrieve an
+ *   enormous result set.
+ * @param &$cursor
+ *   An opaque cursor that allows a caller to iterate through multiple
+ *   result sets. On the first call, pass 0; the correct value to pass
+ *   on the next call will be written into $cursor on return.
  * @return
  *   See field_attach_query().
  */
-function field_attach_query_revisions($field_name, $conditions, $result_format = FIELD_QUERY_RETURN_IDS) {
-  return field_attach_query($field_name, $conditions, $result_format, FIELD_LOAD_REVISION);
+function field_attach_query_revisions($field_name, $conditions, $count, &$cursor = NULL) {
+  return field_attach_query($field_name, $conditions, $count, $cursor, FIELD_LOAD_REVISION);
 }
 
 /**
diff --git a/modules/field/field.module b/modules/field/field.module
index 9f023aa6b..70ae73dde 100644
--- a/modules/field/field.module
+++ b/modules/field/field.module
@@ -90,10 +90,16 @@ define('FIELD_LOAD_REVISION', 'FIELD_LOAD_REVISION');
 /**
  * @name Field query flags
  * @{
- * Flags for use in the $result_format parameter in field_attach_query().
+ * Flags for field_attach_query().
  */
 
 /**
+ * Limit argument for field_attach_query() to request all available
+ * objects instead of a limited number.
+ */
+define('FIELD_QUERY_NO_LIMIT', 'FIELD_QUERY_NO_LIMIT');
+
+/**
  * Result format argument for field_attach_query().
  */
 define('FIELD_QUERY_RETURN_VALUES', 'FIELD_QUERY_RETURN_VALUES');
diff --git a/modules/field/field.test b/modules/field/field.test
index 57357b920..eead0f76d 100644
--- a/modules/field/field.test
+++ b/modules/field/field.test
@@ -26,7 +26,8 @@ class FieldAttachTestCase extends DrupalWebTestCase {
 
     $this->field_name = drupal_strtolower($this->randomName() . '_field_name');
     $this->field = array('field_name' => $this->field_name, 'type' => 'test_field', 'cardinality' => 4);
-    field_create_field($this->field);
+    $this->field = field_create_field($this->field);
+    $this->field_id = $this->field['id'];
     $this->instance = array(
       'field_name' => $this->field_name,
       'bundle' => 'test_bundle',
@@ -296,7 +297,7 @@ class FieldAttachTestCase extends DrupalWebTestCase {
     // Query on the object's values.
     for ($delta = 0; $delta < $cardinality; $delta++) {
       $conditions = array(array('value', $values[$delta]));
-      $result = field_attach_query($this->field_name, $conditions);
+      $result = field_attach_query($this->field_name, $conditions, FIELD_QUERY_NO_LIMIT);
       $this->assertTrue(isset($result[$entity_types[1]][1]), t('Query on value %delta returns the object', array('%delta' => $delta)));
     }
 
@@ -305,74 +306,37 @@ class FieldAttachTestCase extends DrupalWebTestCase {
       $different_value = mt_rand(1, 127);
     } while (in_array($different_value, $values));
     $conditions = array(array('value', $different_value));
-    $result = field_attach_query($this->field_name, $conditions);
+    $result = field_attach_query($this->field_name, $conditions, FIELD_QUERY_NO_LIMIT);
     $this->assertFalse(isset($result[$entity_types[1]][1]), t("Query on a value that is not in the object doesn't return the object"));
 
     // Query on the value shared by both objects, and discriminate using
     // additional conditions.
 
     $conditions = array(array('value', $common_value));
-    $result = field_attach_query($this->field_name, $conditions);
+    $result = field_attach_query($this->field_name, $conditions, FIELD_QUERY_NO_LIMIT);
     $this->assertTrue(isset($result[$entity_types[1]][1]) && isset($result[$entity_types[2]][2]), t('Query on a value common to both objects returns both objects'));
 
     $conditions = array(array('type', $entity_types[1]), array('value', $common_value));
-    $result = field_attach_query($this->field_name, $conditions);
+    $result = field_attach_query($this->field_name, $conditions, FIELD_QUERY_NO_LIMIT);
     $this->assertTrue(isset($result[$entity_types[1]][1]) && !isset($result[$entity_types[2]][2]), t("Query on a value common to both objects and a 'type' condition only returns the relevant object"));
 
     $conditions = array(array('bundle', $entities[1]->fttype), array('value', $common_value));
-    $result = field_attach_query($this->field_name, $conditions);
+    $result = field_attach_query($this->field_name, $conditions, FIELD_QUERY_NO_LIMIT);
     $this->assertTrue(isset($result[$entity_types[1]][1]) && !isset($result[$entity_types[2]][2]), t("Query on a value common to both objects and a 'bundle' condition only returns the relevant object"));
 
     $conditions = array(array('entity_id', $entities[1]->ftid), array('value', $common_value));
-    $result = field_attach_query($this->field_name, $conditions);
+    $result = field_attach_query($this->field_name, $conditions, FIELD_QUERY_NO_LIMIT);
     $this->assertTrue(isset($result[$entity_types[1]][1]) && !isset($result[$entity_types[2]][2]), t("Query on a value common to both objects and an 'entity_id' condition only returns the relevant object"));
 
     // Test FIELD_QUERY_RETURN_IDS result format.
     $conditions = array(array('value', $values[0]));
-    $result = field_attach_query($this->field_name, $conditions);
+    $result = field_attach_query($this->field_name, $conditions, FIELD_QUERY_NO_LIMIT);
     $expected = array(
       $entity_types[1] => array(
-        $entities[1]->ftid => $entities[1]->ftid,
+        $entities[1]->ftid => field_test_create_stub_entity($entities[1]->ftid, $entities[1]->ftvid),
       )
     );
     $this->assertEqual($result, $expected, t('FIELD_QUERY_RETURN_IDS result format returns the expect result'));
-
-    // Test FIELD_QUERY_RETURN_VALUES result format.
-    // Configure the instances so that we test hook_field_load() (see
-    // field_test_field_load() in field_test.module).
-    $this->instance['settings']['test_hook_field_load'] = TRUE;
-    field_update_instance($this->instance);
-    $this->instance2['settings']['test_hook_field_load'] = TRUE;
-    field_update_instance($this->instance2);
-
-    // Query for one of the values in the 1st object and the value shared by
-    // both objects.
-    $conditions = array(array('value', array($values[0], $common_value)));
-    $result = field_attach_query($this->field_name, $conditions, FIELD_QUERY_RETURN_VALUES);
-    $expected = array(
-      $entity_types[1] => array(
-        $entities[1]->ftid => (object) array(
-          'ftid' => $entities[1]->ftid,
-          'ftvid' => $entities[1]->ftvid,
-          'fttype' => $entities[1]->fttype,
-          $this->field_name => array(
-            array('value' => $values[0], 'additional_key' => 'additional_value'),
-            array('value' => $common_value, 'additional_key' => 'additional_value'),
-          ),
-        ),
-      ),
-      $entity_types[2] => array(
-        $entities[2]->ftid => (object) array(
-          'ftid' => $entities[2]->ftid,
-          'ftvid' => $entities[2]->ftvid,
-          'fttype' => $entities[2]->fttype,
-          $this->field_name => array(
-            array('value' => $common_value, 'additional_key' => 'additional_value'),
-          ),
-        ),
-      ),
-    );
-    $this->assertEqual($result, $expected, t('FIELD_QUERY_RETURN_VALUES result format returns the expect result'));
   }
 
   /**
@@ -402,7 +366,7 @@ class FieldAttachTestCase extends DrupalWebTestCase {
     // Query on the object's values.
     for ($delta = 0; $delta < $cardinality; $delta++) {
       $conditions = array(array('value', $values[$delta]));
-      $result = field_attach_query_revisions($this->field_name, $conditions);
+      $result = field_attach_query_revisions($this->field_name, $conditions, FIELD_QUERY_NO_LIMIT);
       $this->assertTrue(isset($result[$entity_type][1]), t('Query on value %delta returns the object', array('%delta' => $delta)));
     }
 
@@ -411,62 +375,29 @@ class FieldAttachTestCase extends DrupalWebTestCase {
       $different_value = mt_rand(1, 127);
     } while (in_array($different_value, $values));
     $conditions = array(array('value', $different_value));
-    $result = field_attach_query_revisions($this->field_name, $conditions);
+    $result = field_attach_query_revisions($this->field_name, $conditions, FIELD_QUERY_NO_LIMIT);
     $this->assertFalse(isset($result[$entity_type][1]), t("Query on a value that is not in the object doesn't return the object"));
 
     // Query on the value shared by both objects, and discriminate using
     // additional conditions.
 
     $conditions = array(array('value', $common_value));
-    $result = field_attach_query_revisions($this->field_name, $conditions);
+    $result = field_attach_query_revisions($this->field_name, $conditions, FIELD_QUERY_NO_LIMIT);
     $this->assertTrue(isset($result[$entity_type][1]) && isset($result[$entity_type][2]), t('Query on a value common to both objects returns both objects'));
 
     $conditions = array(array('revision_id', $entities[1]->ftvid), array('value', $common_value));
-    $result = field_attach_query_revisions($this->field_name, $conditions);
+    $result = field_attach_query_revisions($this->field_name, $conditions, FIELD_QUERY_NO_LIMIT);
     $this->assertTrue(isset($result[$entity_type][1]) && !isset($result[$entity_type][2]), t("Query on a value common to both objects and a 'revision_id' condition only returns the relevant object"));
 
     // Test FIELD_QUERY_RETURN_IDS result format.
     $conditions = array(array('value', $values[0]));
-    $result = field_attach_query_revisions($this->field_name, $conditions);
+    $result = field_attach_query_revisions($this->field_name, $conditions, FIELD_QUERY_NO_LIMIT);
     $expected = array(
       $entity_type => array(
-        $entities[1]->ftvid => $entities[1]->ftid,
+        $entities[1]->ftid => field_test_create_stub_entity($entities[1]->ftid, $entities[1]->ftvid),
       )
     );
     $this->assertEqual($result, $expected, t('FIELD_QUERY_RETURN_IDS result format returns the expect result'));
-
-    // Test FIELD_QUERY_RETURN_VALUES result format.
-    // Configure the instance so that we test hook_field_load() (see
-    // field_test_field_load() in field_test.module).
-    $this->instance['settings']['test_hook_field_load'] = TRUE;
-    field_update_instance($this->instance);
-
-    // Query for one of the values in the 1st object and the value shared by
-    // both objects.
-    $conditions = array(array('value', array($values[0], $common_value)));
-    $result = field_attach_query_revisions($this->field_name, $conditions, FIELD_QUERY_RETURN_VALUES);
-    $expected = array(
-      $entity_type => array(
-        $entities[1]->ftvid => (object) array(
-          'ftid' => $entities[1]->ftid,
-          'ftvid' => $entities[1]->ftvid,
-          'fttype' => $entities[1]->fttype,
-          $this->field_name => array(
-            array('value' => $values[0], 'additional_key' => 'additional_value'),
-            array('value' => $common_value, 'additional_key' => 'additional_value'),
-          ),
-        ),
-        $entities[2]->ftvid => (object) array(
-          'ftid' => $entities[2]->ftid,
-          'ftvid' => $entities[2]->ftvid,
-          'fttype' => $entities[2]->fttype,
-          $this->field_name => array(
-            array('value' => $common_value, 'additional_key' => 'additional_value'),
-          ),
-        ),
-      ),
-    );
-    $this->assertEqual($result, $expected, t('FIELD_QUERY_RETURN_VALUES result format returns the expect result'));
   }
 
   function testFieldAttachViewAndPreprocess() {
diff --git a/modules/field/modules/field_sql_storage/field_sql_storage.module b/modules/field/modules/field_sql_storage/field_sql_storage.module
index c9666d20f..b74bad0da 100644
--- a/modules/field/modules/field_sql_storage/field_sql_storage.module
+++ b/modules/field/modules/field_sql_storage/field_sql_storage.module
@@ -355,8 +355,7 @@ function field_sql_storage_field_storage_delete($obj_type, $object) {
 /**
  * Implement hook_field_storage_query().
  */
-function field_sql_storage_field_storage_query($field_name, $conditions, $result_format, $age) {
-  $load_values = $result_format == FIELD_QUERY_RETURN_VALUES;
+function field_sql_storage_field_storage_query($field_name, $conditions, $count, &$cursor, $age) {
   $load_current = $age == FIELD_LOAD_CURRENT;
 
   $field = field_info_field($field_name);
@@ -367,16 +366,12 @@ function field_sql_storage_field_storage_query($field_name, $conditions, $result
   $query = db_select($table, 't');
   $query->join('field_config_entity_type', 'e', 't.etid = e.etid');
   $query
+    ->fields('t', array('bundle', 'entity_id', 'revision_id'))
     ->fields('e', array('type'))
     ->condition('deleted', 0)
-    ->orderBy('delta');
-  // Add fields, depending on the return format.
-  if ($load_values) {
-    $query->fields('t');
-  }
-  else {
-    $query->fields('t', array('entity_id', 'revision_id'));
-  }
+    ->orderBy('t.etid')
+    ->orderBy('t.entity_id');
+
   // Add conditions.
   foreach ($conditions as $condition) {
     // A condition is either a (column, value, operator) triple, or a
@@ -406,45 +401,38 @@ function field_sql_storage_field_storage_query($field_name, $conditions, $result
     $query->condition($column, $value, $operator);
   }
 
-  $results = $query->execute();
-
-  // Build results.
+  // Initialize results array
   $return = array();
-  $delta_count = array();
-  foreach ($results as $row) {
-    // If querying all revisions and the entity type has revisions, we need to
-    // key the results by revision_ids.
-    $entity_type = field_info_fieldable_types($row->type);
-    $id = ($load_current || empty($entity_type['revision key'])) ? $row->entity_id : $row->revision_id;
-
-    if ($load_values) {
-      // Populate actual field values.
-      if (!isset($delta_count[$row->type][$id])) {
-        $delta_count[$row->type][$id] = 0;
-      }
-      if ($field['cardinality'] == FIELD_CARDINALITY_UNLIMITED || $delta_count[$row->type][$id] < $field['cardinality']) {
-        $item = array();
-        // For each column declared by the field, populate the item
-        // from the prefixed database column.
-        foreach ($field['columns'] as $column => $attributes) {
-          $column_name = _field_sql_storage_columnname($field_name, $column);
-          $item[$column] = $row->$column_name;
-        }
 
-        // Initialize the 'pseudo object' if needed.
-        if (!isset($return[$row->type][$id])) {
-          $return[$row->type][$id] = field_attach_create_stub_object($row->type, array($row->entity_id, $row->revision_id, $row->bundle));
-        }
-        // Add the item to the field values for the entity.
-        $return[$row->type][$id]->{$field_name}[] = $item;
-        $delta_count[$row->type][$id]++;
-      }
+  // Query for batches of rows until we've read $count objects or
+  // until we get no new rows.
+  $limit = $count;
+  do {
+    if ($limit != FIELD_QUERY_NO_LIMIT) {
+      $query->range($cursor, $limit);
     }
-    else {
-      // Simply return the list of selected ids.
-      $return[$row->type][$id] = $row->entity_id;
+    $results = $query->execute();
+    
+    $found = FALSE;
+    foreach ($results as $row) {
+      $found = TRUE;
+      ++$cursor;
+      
+      // If querying all revisions and the entity type has revisions, we need to
+      // key the results by revision_ids.
+      $entity_type = field_info_fieldable_types($row->type);
+      $id = ($load_current || empty($entity_type['revision key'])) ? $row->entity_id : $row->revision_id;
+
+      // We get multiple rows if the field has multiple deltas.  Only
+      // count the first one.
+      if (isset($return[$row->type][$id])) {
+        continue;
+      }
+      
+      $return[$row->type][$id] = field_attach_create_stub_object($row->type, array($row->entity_id, $row->revision_id, $row->bundle));
+      --$count;
     }
-  }
+  } while ($found && ($limit != FIELD_QUERY_NO_LIMIT || $count > 0));
 
   return $return;
 }