EntityFieldQuery v2

Here's the latest patch provided by chx (uploading it myself because drupal.org seems to hate him today).

Planning to review and write tests.

Edit from chx: I know this does not pass all tests but I think it passes most. Field info tests are failing for no apparent reason. This needs lots of doxygen and through testing the new classes and interfaces but the existing tests do a lot of that of verification already.

Hope to dive into this soon :)

chx asked me to review the sorting implementation explained in #18 for multi-value fields. Once the reverse sort order change I requested (that it use MAX versus MIN) is in place, I think this closely approximates user expectations. It's ugly but correct.

+++ b/core/modules/field/field.moduleundefined
@@ -1045,16 +1045,27 @@ function field_get_items($entity_type, $entity, $field_name, $langcode = NULL) {
-  $query = new EntityFieldQuery();
-  return (bool) $query
-    ->fieldCondition($field)
-    ->range(0, 1)
-    ->count()
-    // Neutralize the 'entity_field_access' query tag added by
-    // field_sql_storage_field_storage_query(). The result cannot depend on the
-    // access grants of the current user.
-    ->addTag('DANGEROUS_ACCESS_CHECK_OPT_OUT')
-    ->execute();
+  $field = field_info_field_by_id($field['id']);
+  $columns = array_keys($field['columns']);
+  $factory = drupal_container()->get('entity.query');
+  foreach ($field['bundles'] as $entity_type => $bundle) {
+    $query = $factory->get($entity_type);
+    $group = $query->orConditionGroup();
+    foreach ($columns as $column) {
+      $group->exists($field['field_name'] .'.' . $column);
+    }
+    $result = $query
+      ->condition($group)
+      ->count()
+      ->skipAccessCheck()
+      ->range(0, 1)
+      ->execute();
+    if ($result) {
+      return TRUE;
+    }
+  }
+

This implementation needs a follow-up and should be more lightweight. could be done after Dec1

From IRC

[07:56]	andypost: chx: I think this query should be done in storage controller
[07:57]	andypost: and field_has_data should be just a wrapper
[07:57]	chx: the problem is _options_values_in_use
[07:57]	chx: i ended up having two parallel kinds of queries , one the EntityQuery in the patch and a FieldCountQuery just to faciliate _options_values_in_use and field_has_data
[07:58]	andypost: oh, another query to storage backend
[07:58]	chx: are we going to write a separate method for the storage backend for every such function?
[07:58]	chx: tht'd be ridiculous
[07:59]	chx: add a mini query builder to storage engine just for these? very strongly MEH.
[07:59]	chx: if contrib uses this function, it does it wrong
[07:59]	chx: plain and simple
[07:59]	chx: there is no valid use for calling field_has_data unless you are about to update it.
[07:59]	chx: i might _ it just to avoid this debate
[08:00]	andypost: chx but current implementation is really scare
[08:01]	chx shrugs
[08:01]	chx: it works.

fixed a little

+++ b/core/includes/entity.api.phpundefined
@@ -368,8 +357,8 @@ function hook_entity_delete(Drupal\Core\Entity\EntityInterface $entity) {
+function hook_entity_query_alter(Drupal\Core\Entity\Query\EntityQueryInterface $query) {
+
 }

needs @todo code example

+++ b/core/lib/Drupal/Core/CoreBundle.phpundefined
@@ -66,6 +66,9 @@ public function build(ContainerBuilder $container) {
+    $container->register('entity.query', 'Drupal\Core\Entity\Query\QueryFactory')
+      ->addArgument(new Reference('service_container'));

Core bundle? What for? Is database initialized?

+++ b/core/lib/Drupal/Core/Database/Connection.phpundefined
@@ -631,7 +631,7 @@ public function getDriverClass($class) {
-   * @return Drupal\Core\Database\Query\SelectInterface
+   * @return \Drupal\Core\Database\Query\SelectInterface

+++ b/core/lib/Drupal/Core/Entity/Query/QueryFactory.phpundefined
@@ -0,0 +1,40 @@
+   * var \Symfony\Component\DependencyInjection\ContainerInterface
...
+   * @param \Symfony\Component\DependencyInjection\ContainerInterface $container

+++ b/core/lib/Drupal/Core/Entity/Query/QueryInterface.phpundefined
@@ -0,0 +1,181 @@
+   * @return \Drupal\Core\Entity\Query\QueryInterface
...
+   * @return \Drupal\Core\Entity\Query\QueryInterface
...
+   * @return \Drupal\Core\Entity\Query\QueryInterface

please revert

+++ b/core/lib/Drupal/Core/Entity/DatabaseStorageController.phpundefined
@@ -264,30 +265,24 @@ public function deleteRevision($revision_id) {
-   * @param Drupal\Core\Entity\EntityFieldQuery $entity_query
-   *   EntityFieldQuery instance.
+   * @param \Drupal\Core\Entity\Query\EntityQueryInterface $entity_query

Forward slash

+++ b/core/lib/Drupal/Core/Entity/EntityFieldQuery.phpundefined
@@ -39,7 +39,7 @@
-class EntityFieldQuery {
+class xEntityFieldQuery {

Filename is different from class inside? Maybe better delete this?

+++ b/core/lib/Drupal/Core/Entity/Query/QueryBase.phpundefined
@@ -0,0 +1,161 @@
+  protected $age = FIELD_LOAD_CURRENT;

is this constant accessible here?

+++ b/core/modules/field/field.moduleundefined
@@ -1308,4 +1316,13 @@ function _field_create_entity_from_ids($ids) {
+function field_reserved_columns() {
+  return array('deleted', 'langcode', 'delta');

only 3?

+++ b/core/modules/field/lib/Drupal/field/Tests/BulkDeleteTest.phpundefined
@@ -115,7 +113,7 @@ function setUp() {
-    $id = 0;
+    $id = 1;

how it works before?

+++ b/core/modules/field/modules/field_sql_storage/lib/Drupal/field_sql_storage/Entity/Query.phpundefined
@@ -0,0 +1,130 @@
+    }    ¶

trailing whitespace

+++ b/core/modules/system/lib/Drupal/system/Tests/Entity/EntityFieldQueryTest.phpundefined
@@ -25,7 +25,7 @@ class EntityFieldQueryTest extends WebTestBase {
-  public static function getInfo() {
+  public static function xgetInfo() {

commented test?

+++ b/core/modules/system/lib/Drupal/system/Tests/Entity/EntityTranslationTest.phpundefined
index 369b204..0000000
--- a/core/modules/system/tests/modules/entity_query_access_test/entity_query_access_test.info

--- a/core/modules/system/tests/modules/entity_query_access_test/entity_query_access_test.info
+++ /dev/nullundefined

Please mention about why this not needed any more

+++ b/core/modules/user/user.moduleundefined
@@ -290,7 +290,7 @@ function user_external_load($authname) {
- * @see Drupal\Core\Entity\EntityFieldQuery
+ * @see Drupal\Core\Entity\Query\EntitydQueryInterface

typo

Marked #1179180: Custom pager for an EntityFieldQuery ? as closed duplicate.

From a quick overview of the patch:

<?php

/**
 * @file
 * Definition of Drupal\Core\Entity\QueryException.
 */

namespace Drupal\Core\Entity\Query;

/**
 * Exception thrown by Query() on unsupported query syntax.
 *
 * Some storage modules might not support the full range of the syntax for
 * conditions, and will raise a QueryException when an unsupported
 * condition was specified.
 */
class QueryException extends \Exception { }
?>

On Best practices for exception handling it is not recommended to create new custom exceptions if they do not have useful information for client code.

It is not giving any useful information to the client code, other than an indicative exception name. Do not forget that PHP Exception classes are like other classes, wherein you can add methods that you think the client code will invoke to get more information.

It is worth repeating that checked exceptions are to be used in situations where the client API can take some productive action based on the information in the exception. Prefer unchecked exceptions for all programmatic errors. They make your code more readable.

On your tests methods you are converting testOr, and testAndWhere to testor and testAndwhere. Why removing the camel case ?

Here's my rough visual review. Just found some nit-picky stuff, I hope I'll find the time for a functional review over the weekend.

+++ b/core/lib/Drupal/Core/Config/Entity/ConfigStorageController.php
@@ -374,4 +374,11 @@ protected function invokeHook($hook, EntityInterface $entity) {
+  /**
+   * Implements Drupal\Core\Entity\EntityStorageControllerInterface::
+   */

Shouln't there be something after ::?

+++ b/core/lib/Drupal/Core/Entity/DatabaseStorageController.php
@@ -264,30 +265,24 @@ public function deleteRevision($revision_id) {
+   * @param \Drupal\Core\Entity\Query\EntityQueryInterface $entity_query
+   *   EntityQuery instance.
    * @param array $values
    *   An associative array of properties of the entity, where the keys are the
    *   property names and the values are the values those properties must have.
    */
-  protected function buildPropertyQuery(EntityFieldQuery $entity_query, array $values) {
+  protected function buildPropertyQuery(QueryInterface $entity_query, array $values) {

Param type don't seem to match, is this intended?

+++ b/core/lib/Drupal/Core/Entity/Query/ConditionBase.php
@@ -0,0 +1,69 @@
+abstract class ConditionBase implements ConditionInterface {

+++ b/core/lib/Drupal/Core/Entity/Query/ConditionInterface.php
@@ -0,0 +1,65 @@
+/**
+ *
+ */
+interface ConditionInterface {

+++ b/core/lib/Drupal/Core/Entity/Query/QueryFactory.php
@@ -0,0 +1,40 @@
+/**
+ *
+ */
+class QueryFactory {

+++ b/core/modules/field/modules/field_sql_storage/lib/Drupal/field_sql_storage/Entity/Query.php
@@ -0,0 +1,158 @@
+class Query extends QueryBase {

+++ b/core/modules/field/modules/field_sql_storage/lib/Drupal/field_sql_storage/Entity/QueryFactory.php
@@ -0,0 +1,21 @@
+class QueryFactory {

+++ b/core/modules/field/modules/field_sql_storage/lib/Drupal/field_sql_storage/Entity/Tables.php
@@ -0,0 +1,125 @@
+class Tables {

I think these classes would profit from a small docblock which explains what they are for.

+++ b/core/modules/field/field.module
@@ -1045,16 +1045,27 @@ function field_get_items($entity_type, $entity, $field_name, $langcode = NULL) {
+      $group->exists($field['field_name'] .'.' . $column);

Missing space in .'

+++ b/core/modules/field/lib/Drupal/field/Tests/BulkDeleteTest.php
@@ -176,27 +174,30 @@ function testDeleteFieldInstance() {
+    $entities = array();;

Superfluous ;

+++ b/core/modules/field/modules/field_sql_storage/lib/Drupal/field_sql_storage/Entity/Condition.php
@@ -0,0 +1,76 @@
+    // If this is not the top level condition group then the sql query is
+    // added to the $conditionContainer object by this function itself. The SQL query
+    // object is only necessary to pass to Query::addField() so that it can
+    //  join tables as necessary. conditions need to be added to the $conditionContainer
+    //  object to keep grouping.

Looks like the comments are a bit long and the last two lines seem to have a wrong indent.

+++ b/core/modules/field/modules/field_sql_storage/lib/Drupal/field_sql_storage/Entity/Condition.php
@@ -0,0 +1,76 @@
+        break;
+      case 'CONTAINS':
...
+        break;
+      case 'ENDS_WITH':
...
+        break;

I think according to the current coding standards a break;</coed> in a <code>switch should be followed by a blank line.

+++ b/core/modules/field/modules/field_sql_storage/lib/Drupal/field_sql_storage/Entity/QueryFactory.php
@@ -0,0 +1,21 @@
+ * Definition of Drupal\field_sql_storage\EntityQueryFactory.

I think this should be Definition of Drupal\field_sql_storage\Entity\QueryFactory.

+++ b/core/modules/field/modules/field_sql_storage/lib/Drupal/field_sql_storage/Entity/Tables.php
@@ -0,0 +1,125 @@
+/**
+ */

How about

/**
 * @file
 * Definition of Drupal\field_sql_storage\Entity\Tables.
 */

+++ b/core/modules/field/modules/field_sql_storage/lib/Drupal/field_sql_storage/FieldSqlStorageBundle.php
@@ -0,0 +1,17 @@
+namespace Drupal\field_sql_storage;

How about

/**
 * @file
 * Definition of Drupal\field_sql_storage\FieldSqlStorageBundle.
 */

+++ b/core/modules/field/modules/options/options.module
@@ -384,12 +384,19 @@ function options_field_update_forbid($field, $prior_field, $has_data) {
+        ->condition($field['field_name'] .'.value', $values)

Missing space in .'

+++ b/core/modules/file/file.field.inc
@@ -930,6 +930,30 @@ function file_field_formatter_view($entity_type, $entity, $field, $instance, $la
+ */
+
+function file_field_find_file_reference_column($field) {

Superfluous line break?

+++ b/core/modules/system/lib/Drupal/system/Tests/Entity/EntityQueryTest.php
@@ -0,0 +1,423 @@
+    // When querying current revisions, this string

Looks like an unfinished sentence.

So, finally had a first look at this one. Dropping my thoughts:

For further simplifying this API, we can merge Property and Field into one condition (or sort) method with a simple syntax: ->condition('body.format', 2) ie. field name, dot and column name.

+1000.

With the new Entity Field API, we also have knowledge about the data we query. For example, we could use that to automatically query for the 'text.value' if 'text' is given, or support passing in an entity when querying for entity references. That kind of stuff probably deserves its own issue though.

Reading the example query, I must say I have a hard time understanding it. I think it queries for an entity while looking for an entity in a special language and having a certain field translation in another language. Still, I'm confused of having ->condition('langcode', $langcode) as well as ->condition("$this->field_name.langcode", $langcode); in the translation part. Why do we need both??

I'd have seen "language" as a special, optional parameter when querying for any value, while the property/field 'langcode' is a regular property/field we use always when querying for the entity's (default/source) language. Such that you could simply do

$query
->condition("$this->field_name.value", $field_value, '=', 'en')
->condition("$this->field_name.value", $field_value, '=', 'de')

?

$query = drupal_container()->get('entity.query')->get('entity_test');

Could we get a small entity_query($entity_type) helper function here? That would be useful imo.

+  /**
+   * @return \Drupal\Core\Entity\Query\QueryInterface
+   *   The called object.
+   */
+  public function skipAccessCheck();

We should move to an opt-in approach for access checking IMO. Having queries doing access checks by default is major DX WTF to me. I guess this deserves its own issue though?

+  public function getQueryServiceName() {
+    return 'entity.query.field_sql_storage';
+  }

Maybe this could even directly live in the DIC. Afaik it is able to spawn a new instance of a service each time it's used, so it could already serve as factory and use whatever has been registed there, e.g. at entity.query.ENTITY_TYPE ? Not sure about that though.

Talk to Crell, it's not me whom you need to convince. In general wrapping the DIC in functions is perhaps not desired.

I think that's already common, see state() or typed_data().

Overally, I like what I see :-) Here a review:

+++ b/core/lib/Drupal/Core/Entity/EntityStorageControllerInterface.php
@@ -79,7 +79,7 @@ public function deleteRevision($revision_id);
-  public function loadByProperties(array $values);
+  public function loadByProperties(array $values = array());

Unrelated, but this default value does not make much sense. Imho it's the implementations that does it wrong.

+++ b/core/lib/Drupal/Core/Entity/Query/ConditionBase.php
@@ -0,0 +1,70 @@
+abstract class ConditionBase implements ConditionInterface {

Misses docs. And is Condition the right term for it? It seems to contain multiple conditions.

+++ b/core/lib/Drupal/Core/Entity/Query/ConditionBase.php
@@ -0,0 +1,70 @@
+  public function getConjunction() {
+    return $this->conjunction;
+  }

Quite some methods misses docs there.

+++ b/core/lib/Drupal/Core/Entity/Query/ConditionInterface.php
@@ -0,0 +1,67 @@
+ * Defines the entity query condition interface.
+ */
+interface ConditionInterface {

Again, is it a condition? From the itnerface, I see it allows me to add multiple conditions and has a conjunction. So it's a condition group?

+++ b/core/lib/Drupal/Core/Entity/Query/QueryBase.php
@@ -0,0 +1,233 @@
+    $this->condition = $this->conditionGroupFactory($conjunction);

As this is a group, the variable should be called conditionGroup.

+++ b/core/lib/Drupal/Core/Entity/Query/QueryBase.php
@@ -0,0 +1,233 @@
+  public function skipAccessCheck() {
+    $this->accessCheck = FALSE;
+    return $this;

I should be able to renable it, e.g. have a boolean parameter.

+++ b/core/lib/Drupal/Core/Entity/Query/QueryInterface.php
@@ -0,0 +1,240 @@
+   * @param $langcode
+   *   Language code. Optional.

Let's use LANGUAGE_DEFAULT as default here? Also, docs should use the regular (optional) notation.

+++ b/core/modules/field/modules/options/options.module
@@ -384,12 +384,19 @@ function options_field_update_forbid($field, $prior_field, $has_data) {
+    $field = field_info_field_by_id($field['id']);
+    $factory = drupal_container()->get('entity.query');
+    foreach ($field['bundles'] as $entity_type => $bundle) {
+      $result = $factory->get($entity_type)

This can be simplified with entity_query(). :-)

+++ b/core/modules/system/lib/Drupal/system/Tests/Entity/EntityQueryTest.php
@@ -0,0 +1,433 @@
+    $GLOBALS['debug'] = TRUE;
+    unset($GLOBALS['debug']);

Looks like the patch does a lot related to file references. Why that?

+    $configurable_fields = array();
+    foreach (field_info_instances($this->entityType) as $instances) {
+      foreach ($instances as $field_name => $instance) {
+        $configurable_fields[$field_name] = TRUE;
+      }
+    }

For this kind of stuff, please use the new field_info_field_map() instead. #1040790: _field_info_collate_fields() memory usage optimized field_info_instances($entity_type, $bundle) (with both params), but the other forms (no params or only $entity_type) are still extra costly and should be avoided. I'll probably open an issue to remove them altogether when I get back to my coding env in november.

This patch is great! It looks damn close to what we need and what we want. Attached you can find an interdiff with some minor stuff you may wish to merge into the sandbox (I couldn't find it).

+++ b/core/lib/Drupal/Core/Entity/Query/QueryBase.php
@@ -0,0 +1,233 @@
+  public function setAccessCheck($access_check = TRUE) {

+++ b/core/lib/Drupal/Core/Entity/Query/QueryInterface.php
@@ -0,0 +1,240 @@
+  public function setAccessCheck($access_check = TRUE);

No setter is using the set prefix here, except this one. For consistency with rest this should be named accessCheck().

+++ b/core/lib/Drupal/Core/Entity/Query/QueryBase.php
@@ -0,0 +1,233 @@
+      $page = pager_find_page($this->pager['element']);
...
+      pager_default_initialize($this->pager['total'], $this->pager['limit'], $this->pager['element']);

Sooner or later it would be nice to able to avoid accessing global functions here. Ideally we should exploit an injected OO pager implementation.

+++ b/core/lib/Drupal/Core/Entity/Query/QueryBase.php
@@ -0,0 +1,233 @@
+    $order = tablesort_get_order($headers);
+    $direction = tablesort_get_sort($headers);

Same as a pager. I'm not even sure table sort should be part of the base class and not being a decorator or stuff like that instead.

+++ b/core/modules/field/field.module
@@ -1308,4 +1316,13 @@ function _field_create_entity_from_ids($ids) {
+  return array('deleted');

Sorry, I missed the iteration where langcode stopped to be a reserved column: it's part of the PK of the default field SQL storage schema, how can it be non-reserved?

+++ b/core/modules/system/lib/Drupal/system/Tests/Entity/EntityTranslationTest.php
@@ -271,35 +269,34 @@ function testMultilingualProperties() {
+    $query = entity_query('entity_test');
+    $group = $query->andConditionGroup()
+      ->condition('user_id', $properties[$default_langcode]['user_id'], '=', $default_langcode)
+      ->condition('name', $properties[$default_langcode]['name'], '=', $default_langcode);
+    $result = $query
+      ->condition($group)
+      ->condition('name', $properties[$langcode]['name'], '=', $langcode)
+      ->execute();

Lovely

+++ b/core/modules/system/lib/Drupal/system/Tests/Entity/EntityTranslationTest.php
@@ -271,35 +269,34 @@ function testMultilingualProperties() {
+    $query = entity_query('entity_test');
+    $default_langcode_group = $query->andConditionGroup()
+      ->condition('user_id', $properties[$default_langcode]['user_id'], '=', $default_langcode)
+      ->condition('name', $properties[$default_langcode]['name'], '=', $default_langcode);
+    $langcode_group = $query->andConditionGroup()
+      ->condition('name', $properties[$langcode]['name'], '=', $langcode)
+      ->condition("$this->field_name.value", $field_value, '=', $langcode);
+    $result = $query
+      ->condition('langcode', $default_langcode)
+      ->condition($default_langcode_group)
+      ->condition($langcode_group)
+      ->execute();

Even more lovely :)

+++ b/core/modules/system/lib/Drupal/system/Tests/Entity/EntityTranslationTest.php
@@ -271,35 +269,34 @@ function testMultilingualProperties() {
-    $query->propertyOrderBy('name', 'ASC', 'original');
...
-    $query->propertyOrderBy('name', 'ASC', 'original');

The sort clause was important in the old code: it allowed to test proper language grouping when sorting on multilingual properties. Isn't something similar needed here?

Related: #1798140: Agree upon terminology for configurable fields.

A review from my side is long overdue. I've been discussing this patch with chx since before a single line was written, and it has morphed into something amazing.

I've already reviewed the code documentation, and helped improve it on IRC, so I won't nitpick it (and I don't consider it critical on work of this size and importance).

In general, I believe it is a huge step forward and offer a +1 an RTBC any time of day.
It can be improved and build upon, but it already offers big benefits, the API is just miles ahead and syncs EFQ with the recent core advances (Property API, translatable properties, etc)

Quick recap:
1) Each query is now per-entity-type, which is a major part of the work needed to shift from Field Storage to Entity Storage (#1497374: Switch from Field-based storage to Entity-based storage), which is a desired cleanup with broad support. This allows for a much more sensible API, and allows us to treat properties and fields the same way.

2) Cleaner code, since the query is now built only in one place. Old EFQ had one query builder for "property queries" and another for "field queries" (queries that contain at least one field). Duplicate code paths--.

3) Unified condition API (->condition() instead of ->entityCondition(), ->fieldCondition(), propertyCondition()).
Condition groups, OR support. One of the biggest EFQ feature requests I've seen.
The new API is also very DBTNG like, which gives a better DX, and allows further unification of condition interfaces & classes across the board.

It also removes the delta and langcode group numbers and other related hacks which were becoming a huge DX WTF.

Plus, I'll never again have to see people on StackExchange/forums/IRC wondering why ->fieldCondition('nid', 20) doesn't work (this was really common)

Thoughts:
- It's great that ->condition($field_name) assumes the "value" column. We might want to extend this to all single column fields, so when
entityreference lands and gives us "target_id" columns, I can still just do ->condition('related_node', 10);
(EDIT: Or maybe this is too much magic? DamZ says "eeewww". I'm not married to the idea.). Also ER might not stay a single column field for too long (we need to store the revision id / uuid as well) so it might not be the best example

- I too dislike the pager and tablesort code in the base class. However, the rewrite was not supposed to remove any features, so we should attack that
in a followup. I would personally remove the tablesort completely, and rewrite the pager code so that it's pluggable & injectable.

Please look at the functions we use for pager and for tablesort. They are not injectable, they are not SQL bound. They are mere data-shuffling simple functions. It is not impossible to move those into classes, sure but that's far out of scope.

Sure, I was noting that because it's the only part of the patch I don't really like, but this should totally be a separate issue. Sorry for not making it clear.
As @Bojanz said, I think this is RTBC or very close to it. I'll have another look to this tomorrow. But it won't probably be necessary :)

+++ b/core/lib/Drupal/Core/Entity/Query/ConditionInterface.phpundefined
@@ -0,0 +1,85 @@
+  public function __construct($conjunction = 'AND');

Can we remove the constructor from the interface? Dries just recently committed a patch which did away with those in various places.

The feedback so far has been addressed, plach and fago seem satisfied. Let's move forward.

I guess @chx owe us ;) the complete patch nuking the old EFQ before actual RTBC.

Edit: As per #12.

Hopefully I'll be able to perform another review tonight but I don't want to unnecessarily block this one on my personal agenda. I fully trust @bojanz's feedback.

I had a first proper look through this. Overall I can't find anything much to complain about, looks like a really, really great job. There's several nitpicks here, none of them really block a commit (well maybe the test hack does). I'm not awake enough to try to do a meaningful review of the field_sql_storage implementation of this, there's a lot of complexity in there but most of it looks (unfortunately) justified due to the complexity of our current entity storage.

Leaving RTBC but I don't plan to commit for another day or so to leave some time for additional reviews. If someone fancies tackling the nitpicks before then, great, but this is a major step towards entity-centric storage, as well as potentially efq_views in core and similar, so would like to see it go in sooner rather than later as well.

+++ b/core/lib/Drupal/Core/Config/Entity/ConfigStorageController.phpundefined
@@ -374,4 +374,11 @@ protected function invokeHook($hook, EntityInterface $entity) {
+  /**
+   * Implements Drupal\Core\Entity\EntityStorageControllerInterface::getQueryServicename().
+   */
+  public function getQueryServicename() {
+    throw new \LogicException('Querying configuration entities is not supported.');

I still think we should separate ConfigEntity and ContentEntity more (separate info hooks, possibly separate interface) - this is a method that just shouldn't be on the ConfigEntity interface at all no? Not introduced by this patch at all regardless.

+++ b/core/lib/Drupal/Core/Entity/Query/ConditionInterface.phpundefined
@@ -0,0 +1,80 @@
+  /**
+   * Implements Countable::count().
+   *
+   * Returns the size of this conditional. The size of the conditional is the
+   * size of its conditional array minus one, because one element is the the
+   * conjunction.
+   */

Why do we need to count these?

+++ b/core/lib/Drupal/Core/Entity/Query/QueryBase.phpundefined
@@ -0,0 +1,236 @@
+
+  /**
+   * Flag indicating whether to query the current revision or all revisions.
+   *
+   * Can be either FIELD_LOAD_CURRENT or FIELD_LOAD_REVISION.
+   *
+   * @var string
+   */
+  protected $age = FIELD_LOAD_CURRENT;

Not sure we should re-use these constants here. It breaks encapsulation of the class, we're querying rather than loading, and this eventually won't only apply to fields anyway.

+++ b/core/lib/Drupal/Core/Entity/Query/QueryBase.phpundefined
@@ -0,0 +1,236 @@
+  /**
+   * Constructs this object.
+   */
+  public function __construct($entity_type, $conjunction) {
+    $this->entityType = $entity_type;
+    $this->conjunction = $conjunction;

I was a bit confused why $this->condition isn't documented as a property. Additionally why conjunction is passed into the constructor at all. But this looks like just convenience to avoid having to specify AND everywhere, and you can still add OR, could possibly use more docs though but not enough to hold the patch up.

+++ b/core/lib/Drupal/Core/Entity/Query/QueryBase.phpundefined
@@ -0,0 +1,236 @@
+  /**
+   * Implements Drupal\Core\Entity\Query\QueryInterface::accessCheck().
+   */
+  public function accessCheck($access_check = TRUE) {
+    $this->accessCheck = $access_check;
+    return $this;

I can see people getting caught out by accessCheck() defaulting to TRUE, since it doesn't for normal queries where you have to add a tag. Why reverse the assumption we have elsewhere? Running a n access checked query when you're expecting one that's not can result in data loss/inconsistency (i.e. if you only bulk update a fraction of the nodes you meant to), so it's a case of balancing the potential for that vs. the potential for access bypass.

+++ b/core/lib/Drupal/Core/Entity/Query/QueryInterface.phpundefined
@@ -0,0 +1,243 @@
+   * @TODO: Once revision tables have been cleaned up, revisit this.

Yep, too messy to do this reliably now: #1082292: Clean up/refactor revisions

+++ b/core/lib/Drupal/Core/Entity/Query/QueryInterface.phpundefined
@@ -0,0 +1,243 @@
+   *   Returns an integer for count queries or an array of ids. The values of
+   *   the array are always entity ids. The keys will be revision ids if the

Does the 'keys are revision IDs' hold even if you query for FIELD_LOAD_CURRENT? Seems a bit odd.

+++ b/core/modules/field/field.moduleundefined
@@ -1045,16 +1045,27 @@ function field_get_items($entity_type, $entity, $field_name, $langcode = NULL) {
-    // Neutralize the 'entity_field_access' query tag added by
-    // field_sql_storage_field_storage_query(). The result cannot depend on the
-    // access grants of the current user.
-    ->addTag('DANGEROUS_ACCESS_CHECK_OPT_OUT')

This comment wasn't carried over to the new code though.

+++ b/core/modules/field/field.moduleundefined
@@ -1045,16 +1045,27 @@ function field_get_items($entity_type, $entity, $field_name, $langcode = NULL) {
+    $result = $query
+      ->condition($group)
+      ->count()
+      ->accessCheck(FALSE)
+      ->range(0, 1)
+      ->execute();
+    if ($result) {
+      return TRUE;
+    }

This is lovely, so much nice r than the removed lines.

+++ b/core/modules/field/field.moduleundefined
@@ -1308,4 +1316,13 @@ function _field_create_entity_from_ids($ids) {
+/**
+ * A list of columns that can not be used as field type columns.
+ *
+ * @return array
+ */
+function field_reserved_columns() {
+  return array('deleted');

Why's this necessary?

+++ b/core/modules/field/modules/field_sql_storage/lib/Drupal/field_sql_storage/Entity/Query.phpundefined
@@ -0,0 +1,172 @@
+  /**
+   * Implements Drupal\Core\Entity\Query\QueryInterface::execute().
+   */
+  public function execute() {
+    $entity_type = $this->entityType;
...
+    }, field_info_field_map());

We should sort out the entity info so it can be injected, but that's likely to be easier once entity types as plugins is in, so OK leaving it for now.

Apart from that, I'm not awake enough to meaningfully review this hunk. There's a lot of complexity there, but the SQL storage for entities is complex and inconsistent, so...

+++ b/core/modules/field/modules/field_sql_storage/lib/Drupal/field_sql_storage/FieldSqlStorageBundle.phpundefined
@@ -0,0 +1,22 @@
+
+class FieldSqlStorageBundle extends Bundle {

oooh fancy.

+++ b/core/modules/field/modules/field_sql_storage/lib/Drupal/field_sql_storage/Tests/FieldSqlStorageTest.phpundefined
@@ -309,7 +309,7 @@ function testUpdateFieldSchemaWithData() {
-    field_attach_insert('test_entity', $entity);

Much nicer.

+++ b/core/modules/field/modules/options/options.moduleundefined
@@ -384,12 +384,19 @@ function options_field_update_forbid($field, $prior_field, $has_data) {
 function _options_values_in_use($field, $values) {
   if ($values) {
-    $query = new EntityFieldQuery();
-    $found = $query
-      ->fieldCondition($field['field_name'], 'value', $values)
-      ->range(0, 1)
-      ->execute();
-    return !empty($found);
+    $field = field_info_field_by_id($field['id']);
+    $factory = drupal_container()->get('entity.query');
+    foreach ($field['bundles'] as $entity_type => $bundle) {
+      $result = $factory->get($entity_type)
+        ->condition($field['field_name'] . '.value', $values)
+        ->count()
+        ->accessCheck(FALSE)
+        ->range(0, 1)
+        ->execute();
+      if ($result) {
+        return TRUE;
+      }
+    }

This got a bit more complicated. Any particular reason?

+++ b/core/modules/file/file.moduleundefined
@@ -1555,27 +1533,83 @@ function file_icon_map(File $file) {
  */

This also got considerably more complex. Why can't it be a straight port of the query?

+++ b/core/modules/system/lib/Drupal/system/Tests/Entity/EntityQueryTest.phpundefined
@@ -0,0 +1,433 @@
+  function setUp() {
+    parent::setUp();
+    drupal_install_system();
+    module_enable(self::$modules);

This looks like a good case for the new unit test base that sun's working on. For now WebTestCase would be better though than installing system module within the unit test.

+++ b/core/modules/system/lib/Drupal/system/Tests/Entity/EntityQueryTest.phpundefined
@@ -0,0 +1,433 @@
+    $GLOBALS['debug'] = TRUE;

;)

+++ b/core/modules/system/lib/Drupal/system/Tests/Entity/EntityQueryTest.phpundefined
@@ -0,0 +1,433 @@
+  function tearDown() {
+    $modules = self::$modules;
+    $modules[] = 'system';
+    foreach (self::$modules as $module) {
+      module_load_install($module);
+      $function = $module . '_schema';
+      if (function_exists($function)) {
+        foreach ($function() as $table => $schema) {
+          db_drop_table($table);
+        }
+      }
+    }
+    parent::tearDown();

Yeah this should definitely just use WebTestCase.

+++ b/core/modules/system/lib/Drupal/system/Tests/Entity/EntityQueryTest.phpundefined
@@ -0,0 +1,433 @@
+   * Warning: this is complicated.

hehe. Yes, yes it is.

I really hate to do this, since we are way above thresholds atm, but I don't see how this can be classified as a task. :(

A couple remarks - those are not criticals, and do not have to affect RTBC status.

function field_has_data($field) {
-  $query = new EntityFieldQuery();
-  return (bool) $query
-    ->fieldCondition($field)
-    ->range(0, 1)
-    ->count()
-    // Neutralize the 'entity_field_access' query tag added by
-    // field_sql_storage_field_storage_query(). The result cannot depend on the
-    // access grants of the current user.
-    ->addTag('DANGEROUS_ACCESS_CHECK_OPT_OUT')
-    ->execute();
+  $field = field_info_field_by_id($field['id']);

You receive $field as a param, so field_info_field_by_id() should not be needed ?
Same applies to _options_values_in_use().

 function _field_sql_storage_columnname($name, $column) {
-  return $name . '_' . $column;
+  return in_array($column, field_reserved_columns()) ? $column : $name . '_' . $column;
 }

I'm not sure I get the logic for what is being done here. field_create_field() should raise an exception if a column name is forbidden anyway, right ?

Drupal\field_sql_storage\Entity\Query::execute()

That's a block of 100+ lines, could really use a couple empty line separators :-)
Since this seems to be the very heart of the whole thing, it would help to make it visually digestible.
To a lesser extent, also applies to the methods in Drupal\field_sql_storage\Entity\Tables.

(in file_get_file_references())
+        // We need to find file fields for this entity type and bundle.
+        if (!isset($file_fields[$entity_type][$bundle])) {
+          $file_fields[$entity_type][$bundle] = array();
+          // This contains the possible field names.
+          $instances = field_info_instances($entity_type, $bundle);
+          foreach ($instances as $field_name => $instance) {
+            $current_field = field_info_field($field_name);
+            // If this is the first time this field type is seen, check
+            // whether it references files.
+            if (!isset($field_columns[$current_field['type']])) {
+              $field_columns[$current_field['type']] = file_field_find_file_reference_column($current_field);
+            }
+            // If the field type does reference files then record it.
+            if ($field_columns[$current_field['type']]) {
+              $file_fields[$entity_type][$bundle][$field_name] = $field_columns[$current_field['type']];
+            }
+          }
+        }

Looks like this could also be simplified/optimized by using field_info_field_map(). As is, code does field_info_instances($entity_type, $bundle); on all entity types and bundles, and is thus a drag memory-wise, since we keep those lists in statics for the rest of the request .
Also, this code block could use some air too...

Now the nitpicks :

-   * @return Drupal\Core\Database\Query\SelectInterface
+   * @return \Drupal\Core\Database\Query\SelectInterface

I thought we didn't do that ?
(also applies to several other places, @see, @var or @return statements - a search for \Drupal should raise a couple)

[edit: scratch that, turns out code standards promote leading antislashes on namespaces in comments... http://drupal.org/coding-standards/docs#namespaces]

+  /**
+   * Queries for the existence of a field.
+   *
+   * @param $field
+   * @param string $langcode
+   * @return ConditionInterface
+   * @see \Drupal\Core\Entity\Query\QueryInterface::exists()
+   */
+  public function exists($field, $langcode = NULL);

$field should be hinted as a string in the phpdoc, like for other methods in ConditionInterface ?
Also applies to the methods in QueryInterface.

(in field.module)
+use Drupal\Core\Entity\Query\QueryFactory;

Doesn't seem to be needed, we never refer to QueryFactory directly in this file, we call drupal_container()->get('entity.query');
Same applies to options.module and EfqTest.php

A couple methods have no phpdoc block at all.

This patch comes with precisely zero new features, so I cannot see how this can be classified as a feature request.

I think webchick did this for triage purpose because it raises the number of tasks way above threshold. For that I'd leave her this choice to make.

I will defer to webchick, but this is not a feature request in any way, shape or form.

If you defer to webchick, do not change again the status. And yes this can be treated as a feature request, since it adds features to the EFQ.

Does this issue also address (cross-entity-join) http://drupal.org/node/1446600 ?
Because it's not mentioned in summary

@sinasalek
It doesn't.

I discussed the category with webchick. IMO this is straightforward refactoring, making it a task - it's exactly the sort of thing we should be doing after feature freeze - trying to bring subsystems up-to-date with each other. It does introduce some new features to EntityFieldQuery but that's mostly a byproduct of the refactoring.

However it's also a sufficiently large refactoring that it should probably be blocked on the critical bug threshold while it's so ridiculously over.

I'm ambivalent to how it's classified, I just wanted to note that it's huge refactoring and I strongly believe we need to be under the criticals threshold to let it in. I believe catch and I are on the same page about that, so no need to dicker about with the category.

Getting there on thresholds now! Some persnickety major bugs.

This is all catch, all the time, so assigning to him for review/commit.

Things are looking much better with thresholds, so I've gone ahead and committed this. Will need a change notice. Please also open a follow-up for yched's points before marking this one fixed.

The change notice will help clarify what to do to reroll other patches on other issues (like #1188388-206: Entity translation UI in core, right? (Forgive the novice-ish Q; I'm still getting familiar with current core workflow stuff.)

#63 needs a follow-up to be created before this is closed, but the change notice is good.

Hm, would it make sense to maybe have a simple, typical 7.x EFQ query in the change notice with just 2-3 simple conditions and the result then passed to entity_load().

Because that example in the change notice is a) very complicated and b) not actually 7.x EFQ, there is no such thing as translatable properties in 7.x and therefore also no propertyLanguageCondition(), that was added in 8.x.

#1827476: Docs cleanup for EFQ v2 - I copied #63 there.

Thanks, but #63 was only minor doc stuff. #62 has the more important remarks.

thus, reopening until #62 is addressed or a corresponding followup is created.

Related: #1828790: Entity count query not working with fields shared among different entity types.

I was reading this patch yet again, and I saw this:
public function &conditions();

Returning a direct reference to an object property wakes up buggy PHP behaviors, see #1671848: by reference getter on objects such as dbSelect may break the __clone behavior of such objects.

I think the right way to go is to have methods such as addCondition() and removeCondition(ConditionInterface $condition) altogether.

Will it be possible to backport EntityFieldQuery v2 to Drupal 7 ? (in terms of dependencies)

Maybe as a contrib module (but does it even have a point then?), to Drupal 7 core never (it's a complete API change).

Re #31, done: #1874254: Provide more context in typed exceptions :)

#1969136: Move field_reserved_columns() to a static method would welcome some feedback from the authors of the patch.

The code contains no explanation on the logic behind this.
@catch in #58 and myself in #62 asked for some, but this was left unanswered.

Feedback also welcome in #1931900: EFQ calls field_info_field() too liberally (perf issue).
See #3 and following over there.

While you are changing EFQ anyway, how about allowing it to select fields instead of just filtering by them?
https://drupal.org/project/efq_extra_field in core! :)

@georgir, that's out of scope for this issue. Also, we've already passed API freeze, so please open a feature request against Drupal 9.

@georgir
This is probably the worst question you could have asked, sorry. That module is fundamental wrong in terms of API design of drupal.
All what drupal supports is entities, that's it.

If you are crazy you could get this information by using the aggregated query feature :)

updated

The query result is very useful: it's a simple array where the values are entity IDs so it can always directly be fed into entity_load_multiple. If the entity supports revision IDs then the array keys will be revision IDs, otherwise entity IDs -- as this does not affect the operation of entity_load_multiple it allowed for a simple way to get revision IDs whenever possible.

This feels like a regression to me as it implies (doesn't actually stop us, although we are departing from the interface) that you need to couple the query with a load_multiple to do anything useful

This idiom is now the norm in D7

$query
  ->entityCondition('entity_type', 'node')
  ->execute();
if (!empty($result['node'])) {
  foreach (node_load_multiple(array_keys($result['node'])) as $node) {
    ...
  }
}

While about 50% of the usages of this that I have seen were to simply get lists of node titles.

It was a trivial task to extend D7 EntityFieldQuery::finishQuery() to insert additional fields like node.title or whatever, drastically reducing server loads, but now if we do this in D8 we are breaking the expected returned output of REVISION_ID => ID as defined in QueryInterface.

For example in D7, an overridden finishQuery() to do this:

function finishQuery($select_query, $id_key = 'entity_id') {
  ... cloning parent functionality....
 
  // Add the title field.
  $select_query->addField('node', 'title', 'entity_label');
 
  $return = array();
  foreach ($select_query->execute() as $partial_entity) {
    $bundle = isset($partial_entity->bundle) ? $partial_entity->bundle : NULL;
    $entity = entity_create_stub_entity($partial_entity->entity_type, array($partial_entity->entity_id, $partial_entity->revision_id, $bundle));
    $entity->title = $partial_entity->entity_label;
    $return[$partial_entity->entity_type][$partial_entity->$id_key] = $entity;
    $this->ordered_results[] = $partial_entity;
  }
  return $return;
}

Note
- note no i18n considerations here, but it could easily be extended to include these as required
- assumes SQL storage, if it matters, I have never had to support anything else

I only found this after searching to see if multi-entity type queries were dead so that we never needed to do this every again:

if (!empty($result['type'])) {
  foreach($result['type']) {}
}

Big plus to this and everything else though!

[edit]
It is trivial to define your own method ::getEntityStubs() or something, I guess that is just less obvious to developers that you don't actually need to couple execute() with a load() ;)

I don't think this is a regression, on the contrary: returning only identifiers forces you to load entities, but thanks to entity caching in core, this won't hurt to much: 2 queries (including one on the cache backend in best case scenario) won't hurt you, and it ensures that all your entities have been fully loaded, hooks and events passed, and data is consistent. Using the partial entity from EFQ even in Drupal 7 is a real risk you are taking because some modules might change stuff on your entity titles or other fields and you skip that step, the whole stuff may be inconsistent. In Drupal 8 you are supposed to use the Entity accessors and not using direct database data manipulation. You are supposed to always access fields and not direct properties, if I understand the whole well.

Yep, point taken. Thank goodness that Moore’s Law has broken yet. Really a non-issue. ;)

is this fixed?