In Drupal 7 EntityFieldQuery had three kinds of conditions / sorts: Entity (entity id, revision id, bundle and entity type), Property (properties on the entity table) and Field (columns of fields). The proposal is that if the entity type is a given then the Entity conditions can be merged into Property: for example if the entity type is known to be node then instead of 'entity id' you can query for 'nid', 'bundle' becomes 'type', 'revision id' becomes 'vid'. 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. Also, just field name works and presumes the field column is "value". This entity / field specification also extends to every relevant method (exists, notExists, sort) making the API significantly simpler. A common interface for DBTNG, Views and EFQ conditions are in the plans.

It was often requested to add NULL support so exists() and notExists() methods have been added.

The conditions are handled by a proper condition interface, wrapped in the query much like DBTNG does. This allows for a DBTNG-like condition groupping as well. Instead of the truly whack and ugly delta and language grouping, there is clean AND/OR group support. Altogether, here is an example from the entity translation test:

    $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();

There is a drawback here: it is no longer possible to have a field column (like value or tid) called deleted. Every other field column is still valid. It's extremely unlikely this will be a problem.

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.

Files: 
CommentFileSizeAuthor
#60 1801726_60.patch223.18 KBchx
PASSED: [[SimpleTest]]: [MySQL] 46,118 pass(es). View
#59 1801726_59.patch223.73 KBchx
PASSED: [[SimpleTest]]: [MySQL] 46,122 pass(es). View
#53 1801726_53.patch223.79 KBchx
PASSED: [[SimpleTest]]: [MySQL] 46,122 pass(es). View
#50 1801726_50.patch119.29 KBchx
PASSED: [[SimpleTest]]: [MySQL] 46,116 pass(es). View
#48 1801726_48.patch119.28 KBchx
PASSED: [[SimpleTest]]: [MySQL] 46,128 pass(es). View
#45 1801726_45.patch119.24 KBchx
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: failed to enable simpletest module. View
#45 interdiff.txt17.26 KBchx
#42 drupal-efq_v2-1801726-41.interdiff.do_not_test.patch3.61 KBplach
#40 1801726_40.patch119.04 KBchx
PASSED: [[SimpleTest]]: [MySQL] 42,704 pass(es). View
#38 1801726_38.patch119.03 KBchx
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1801726_38.patch. Unable to apply patch. See the log in the details link for more information. View
#38 interdiff.txt6.92 KBchx
#36 1801726_36.patch118.2 KBchx
PASSED: [[SimpleTest]]: [MySQL] 42,696 pass(es). View
#36 interdiff.txt35.96 KBchx
#27 1801726_27.patch117.03 KBchx
PASSED: [[SimpleTest]]: [MySQL] 42,689 pass(es). View
#25 1801726_22.patch113.46 KBchx
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1801726_22.patch. Unable to apply patch. See the log in the details link for more information. View
#23 1801726_23.patch109.89 KBchx
FAILED: [[SimpleTest]]: [MySQL] 42,454 pass(es), 0 fail(s), and 1 exception(s). View
#23 interdiff.txt6.69 KBchx
#19 1801726_17.patch108.8 KBchx
PASSED: . View
#19 interdiff.txt1.7 KBchx
#17 1801726_17.patch108.8 KBchx
PASSED: [[SimpleTest]]: [MySQL] 42,421 pass(es). View
#17 interdiff.txt9.38 KBchx
#15 1801726_15.patch107.22 KBchx
FAILED: [[SimpleTest]]: [MySQL] 42,396 pass(es), 24 fail(s), and 1 exception(s). View
#13 1801726_14.patch101.71 KBchx
PASSED: [[SimpleTest]]: [MySQL] 42,098 pass(es). View
#13 interdiff.txt12.71 KBchx
#12 1801726_12.patch94.8 KBchx
PASSED: [[SimpleTest]]: [MySQL] 42,023 pass(es). View
#10 1801726_10.patch196.12 KBchx
FAILED: [[SimpleTest]]: [MySQL] 41,833 pass(es), 7 fail(s), and 1 exception(s). View
#10 interdiff.txt19.46 KBchx
#8 1801726_8.patch193.02 KBchx
FAILED: [[SimpleTest]]: [MySQL] 41,928 pass(es), 3 fail(s), and 1 exception(s). View
#6 1801726_5.patch211.8 KBchx
FAILED: [[SimpleTest]]: [MySQL] 42,016 pass(es), 3 fail(s), and 1 exception(s). View
#5 1805690_5.patch8.15 KBchx
PASSED: [[SimpleTest]]: [MySQL] 42,180 pass(es). View
#3 1801726-entityfieldquery-2.patch185.51 KBbojanz
FAILED: [[SimpleTest]]: [MySQL] 41,115 pass(es), 7 fail(s), and 1 exception(s). View
#2 1801726_1.patch21.28 KBchx
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: failed to enable simpletest module. View

Comments

chx’s picture

Issue tags: +Needs architectural review
FileSize
21.28 KB
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: failed to enable simpletest module. View

Totally new tests need to be written, the other 14 EntityFieldQuery needs to be converted, the field_has_data needs to be pushed into field_sql_storage. But, this is the new system and I have seen it working by logging in or running the Entity UUID test (both of which loads by properties which has been converted).

chx’s picture

Issue summary: View changes

trying to edit

bojanz’s picture

Status: Active » Needs review
FileSize
185.51 KB
FAILED: [[SimpleTest]]: [MySQL] 41,115 pass(es), 7 fail(s), and 1 exception(s). View

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.

Status: Needs review » Needs work

The last submitted patch, 1801726-entityfieldquery-2.patch, failed testing.

chx’s picture

Status: Needs work » Needs review
FileSize
8.15 KB
PASSED: [[SimpleTest]]: [MySQL] 42,180 pass(es). View

Another revision. Mostly simplification / code reorder but also age, isNull and isNotNull support.

chx’s picture

FileSize
211.8 KB
FAILED: [[SimpleTest]]: [MySQL] 42,016 pass(es), 3 fail(s), and 1 exception(s). View

Wrong _5 patch, doh!

Edit: there are remnants of superseded code in this patch: the classes FieldCountQuery, FieldCountQueryFactory, EntityQueryFactory.

Status: Needs review » Needs work

The last submitted patch, 1801726_5.patch, failed testing.

chx’s picture

Status: Needs work » Needs review
FileSize
193.02 KB
FAILED: [[SimpleTest]]: [MySQL] 41,928 pass(es), 3 fail(s), and 1 exception(s). View

Fixed the field SQL storage tests. Removed a bunch of files that were not in use anyways.

Status: Needs review » Needs work

The last submitted patch, 1801726_8.patch, failed testing.

chx’s picture

Status: Needs work » Needs review
FileSize
19.46 KB
196.12 KB
FAILED: [[SimpleTest]]: [MySQL] 41,833 pass(es), 7 fail(s), and 1 exception(s). View

Surely this won't pass but the API is coming along superb nicely. We have doxygen for the grouping -- thanks to jthorson and xjm for helping with those. I wrote doxygen for condition mostly by copying it over from the old one. The return value got meaningful keys: revision id if that exists, entity id if it doesn't. The values are still entity ids. A lot of thought went into making age work properly. The factory has been brutally simplified by getting the storage controller return the service name for the entity. Some method names have been renamed to be more generic / meaningful.

Status: Needs review » Needs work

The last submitted patch, 1801726_10.patch, failed testing.

chx’s picture

Status: Needs work » Needs review
FileSize
94.8 KB
PASSED: [[SimpleTest]]: [MySQL] 42,023 pass(es). View

Instead of deleting the old EFQ files, I am just neutering them to make this slightly easier to review. When this is commit ready, I will roll one with git rm. I hope this one actually passes. #1812056: field sql storage test failing on various MySQL engines due to indexing unbound text fields. has been filed.

chx’s picture

FileSize
12.71 KB
101.71 KB
PASSED: [[SimpleTest]]: [MySQL] 42,098 pass(es). View

I added tests, more doxygen. The tests found two serious bugs in OR handling and other small tidbits.

plach’s picture

Hope to dive into this soon :)

chx’s picture

FileSize
107.22 KB
FAILED: [[SimpleTest]]: [MySQL] 42,396 pass(es), 24 fail(s), and 1 exception(s). View

New tests which brought in new bugfixes: sorts are now LEFT JOIN, the query is now distinct and addField indeed adds fields which is necessary for the distinct to work. sort and age and string operations are now tested.

Status: Needs review » Needs work

The last submitted patch, 1801726_15.patch, failed testing.

chx’s picture

Status: Needs work » Needs review
FileSize
9.38 KB
108.8 KB
PASSED: [[SimpleTest]]: [MySQL] 42,421 pass(es). View

Sorting of 1-N tables is VERY complicated to do right. I talked to das-peter, consulted stackoverflow and various SQL manuals and I believe this is correct. A new test has been added for counting the result set -- previously the test only passed because fetchAllKeyed collapsed multiple results into one row but pagination would've been broken as the count was incorrect. To offset the new complexity, an optimalization has been added which avoids the complex query if it is not necessary. It does not take a lot to have this optimization and it's worth it.

chx’s picture

So, here's why do we need such ugly sort code. So if you do not want to make sorts to be also exists() calls you need to LEFT JOIN your field tables (edit: people will be dumbstruck if you order on a field and an entity is omitted from the results just because that field wasnt filled in). At this point, the result of pseudo-query SELECT ftid,figures.color,greetings.langcode FROM base LEFT JOIN figures LEFT JOIN greetings ORDER BY figures.color,greetings.langcode,ftid looks like this (I marked the first occurence of each entity with an *):

8 NULL pl *
12 NULL pl *
4 NULL tr *
12 NULL tr  
2 blue NULL *
3 blue NULL *
10 blue pl *
11 blue pl *
14 blue pl *
15 blue pl *
6 blue tr *
7 blue tr *
14 blue tr  
15 blue tr  
1 red NULL  
3 red NULL  
9 red pl *
11 red pl  
13 red pl *
15 red pl  
5 red tr *
7 red tr  
13 red tr  
15 red tr  

Now consider that for pagination reasons the result set must be made into 15 rows long, no matter what. How can you achieve that? You can GROUP BY ftid and then need to keep the order which can be done if you ORDER BY the lowest color value in the group first then the lowest langcode. This way the group containing (12, NULL, pl) and (12, NULL, tr) will be represented by (12, NULL, pl) which is exactly what we want. So the query will end up with GROUP BY ftid ORDER BY MIN(color), MIN(langcode). Very ugly but what can one do? As said, the system optimizes this away if the involved fields are single value.

chx’s picture

FileSize
1.7 KB
108.8 KB
PASSED: . View

I talked to David Strauss at length and he considers this very ugly but correct but noted that MAX needs a DESC. He is right! New test for DESC for multivalue fields.

David Strauss’s picture

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.

andypost’s picture

+++ 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.
andypost’s picture

Issue summary: View changes

fixed a little

chx’s picture

Issue summary: View changes

more issue summary

chx’s picture

Issue summary: View changes

fixed

andypost’s picture

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

chx’s picture

FileSize
6.69 KB
109.89 KB
FAILED: [[SimpleTest]]: [MySQL] 42,454 pass(es), 0 fail(s), and 1 exception(s). View

Thanks for the review.

> needs @todo code example
Added.

>Core bundle? What for? Is database initialized?
Well, this needs to be somewhere where data is always accessible regardless which storages you use There's not a better place imo.

No, namespace separator is backslash not forward slash, sorry you are wrong in this.

> is this constant accessible here?
It is in field.module, so yes.

> only 3?
Actuallly might be just langcode and deleted. This reroll only contains those two.

> how it works before?
Before it was doing this crazy insane thing of saving fields without entities. Now it just saves 'em entities.

> Filename is different from class inside? Maybe better delete this?
> commented test?
As said in #12 , we can wait with deleting up to commit time until then they are just killed. It'd add 100kb needlessly to the patch.

> Please mention about why this not needed any more
Because it only fired from efq test which is not running any more.

Edit: I also added a lot of inline comments into Query about this sorting business.

Status: Needs review » Needs work

The last submitted patch, 1801726_23.patch, failed testing.

chx’s picture

Status: Needs work » Needs review
FileSize
113.46 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1801726_22.patch. Unable to apply patch. See the log in the details link for more information. View

I could argue with #21 for a long time, but I won't, it's a minute detail which indeed can be debated later. I added tablesort, pager and lots of new comments. This patch should be close to ready (except that the old query class + test still needs a deletion which I still skip for the sake of easier review).

Status: Needs review » Needs work

The last submitted patch, 1801726_22.patch, failed testing.

chx’s picture

FileSize
117.03 KB
PASSED: [[SimpleTest]]: [MySQL] 42,689 pass(es). View

bojanz gave it a look over and so there is even more comments and I added tests for pager and tablesort. The tests are based on unit tests to make sure they as speedy as they can be.

chx’s picture

Status: Needs work » Needs review
chx’s picture

Issue summary: View changes

added return

chx’s picture

Issue summary: View changes

added a drawback

Sylvain Lecoy’s picture

Sylvain Lecoy’s picture

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 ?

chx’s picture

> it is not recommended to create new custom exceptions if they do not have useful information for client code.

Get Crell to agree with this and I will remove them. Until then, this is what core does all over the place. Do not try to convince me, I am not the one to convince nor is this issue the place for that. Please file a separate issue. Thanks!

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

That's just an overblown search-and-replace, will restore.

das-peter’s picture

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.

fago’s picture

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.

chx’s picture

> 'text.value' if 'text' is given,

And how will you query for text.summary and text.format? Talked to fago on IRC and we agreed it's not really useful.

> Why do we need both?? (langcode and fieldname.langcode)

Because both the entity and fields are translatable and noone said the entity and the field will have the same langcode. This test gives an example (the only example in core in fact) where an entity has a base table and a data table which contains multiple translations of the same entity. We need the entity langcode. Similarly, a field can have multiple languages on the same entity. I didn't write this insanely complex system, I am merely querying it.

> as a special, optional parameter

Absolutely not. I refuse to make anything a special parameter again, we saw where that lead.

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

That's not from this patch, no idea where you found it.

> Could we get a small entity_query($entity_type) helper function here?

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

> We should move to an opt-in approach for access checking IMO.

Why should we? Opt out is always better for security. Harder to forget. We grab the opportunity that we have semantic knowledge when the access check is (entity type node) and make the system more secure.

> entity.query.ENTITY_TYPE

would require a service per entity type. This sidesteps that need for very cheap.

fago’s picture

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

chx’s picture

FileSize
35.96 KB
118.2 KB
PASSED: [[SimpleTest]]: [MySQL] 42,696 pass(es). View

I have addressed all the minor concerns and have changed the code to use langcode as the last optional for condition/exists/notExists as fago asked and fago/bojanz/me discussed on IRC. I didn't change deleted, it's a mess. I have added better detection for configurable fields based on field_info_instances -- the new entity API is also based on that but it's not always working (perhaps because my entities are NG yet?) and so I thought this simple enough. It now defaults neatly to .value because it no longer needs to know about dots for recognizing a field. It just works. Do we want to add more magick to default to the single column of such fields? Still not removed the old code, the interdiff is 36kb already :)

chx’s picture

Issue summary: View changes

added a few more words

fago’s picture

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?

chx’s picture

FileSize
6.92 KB
119.03 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1801726_38.patch. Unable to apply patch. See the log in the details link for more information. View

> And is Condition the right term for it? It seems to contain multiple conditions.
> 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?
> As this is a group, the variable should be called conditionGroup.

DBTNG uses the class name Condition for it and the property is called conditions and I follow DBTNG. As I mentioned here and there, a unified condition system is planned as a followup and a rename can be done but only together with DBTNG. Having two extremely closely related things named differently is not to our advantage.

> Quite some methods misses docs there.

Fixed.

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

Changed. This is the only code change in this revision.

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

I do not think so, if you want that, pass that in. Every optional $langcode uses NULL in core. I am just following patterns. Optional docs fixed.

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

I deliberately left repeating queries use the factory pattern as it is faster.

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

Because good ole' file_get_file_references ran a bunch of cross entity type queries and was quite broken in the process (I filed a separate issue which noone reviews, I think I found 5 or 6 bugs) and so I would need to run an even larger amount of queries and instead I rewrote it.

Status: Needs review » Needs work

The last submitted patch, 1801726_38.patch, failed testing.

chx’s picture

Status: Needs work » Needs review
FileSize
119.04 KB
PASSED: [[SimpleTest]]: [MySQL] 42,704 pass(es). View

Keep up with HEAD.

yched’s picture

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

plach’s picture

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?

plach’s picture

bojanz’s picture

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.

chx’s picture

FileSize
17.26 KB
119.24 KB
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: failed to enable simpletest module. View

> field_info_field_map()

used.

> this should be named accessCheck().

done.

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

So reserved is only necessary when you write something like fieldname.deleted -- if were to allow a field column called deleted, how would we know whether you query the 'deleted' meta data or the field column deleted? (which is stored as fieldname_deleted in the SQL table but is exposed as ->delete[LANGUAGE_NONE][0]['value']). There was a point where langcode was like that too but now it's a separate value on condition and sort.

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

Is it. I added langcode to sorts as well. This actually simplified the Condition code because it no longer needs to create andConditionGroups -- the langcode is added to the join condition, instead.

> [single column magic] DamZ says "eeewww". I'm not married to the idea.

I side with DamZ on this.

> [thoughts about injecting pager and tablesort]

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.

plach’s picture

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

Status: Needs review » Needs work

The last submitted patch, 1801726_45.patch, failed testing.

chx’s picture

Status: Needs work » Needs review
FileSize
119.28 KB
PASSED: [[SimpleTest]]: [MySQL] 46,128 pass(es). View

No change just made it compatible with PHP 5.3 :)

fubhy’s picture

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

chx’s picture

FileSize
119.29 KB
PASSED: [[SimpleTest]]: [MySQL] 46,116 pass(es). View

I have not mentioned above but the GROUP BY now happens on every query where necessary not just the sorts. This now got a test. And I removed the constructor from the interface.

bojanz’s picture

Status: Needs review » Reviewed & tested by the community

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

plach’s picture

Status: Reviewed & tested by the community » Needs work

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

Edit: As per #12.

chx’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
223.79 KB
PASSED: [[SimpleTest]]: [MySQL] 46,122 pass(es). View

I take that as your approval of the RTBC -- if you have no other concerns than that, great! Reviewers: #50 is easier to do. This patch is the exact same just with two files deleted.

plach’s picture

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.

The last submitted patch, 1801726_53.patch, failed testing.

chx’s picture

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

#53: 1801726_53.patch queued for re-testing.

chx’s picture

Status: Needs review » Reviewed & tested by the community

Status reset after bot fluke.

catch’s picture

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.

chx’s picture

FileSize
223.73 KB
PASSED: [[SimpleTest]]: [MySQL] 46,122 pass(es). View

> Why do we need to count these?

No bloody idea. DBTNG does, so why not?

> This got a bit more complicated. Any particular reason?

Yeah. It queries across entity types which is not support and also I dare you to give the old query some semantic meaning. It's exploiting the fact that the SQL implementation INNER JOINs the tables.

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

The old one didn't even work, there's a separate issue filed with five bugs listed but noone reviewed that. And, this query is much like the previous one, cross entity query.

> 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.
> Yeah this should definitely just use WebTestCase.

Except this is faster and much more contained. I would rather keep it especially as you say sun's patch is coming.

Deleted the debug statements, opsie.

chx’s picture

FileSize
223.18 KB
PASSED: [[SimpleTest]]: [MySQL] 46,118 pass(es). View

catch asks for webtests. Sure. Deleting code is easy.

webchick’s picture

Category: task » feature

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

yched’s picture

Category: task » feature
Issue tags: +Needs architectural review

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

yched’s picture

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.

Damien Tournoud’s picture

Category: feature » task

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

pounard’s picture

Category: task » feature

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.

Damien Tournoud’s picture

Category: feature » task

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

pounard’s picture

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.

sinasalek’s picture

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

bojanz’s picture

@sinasalek
It doesn't.

catch’s picture

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.

chx’s picture

Cross entity joins are a followup and the syntax is scheduled for discussion with David Strauss (at least) next week and anyone else relevant I can hold of during BADcamp. I iwll try to get hold floretan cos of his remote entities article.

webchick’s picture

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.

webchick’s picture

Assigned: Unassigned » catch
Category: feature » task
Issue tags: -Needs architectural review

Getting there on thresholds now! Some persnickety major bugs.

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

catch’s picture

Title: EntityFieldQuery v2 » Change notice/follow-up: EntityFieldQuery v2
Priority: Normal » Critical
Status: Reviewed & tested by the community » Active

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.

YesCT’s picture

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

tim.plunkett’s picture

Title: Change notice/follow-up: EntityFieldQuery v2 » Follow-up: EntityFieldQuery v2

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

chx’s picture

Title: Follow-up: EntityFieldQuery v2 » EntityFieldQuery v2
Status: Active » Fixed
Berdir’s picture

Priority: Critical » Normal

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.

yched’s picture

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

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

yched’s picture

Status: Fixed » Active

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

chx’s picture

#1827588: Followup to EFQ v2 keeping open for the change notice fix.

plach’s picture

pounard’s picture

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.

sinasalek’s picture

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

bojanz’s picture

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

catch’s picture

Assigned: catch » Unassigned
Sylvain Lecoy’s picture

yched’s picture

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

yched’s picture

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

georgir’s picture

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! :)

cweagans’s picture

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

dawehner’s picture

@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 :)

dawehner’s picture

Issue summary: View changes

updated

Alan D.’s picture

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

pounard’s picture

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.

chx’s picture

#94 is right. The data you find in the database might be the same you find in an entity or it might not. Even if the data is the same, you might have access to it or you might not. Drupal has APIs. Use them and forget about talking to the database directly.

chx’s picture

Also, #597236: Add entity caching to core should seriously decrease the cost of mass entity loading.

Alan D.’s picture

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

andypost’s picture

is this fixed?

bojanz’s picture

Status: Active » Fixed

Status: Fixed » Closed (fixed)

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