Task: add relationship support to EntityQuery. Syntax: similar to what entity NG provides. If you have $node->field_image->entity->alt then your condition should be condition('field_image.entity.alt').

Issue fork drupal-1446600

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

saltednut’s picture

Title: EntityFieldQuery (pseudo-)join support » EntityFieldQuery join support
Version: 8.x-dev » 7.x-dev
Category: feature » support

I noticed this too today when attempting to use EntityFieldQuery with a taxonomy term reference field.

PDOException: SQLSTATE[42S22]: Column not found: 1054 Unknown column 'field_data_field_cool0.field_cool_value' in 'where clause': SELECT DISTINCT field_data_field_cool0.entity_type AS entity_type, field_data_field_cool0.entity_id AS entity_id, field_data_field_cool0.revision_id AS revision_id, field_data_field_cool0.bundle AS bundle FROM {field_data_field_cool} field_data_field_cool0 INNER JOIN {node} node ON node.nid = field_data_field_cool0.entity_id WHERE (field_data_field_cool0.field_cool_value IN (:db_condition_placeholder_0, :db_condition_placeholder_1)) AND (field_data_field_cool0.deleted = :db_condition_placeholder_2) AND (node.status = :db_condition_placeholder_3) AND (field_data_field_cool0.entity_type = :db_condition_placeholder_4) AND (field_data_field_cool0.bundle = :db_condition_placeholder_5) ORDER BY node.created DESC LIMIT 10 OFFSET 0; Array ( [:db_condition_placeholder_0] => 1 [:db_condition_placeholder_1] => 2 [:db_condition_placeholder_2] => 0 [:db_condition_placeholder_3] => 1 [:db_condition_placeholder_4] => node [:db_condition_placeholder_5] => article ) in field_sql_storage_field_storage_query() (line 577 of /modules/field/modules/field_sql_storage/field_sql_storage.module).

Note: 'cool' is the taxonomy term vocabulary machine name.

bojanz’s picture

Status: Active » Fixed

EntityFieldQuery doesn't assume SQL storage. The background implementation might not even know what a join is.
So no, you can't have joins. If you need that, either use a SelectQuery, or alter the query that EntityFieldQuery builds.

Alan D.’s picture

Title: EntityFieldQuery join support » EntityFieldQuery (pseudo-)join support
Version: 7.x-dev » 8.x-dev
Category: support » feature
Status: Fixed » Active

So both solutions would assume a sql installation, meaning that these are not useful for popular contrib. module that wants to do things globally. Reopening as a feature request for D8.

saltednut’s picture

NM I realize I was just using EFQ wrong in my case: http://api.drupal.org/api/drupal/includes%21entity.inc/function/EntityFi...

chx’s picture

Title: EntityFieldQuery join support » EntityFieldQuery (pseudo-)join support
Version: 7.x-dev » 8.x-dev
Category: support » feature
Status: Active » Closed (won't fix)

This is not a feature I would like to see implemented. for reasons listed by bojanz.

sinasalek’s picture

Status: Active » Closed (won't fix)

Since Drupal's data structure itself needs joining to represent it's data i think this is an important feature.
By join i mean joining several collections/tables/objects/etc by specific keys.
One common use case is filtering entities by a field on a their related entities (What relation module does)
like filtering comments by their nodes' publish status

So what we need is not join as we use in SQL dbs, since as already mentioned some back-ends may not event understand join. They may have all the information on single object. we need a way to make it possible for different back-ends to handle items relation or connection or join in their different fashion.
For example if back-end keeps comments of each entity as a an item inside it's entity item, It's its job to map the entered criteria and return the proper result.

What i'm trying to say is this a very necessary feature IMHO, if some back-ends are limited and do not support join, developers should simulate it or implement it in driver level. Otherwise we wouldn't be able to fully benefit from field api in modules like views

Another example, MongoDB does not support join but it has MapReduce and Aggregation (in recent versions). We can accomplish similar things in MongoDB too using it's own special methods

sinasalek’s picture

sinasalek’s picture

Status: Closed (won't fix) » Active

I open this issue again for further discuss, it might be a good idea to change the issue title to something more appropriate.

sinasalek’s picture

Status: Closed (won't fix) » Active
mitchell’s picture

Status: Active » Closed (duplicate)

Looks like join support is being added in #1801726: EntityFieldQuery v2.

chx’s picture

Status: Closed (duplicate) » Active

Nope, it's a followup. EFQ v2 does not have entity reference support. It will never have JOINs, those are not semantic.

sinasalek’s picture

Thanks for clarifying chx

chx’s picture

I had a very fruitful discussion (although rather one sided, but I truly don't mind cos I had no idea) with Damien Tournoud about how could we list 'articles created by users who joined the last 24 hours' and he suggested: with the datatype API, you can do $node->uid->entity->created so with EFQ you should be able to do ->condition('uid.entity.created', 1234567890, '>'). This is a viable and consistent syntax. He also raised the multi value (aka list) problem with ->condition('tags[x].entity.name', 'tag1')->condition('tags[x].entity.vid', 1) as a possible syntax for expressing those where x is an arbitrary identifier can be y, foo ...

Thoughts?

sinasalek’s picture

I see, that's actually very interesting. we also need something like ->condition('tags[all].entity.name', 'tag1'), to filter for example article if one of its tags was tag1 or ->condition('tags.entity.name', 'tag1') for if all the tags should be tag1
For this example it might also be a good idea to have a new operator to distinguish between "any" and "all" in case of multi values

I'm sure this approach has some limitations too but all the same it can address many use cases
One example is Drupal Commerce module, as you know in order to display a product it should be attached to a node (via entity reference). which this method you mentioned one can easily filter nodes by attributed of their attached products. But attached products also can use entity references so it should be properly implemented to be able to address more than one level.
->condition('products[all].entity.category.status', '1')

chx’s picture

Status: Active » Needs review
FileSize
13.1 KB

So this now supports ->condition('foo.bar:baz', 1). The colon was chosen to avoid a) the repetition of writing out .entity. all the time b) the horrific confusion arising from the changing behaviour of the dot -- sometimes it's a field-column separator but sometimes it's an entity reference dot. Let's just use some other character.

This needed surprisingly little code: the net added code without the test is just 14 LoC, most of the changes are just moving some code from Query::execute to Tables::addField. The only tricky thing in this patch is the $path variable, cos if you take a 'tags' term reference field on nodes and a 'node_reference' field referring again nodes then

->condition('tags', 2, '>')
->condition('tags', 20, '<')
->condition('node_reference.nid:tags', 2)

For the first two, we must have the same field table but for the third one we actually need a new one so we need to actually discern between the two and that's what $path is for.

chx’s picture

FileSize
4.03 KB
13.23 KB

Calculate entity tables only when ensureFieldTable is being called. Make the relationship chaining join more readable. Zero functional change.

Status: Needs review » Needs work

The last submitted patch, 1446600_16.patch, failed testing.

chx’s picture

Status: Needs work » Needs review
FileSize
14.22 KB

Right, right I used the : for passing in the field id. Either we switch the : to ! or what this here does, change the id: to id@.

chx’s picture

FileSize
16.95 KB

Here's the next version which supports the -> notation of the new entity API and only works with entities and fields both converted to that. If you have a crazy field which has two columns referring two entities, this code will still stay working IMO. Should we decide that field_image->image is better than field_image->entity, bring it in. Once the conversion is complete we can nuke addField and keep addFieldNG.

chx’s picture

FileSize
15.88 KB

Oh and we can revert the id@ changes because we no longer specialcase : , we use the arrow for that. To sum up, the syntax is condition('f0->f1->f2->f3') and then the code figures out whether fX->fX+1 is a field with a column, a field with an entity notation etc etc. It's slightly complex because it needs to decide whether 'image' in field_foo->image is an entity notation or a field column.

One loop does:

  1. If the previous iteration loaded some propertyConditions, those must be such that the current specifier is an entity notation. Ie, user_id->entity, first iteration loads the property definitinos for user_id, this step checks for entity and adds the user entity. Empty out propertyConditions and jump back to the beginning.
  2. Now, handle configurable fields. Look ahead one. If it's an entity notation then read the id source and that is our field column. if it's not an entity notation then it's our field column. If there's nothing ahead of us, fall back to .value.
  3. Now handle entityConditions.
chx’s picture

FileSize
16.62 KB

Added a lot, a lot of comments and merged addField and addFieldNG left comments on what to remove once the NG business is done. I have begun to work on a taxonomy term item in #1839070: Implement the new entity field API for the taxonomy reference field type so we can test configurable fields.

chx’s picture

FileSize
17.07 KB

And now: bundle support. Some duplicate code removal. And, there's nothing to remove once EntityNG is complete. Before that, if you try to query a relationship, you get what you deserve :) Overall nicer.

chx’s picture

FileSize
3.21 KB
16.85 KB

For fields, we are pulling the property definitions straight out of typed_data() instead of creating a whole entity. I wonder whether some simplification is possible on the entity side of this too. And, as usual, better comments (there are more comments than code for some revisions now).

chx’s picture

Issue summary: View changes

Updated issue summary.

chx’s picture

FileSize
17.58 KB

We lost support for $entity->fieldname->columname->entity, this is added back. bojanz found that I forgot to delete id_key from execute, it's now gone. The amount of comments addField now have would probably be approved even by webchick: the function is 86 lines of code plus 52 lines of inline comments.

chx’s picture

FileSize
27.51 KB

As #1839070: Implement the new entity field API for the taxonomy reference field type is not progressing, I rerolled with it included. Note: there is nothing new here only tests and taxonomy. The code behaves as I expected. This issue is ready to go.

chx’s picture

FileSize
5.45 KB
20.23 KB

Rerolled now that the taxonomy patch is in. The interdiff is against #24, as you can see it's just tests.

bojanz’s picture

Status: Needs review » Reviewed & tested by the community

I've reviewed this several times and I think it is ready to go.

It has a test, simplifies execute/ensureEntityTable/ensureFieldTable, adds a giant feature with a small amount of code.
The code (addField()) is not trivial, but it is super-documented (it probably has more docs than code) so it is understandable.

webchick’s picture

Status: Reviewed & tested by the community » Needs work

chx walked me through this patch. The comments are already quite good, but I pointed out a few places they could be improved below.

In general, the something.something.something and parsing relationships out of that seems quite clever. I had one actionable thing which was to change the prefix on table results from 0 and 1 to something more "real" (or drop it altogether). Chx is working on these suggestions right now. Once these are addressed, I have no problems committing.

+++ b/core/modules/field/modules/field_sql_storage/lib/Drupal/field_sql_storage/Entity/Tables.phpundefined
@@ -59,19 +59,140 @@ function __construct(SelectInterface $sql_query) {
+    // This variables ensures grouping works correctly. For example:

(nitpick) This "variable" ensures, or "These" variables "ensure"

+++ b/core/modules/field/modules/field_sql_storage/lib/Drupal/field_sql_storage/Entity/Tables.phpundefined
@@ -59,19 +59,140 @@ function __construct(SelectInterface $sql_query) {
+    // new table. So for the first two, the table array index will be '0.tags'
+    // while the third will be '1.tags'.

In SQL, you disambiguate with "c.nid" vs. "n.nid" or something that gives you some idea what the base table was where things came from. I feel that using 0 and 1 makes it harder to tell where these things originate from. Can these be straight-up table names instead?

+++ b/core/modules/field/modules/field_sql_storage/lib/Drupal/field_sql_storage/Entity/Tables.phpundefined
@@ -59,19 +59,140 @@ function __construct(SelectInterface $sql_query) {
+      // This can either be the name of an entity property (non-configurable
+      // field), a field API field (a configurable field) or an indication
+      // that the next specifier will be on another entity (here it'll be
+      // called relationship field).

This is nice in that it provides a nice overview of what's happening, but it's not clear what order this is happening down below; it all seems a bit mooshed together.

+++ b/core/modules/system/lib/Drupal/system/Tests/Entity/EntityQueryRelationshipTest.phpundefined
@@ -0,0 +1,156 @@
+  /**
+   * Overrides \Drupal\simpletest\WebTestBase::setUp().
+   */

(nitpick) For some dumb reason, we don't comment setUp() even though we do every other "extends" method. :\ So this doc should be removed.

+++ b/core/modules/system/lib/Drupal/system/Tests/Entity/EntityQueryRelationshipTest.phpundefined
@@ -0,0 +1,156 @@
+  protected function setUp() {
...
+  public function testQuery() {

Both of these could stand a few inline comments to walk me through your thought process and why you're setting up these values / testing these combinations.

+++ b/core/modules/system/lib/Drupal/system/Tests/Entity/EntityQueryRelationshipTest.phpundefined
@@ -0,0 +1,156 @@
+    $this->assertResults(array(1, 2));
...
+  protected function assertResults($expected) {

Let's get some PHPDoc on this function. I am not sure what we are actually asserting here.

chx’s picture

Status: Needs work » Needs review
FileSize
9.72 KB
21.72 KB

Very minimal code changes, just made the index_prefix textual. Now the test got a few comments...

webchick’s picture

Status: Needs review » Reviewed & tested by the community

This looks like it resolves all of my feedback (and then some ;)). Thanks!

Unfortunately, we're over thresholds atm, but marking RTBC to reflect that I've looked at it. I'll commit in my next feature queue smash.

chx’s picture

FileSize
21.57 KB

This accidentally got committed, here's a revert to revert and recommit.

catch’s picture

Title: EntityFieldQuery (pseudo-)join support » Change notice: EntityFieldQuery (pseudo-)join support
Category: feature » task
Priority: Normal » Critical
Status: Reviewed & tested by the community » Active

Reverted then re-committed so we can properly blame chx for this when the time comes.

The addField() method is extremely long, but at least part of that is due to the complexity of field_sql_storage itself which isn't this patch's fault.

This could use a change notice.

xjm’s picture

Issue tags: +Needs change record
dawehner’s picture

chx’s picture

Title: Change notice: EntityFieldQuery (pseudo-)join support » EntityFieldQuery (pseudo-)join support
Status: Active » Fixed
amateescu’s picture

Priority: Critical » Normal
Issue tags: -Needs change record

Status: Fixed » Closed (fixed)

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

Anonymous’s picture

Issue summary: View changes

Updated issue summary.

Kingdutch made their first commit to this issue’s fork.