Add entity query condition for delta in EFQ

My first attempt at writing core patch

typo fixed from comments and test only patch added

I see what you mean.

So tests would need to be extended with a field named 'delta' and a delta query. Then this list of columns to be queried with has to come from some other way than the reserved column names.

I'll try...

But wait, the syntax is same. There is no way of knowing whether ->condition('field_name.delta') wants to target field column named delta (so actual column field_name_delta) or the delta of the field. So that wouldn't fix anything.

So the question is can delta be a reserved column name?

entity_id and revision_id removed

Issue summary updated.

I'm not yet sure about langcode, but that is usually not something that is queried per-field. Instead, that's done globally, so you query on condition('langcode', 'en') and then the field joins should respect that. But you're never going to query a field on one langcode and another on a different one.. I think.

For delta, what if we support numbers directly somehow? $query->condition('some_ref_field.1.target_id', 5) ? We don't have to worry about clashes there and then we can create a combined condition for delta = 1 and target_id = 5. I don't know what happens if you try to use multiple deltas for the same field in the same query, but I don't think we need to support that and it wouldn't work if you specify it separately either anyway. So just detect that and throw an exception.*

Edit: Or make a second join?

I think plach has thoughts about querying by langcode, so assigning to him for his opinion.

That's better than nothing, but in Relation there could be a need to do a query for delta > 0. But then again there is also need to ensure that condition groups can not match the same delta value twice so maybe similar workaround could be used for delta > 0 case.

I've added few tests to get this started.

Simple delta conditions implemented. Tests should fail from the wildcard '?' but tests prior to that could pass

And here is the whole deal. The test I posted earlier was a bit wrong so that's updated now. And proposed syntax in issue summary didn't work out as there is no way of knowing which field to target with ->condition('?', ... so I changed that to ->condition('field_name.?', ....

and here is the missing tests plus small test improvements.

+++ b/core/modules/system/src/Tests/Entity/EntityQueryTest.php
@@ -443,6 +443,44 @@ public function testCount() {
+      ->condition("$figures.0.color", 'red')
...
+      ->condition("$figures.1.color", 'red')
...
+      ->condition("$figures.?.color", array('blue', 'red'))
+      ->condition("$figures.?", array(0, 1), 'IN')
...
+      ->condition("$figures.?", 1)

I think @alexpott has said in IRC he has no issue with this patch. Code review looks fine to me. Can we add/update a change notice for above cases? Thank you @mikran and @chx for all the hard work on the patch.

Looks OK to me, although I'm not very happy to make get addField() method even larger. I'll try to refactor it a bit in #2391217: Support base fields with multiple columns in entity queries, I think structuring that a bit in multiple methods will help a lot with being able to read and understand the code in there (Especially for non-super-humans aka @chx ;))

A bit confused about the "?" magic though, what happens if you don't provide a corresponding condition for it? What happens if there is more than one?

This will conflict with #2068655: Entity fields do not support case sensitive queries, which I'd like to get in first. (Edit: Wrong issue reference there before, fixed now)

reroll

#2068655: Entity fields do not support case sensitive queries is in, can we get this forward?

And I'd say this could also be major as a regression from D7.

another reroll

Had to debug this a bit to understand how it works, got it now.

That said, a lot of thought went into getFieldColumName() recently in #2392209: DefaultTableMapping::getFieldColumName is broken for base tables and how Tables uses it (and there is a follow-up related to deleted fields), and I'm not sure if this is the right way to do it.

Would be great if @yched could have a look at it.

reroll

Haven't really delved into the code yet.

For now, just a note :

+++ b/core/lib/Drupal/Core/Entity/Sql/DefaultTableMapping.php
@@ -146,7 +146,10 @@ public function getColumnNames($field_name) {
-    if ($this->allowsSharedTableStorage($storage_definition)) {
+    if ($property_name == '?') {
+      return 'delta';
+    }

As pointed in #2392809: EFQ should not call DefaultTableMapping::getFieldColumName() for the 'deleted' column, I think it's really weird that DefaultTableMapping::getFieldColumName() has to account for EFQ calling it for non-field columns like "deleted" (in current HEAD) and "delta" (with this patch). I guess I'm fine with cleaning that up in that issue over there though.

No real opinion on the code internal to Tables - that code is too complex for me, so I'm happy if tests pass :-)

+++ b/core/lib/Drupal/Core/Entity/Sql/DefaultTableMapping.php
@@ -146,7 +146,10 @@ public function getColumnNames($field_name) {
+    if ($property_name == '?') {
+      return 'delta';
+    }

This frankly bugs me, though. It's syntax convention from EFQ leaking into the field storage code, I really wish we could avoid that.

The job of DefaultTableMapping::FieldColumnName() is to return the DB column name corresponding to a *field property*.
I understand that EFQ also needs express conditions on "meta" things that are *not* field properties (deleted, delta), and it currently has no-one to turn to to know the name of the actual db column for those.

I guess I'd be fine if FieldColumnName() also gets asked for the column names of meta properties, if those meta properties were clearly denoted with an underscore :

if ($property_name == '_deleted') {
  return 'deleted';
}
elseif ($property_name == '_delta') {
  return 'delta';
}
else {
  // actual field property, compute the name...
}

So, whatever the syntax EFQ choses for delta conditions, it should end up calling DefaultTableMapping::FieldColumnName() with $actual_field_property_name, '_deleted' or 'delta'.

I'm fine with handling the case of '_deleted' in #2392809: EFQ should not call DefaultTableMapping::getFieldColumName() for the 'deleted' column, but I'd prefer if we passed '_delta' rather than '?' in this patch here.

Also : in the current patch, DefaultTableMapping::getFieldColumnName() always replies "sure, I have a column for 'delta', and it's called 'delta'". But that is only true for fields stored in dedicated table.

If someone writes an EFQ with a delta condition on a field that is stored in a base table (typically, a base field like node.title), then the current code will execute a query on the 'delta' column of the base table :
- if you're lucky, there's no 'delta' column in that table and you query will fail, letting you know you wrote a flawed EFQ
- if the entity type happens to have a base field named 'delta', then the column does exist, but it's something completely unrelated, and the query will behave in very weird and unexpected ways.

In short :
- DefaultTableMapping::getFieldColumnName() should not lie, it should only return the name of the 'delta' column if the field is stored in a shared table
- I think we need to throw an exception when someone writes an EFQ with a delta condition on a field that is stored in a base table

Last, just a matter of taste, feel free to ignore :
Not sure '?' is the most intuitive syntax convention for deltas, I'd think '*' would possibly make more sense, both for :
"count deltas" : ->condition("myfield.*", 3)
"condition on any delta" : ->condition("myfield.*.value", 'foobar')

I think I've now addressed your concerns. Tables is now passing _delta instead of ? to DefaultTableMapping::getFieldColumnName() and that same function restructured a bit so that the new _delta is resolved only for dedicated table storage fields, same as with the deleted.

And about the wildcard. I think * has other meaning as a 'any' wildcard already. That could be understood as ->condition("myfield.*", 3) if any column has value of 3.

Thanks for the fast update, @mikran.

The logic in DefaultTableMapping::getFieldColumnName() is not right yet.

    if ($this->allowsSharedTableStorage($storage_definition)) {
      $column_name = count($storage_definition->getColumns()) == 1 ? $field_name :  $field_name . '__' . $property_name;
    }

This still means that if I write an EFQ with a delta condition on a field stored in a shared (base) table (for which there exists no 'delta' column for the field to begin with), the 1st 'if" branch here runs, and the resulting query will have a condition on an incorrect column - either $field_name or "{$field_name}___delta", depending on the field type. If the latter, the column doesn't exist and the query will break; if the former, the column does exist but is the one that holds the field values, so the query has unexpected behavior.

--> The "$this->allowsSharedTableStorage($storage_definition)" case also needs to check "if $property_name is _delta, return NULL".

What EFQ then does if getFieldColumnName() says "I don't have a delta column", I'm not sure.
If we throw an exception, it means the same EFQ will either work or throw an exception, depending on how the field is stored, and that's a situation we've always tried to avoid : the same code should work whatever the way the field is stored.

Semantically, a field stored in a shared base table only has either no value (db stores NULL) or one value at delta 0 - but there is no explicit 'delta' info in the db.
So ideally, if an EFQ contains a delta condition on a "base table" field, the EFQ runtime code would "translate" it according to the above semantics and ensure the actual query runs ok and returns the correct results. Not sure that's doable though :-/

Sorry if #145 was confusing, the truth is, it's not clear to me how to move forward :-/

Trying to clarify. Bottomline is : how do we handle conditions on "delta" on fields that are stored in a shared table, since there is no "delta" column for the field in the db ?

1) DefaultTableMapping::getFieldColumnName('_delta') should return NULL instead of "delta" in the "if ($this->allowsSharedTableStorage($storage_definition))" case.
There is no delta information in the db in shared tables, we cannot pretend there is and build a query on it, because there could be a totally unrelated column that happens to be named "delta". In this case, the query would run OK, but return completely weird results.

2) When receiving that NULL, Tables::addField() needs to decide what to do, and this is where it gets tricky.

I see several options :

a) Throw an exception. This de-facto forbids writing an EFQ with a '_delta' condition on a field that is stored in a shared table.
That's sucky DX, because it requires you to know how the SqlStorage decided to store the field before writing your EFQ.
it also means that some code that works today might break tomorrow, for example if a base field gets changed from "mono value" to "multiple values'.

b) Just silently skip the condition. That would return "wrong" results.
For a condition of "field with value 'foo' in a delta > 3", keeping or ignoring that condition depending on how the field is stored means you cannot actually trust the result set actually matches the condition you specified.

c) Figure out a finer way of handling those conditions at runtime, since we can't rely on the query to enforce them. Like :
- if the condition is enforcing values in deltas > 0, then don't run the query at all, the result set is empty (a field in a shared table cannot have multiple values)
- If the condition is on delta 0, just treat it as a condition without the delta part
- ...
But getting all the cases right is likely to be tricky : we have "OR" groups, conditions on "value == 'foo'" are probably not the same as conditions on "value <> 'foo'"...

I don't have the answer, thoughts welcome :-)

Yay - happy that you think there's a way to make c) work :-)

I'll work a bit on this, here is rerolled patch for starters

Updated patch. I tried to implement the idea of 'c' option.

I didn't touch getFieldColumnName() but instead deltas are checked and handled before field columns. Wildcard conditions must match 0 and 1 <> 1 condition is added if delta is specified and the value is greater than 0.

minor fix

and that was wrong patch, reupload

I've manually tested this and it looks to me like it applies and works correctly. I can't speak for the decision about the internals other than the code looks good and well documented, so I'm going to tentatively set this as RTBC. Would be fantastic to get this in as it's a really useful addition to entity queries.

I think the syntax for using it is nice and obvious. I agree that * is probably more prone to confusion than ?, though I can understand why ? maybe also isn't ideal.

I don't know if this can go into 8.0.x as a bug fix (as it was a regression and is documented as though it's a working feature in https://www.drupal.org/node/1827278), or if it needs to wait for 8.1.x as a new feature?

Moving to 8.1.x and task.

Some of the capabilities here I'm struggling with a use-case for, but if we had it in 7.x it's feasible that someone's relying on it.

Why ? as wildcard and not *?

Okay, maybe I don't have a strong argument for which wildcard we should use but I do have many use cases for delta queries.

One example for the wildcard. Let's assume a multi-value field that can be ordered. Field title is "My favorite artists" and on the artist page we then show a number of users who include this artist in their TOP5 artists.

To get that number we need a query with a delta wildcard condition like: ->condition('field_artist.?.name', '{artist-name}')->condition('field_artist.?', 5 '<')

Looks like this needs to be rolled again as it won't apply to 8.x-1.x

Cleaning up tags.

Rerolled the patch from #58.

Replaced ? wild card with *.

1. +++ b/core/lib/Drupal/Core/Entity/Query/QueryInterface.php
   @@ -46,6 +46,17 @@ public function getEntityTypeId();
   +   *   entities which have at least six tags. Also, tags.* within a condition
   +   *   group means a specific delta so
   +   * @code
   +   *   ->condition('tags.*', 0, '>'))
   +   *   ->condition('tags.*.value', 'foo'))
   +   * @endcode
   

   I'm not sure I fully understand the need for the * on the condition. Is it that it shares the same $index_prefix internally so it uses the same join? We should try to document this better.

2. +++ b/core/lib/Drupal/Core/Entity/Query/QueryInterface.php
   @@ -46,6 +46,17 @@ public function getEntityTypeId();
   +   *   ->condition('tags.*.value', 'foo'))
   
   +++ b/core/lib/Drupal/Core/Entity/Query/Sql/Tables.php
   @@ -265,12 +307,12 @@ protected function ensureFieldTable($index_prefix, &$field, $type, $langcode, $b
          }
   -      $this->fieldTables[$index_prefix . $field_name] = $this->addJoin($type, $table, "%alias.$field_id_field = $base_table.$entity_id_field", $langcode);
   +      $this->fieldTables[$index_prefix . $field_name] = $this->addJoin($type, $table, "%alias.$field_id_field = $base_table.$entity_id_field", $langcode, $delta);
        }
   

   delta is not part of the key.

   Isn't that a problem if you for example have two different conditions for the same field, one with delta and one without.

3. +++ b/core/lib/Drupal/Core/Entity/Query/Sql/Tables.php
   @@ -255,7 +297,7 @@ protected function ensureEntityTable($index_prefix, $property, $type, $langcode,
       */
   -  protected function ensureFieldTable($index_prefix, &$field, $type, $langcode, $base_table, $entity_id_field, $field_id_field) {
   +  protected function ensureFieldTable($index_prefix, &$field, $type, $langcode, $base_table, $entity_id_field, $field_id_field, $delta) {
        $field_name = $field->getName();
   

   Can we add a new argument to a protected method? in a minor release? in a patch release?

   I guess we should make the whole class @internal, which would then definitely allow changes like this, but again not sure under which conditions we can do that exactly.

4. +++ b/core/lib/Drupal/Core/Entity/Sql/DefaultTableMapping.php
   @@ -203,7 +203,12 @@ public function getFieldColumnName(FieldStorageDefinitionInterface $storage_defi
   +      if ($property_name == '_delta') {
   

   is _delta safe enough as a magic property name? should we make that a constant? Or use an invalid character for an actual propery, like :delta or *delta, to stick to the wildcard character?

5. +++ b/core/tests/Drupal/KernelTests/Core/Entity/EntityQueryTest.php
   @@ -512,6 +512,44 @@ public function testNestedConditionGroups() {
   +    $figures = $this->figures;
   +    // Test numeric delta value in field condition.
   +    $this->queryResults = $this->factory->get('entity_test_mulrev')
   +      ->condition("$figures.0.color", 'red')
   +      ->sort('id')
   +      ->execute();
   +    // As unit 0 at delta 0 was the red triangle bit 0 needs to be set.
   +    $this->assertResult(1, 3, 5, 7, 9, 11, 13, 15);
   

   based on what I wrote above, I think we need a test that does a check on two different deltas, e.g. a an or condition?

Should this be a bug since it is a regression compared to D7?

Addressed #70.4 for now. Is \Drupal\Core\Entity\Query\QueryInterface right place to add this constant?

Wrong patch.

Wrong patch uploaded. This should be the correct one.

Moved the constant in Drupal\Core\Entity\Sql\TableMappingInterface as discussed with @Berdir and @slashrsm.

Regarding #70.2, it looks like $index_prefix already contains a delta in it. Added tests for it.

Patch is passing.

Attempt to improve the documentation.

Ant 74-77 interdiff.

+++ b/core/lib/Drupal/Core/Entity/Query/QueryInterface.php
@@ -47,16 +47,27 @@ public function getEntityTypeId();
+   *   name and the column name. This will require condition to be satisfied on
+   *   a specific delta of the field.
+   * @code
+   *   ->condition('tags.5.value', 'news')
+   * @endcode
+   *   Condition above will require exactly 6th value of the field to match the
+   *   provided value.
+   *   Further, it's possible to create a condition on delta itself by using a
+   *   * as a wildcard. For example,
+   * @code
+   *   ->condition('tags.*', 5)
+   * @endcode
+   *   will find only entities which have at least six tags.
+   *   Finally, wildcard condition on delta accompanied with a condition on the
+   *   value itself will require the value to appear in the specific delta
+   *   range. For example,

Documentation itself seems fine, just some changes on the wording/grammar.

I would remove "This will require condition.." sentence, instead, move the explanation below the code example above, The condition below... and I'd just write "the 6th value" instead of exactly.

.. on *the* delta ... Maybe also leave out "as a wildcard", because it's actually not really a wildcard. It's just the character we use for the delta.

Also not sure if we make code examples part of a sentence like that, might be easier to just have the explanation above.

Fixed most of comments from #79.

I was discussing this with @berdir and we agreed we could use something that is more self-explanatory instead of "*". "%delta" for example. In this case it would make sense to use this for const DELTA too.

Replaced * with %delta.

%delta feels a lot better.

1. +++ b/core/lib/Drupal/Core/Entity/Query/QueryInterface.php
   @@ -46,6 +46,28 @@ public function getEntityTypeId();
   +   *   field. Condition above will require 6th value of the field to match the
   +   *   provided value.
   +   *   Further, it's possible to create a condition on the delta itself by using
   +   *   a  '%delta'. For example,
   

   Some grammar things, native speakers are welcome to correct me:

   The condition above will require the 6th value of the field to match the provided value.

   .. by using '%delta'.

2. +++ b/core/lib/Drupal/Core/Entity/Query/QueryInterface.php
   @@ -46,6 +46,28 @@ public function getEntityTypeId();
   +   *   Finally, wildcard condition on delta accompanied with a condition on the
   +   *   value itself will require the value to appear in the specific delta
   +   *   range. For example,
   +   * @code
   +   *   ->condition('tags.%delta', 0, '>'))
   +   *   ->condition('tags.%delta.value', 'news'))
   

   I still think that it's not really a wildcard? and that code as part of sentences is problematic.

What if we just omit the word "wildcard". It still sounds clear enough to me.

Just for my own clarification, we completely ignore these string when we deal with config entities?

@dawehner: Yes. config and content entities interpret conditions in completely different ways, code isn't shared there.

But yes, it's possible that we should document that this only works for content entities?

1. The general documentation looks okay to me

2. +++ b/core/lib/Drupal/Core/Entity/Query/QueryInterface.php
   @@ -46,6 +46,28 @@ public function getEntityTypeId();
       *   same delta within that field.
   +   *   It is also possible to add the numeric delta value between the field
   

   This doesn't flow right.

3. +++ b/core/lib/Drupal/Core/Entity/Query/QueryInterface.php
   @@ -46,6 +46,28 @@ public function getEntityTypeId();
   +   * @code
   +   *   ->condition('tags.5.value', 'news')
   +   * @endcode
   

   I think the @code should be indented too? no. I couldn't see the standards for this on https://www.drupal.org/coding-standards/docs#code. But we certainly got other instances in core.

4. +++ b/core/lib/Drupal/Core/Entity/Query/QueryInterface.php
   @@ -46,6 +46,28 @@ public function getEntityTypeId();
   +   *   match the provided value.
   +   *   Further, it's possible to create a condition on the delta itself by using
   ...
   +   *   will find only entities which have at least six tags.
   +   *   Finally, the condition on the delta itself accompanied with a condition
   

   Wrapping.

5. +++ b/core/lib/Drupal/Core/Entity/Query/Sql/Tables.php
   @@ -164,6 +189,24 @@ public function addField($field, $type, $langcode) {
   +          // Is this a delta wildcard?
   ...
   +          // Is this a delta value?
   

   I think this documentation could be a clearer as to what is going on here.

6. +++ b/core/lib/Drupal/Core/Entity/Query/Sql/TablesInterface.php
   @@ -11,8 +11,10 @@
   +   *   If it doesn't contain a dot, then an entity base field name.
   +   *   If it contains a dot, then either field name dot field column or
   +   *   field name dot delta dot field column. Delta can be a numeric value or a
   +   *   %delta for any value.
   

   wrapping

Rerolled. Fixed all items from #88. Also added a note about config entities and added few more test cases.

+++ b/core/lib/Drupal/Core/Entity/Query/Sql/Tables.php
@@ -264,7 +312,7 @@ protected function ensureEntityTable($index_prefix, $property, $type, $langcode,
    */
-  protected function ensureFieldTable($index_prefix, &$field, $type, $langcode, $base_table, $entity_id_field, $field_id_field) {
+  protected function ensureFieldTable($index_prefix, &$field, $type, $langcode, $base_table, $entity_id_field, $field_id_field, $delta) {
     $field_name = $field->getName();
     if (!isset($this->fieldTables[$index_prefix . $field_name])) {
       $entity_type_id = $this->sqlQuery->getMetaData('entity_type');

This is my only remaining remark.

I think we can do this in 8.2.x since it is a protected method, but it's not something we can get into 8.1.x AFAIK. If it's worth fixing there, we could set it back to 8.1.x after committing to 8.2.x and find a way that avoids this change?

It's worth noting that this is a (soft) blocker for the now critical #1266748: Changing cardinality lower than highest existing delta causes data loss upon save, as this would give us a very easy way to check if there are any entities with a delta > the one the user is trying to limit the field to.

Committed 7109454 and pushed to 8.2.x. Thanks!

Personally I think we should consider committing this as is to aloow for an easy fix to #1266748: Changing cardinality lower than highest existing delta causes data loss upon save. The ensureFieldTable() is protected and work arounds are likely to be complex.

diff --git a/core/tests/Drupal/KernelTests/Core/Entity/EntityQueryTest.php b/core/tests/Drupal/KernelTests/Core/Entity/EntityQueryTest.php
index b4ce0a0..d7a160a 100644
--- a/core/tests/Drupal/KernelTests/Core/Entity/EntityQueryTest.php
+++ b/core/tests/Drupal/KernelTests/Core/Entity/EntityQueryTest.php
@@ -563,12 +563,12 @@ public function testDelta() {
     // Numeric delta on single value base field should return results only if
     // the first item is being targeted.
     $this->queryResults = $this->factory->get('entity_test_mulrev')
-      ->condition("id.0.value", array(1,3,5), 'IN')
+      ->condition("id.0.value", array(1, 3, 5), 'IN')
       ->sort('id')
       ->execute();
     $this->assertResult(1, 3, 5);
     $this->queryResults = $this->factory->get('entity_test_mulrev')
-      ->condition("id.1.value", array(1,3,5), 'IN')
+      ->condition("id.1.value", array(1, 3, 5), 'IN')
       ->sort('id')
       ->execute();
     $this->assertResult();
@@ -576,18 +576,18 @@ public function testDelta() {
     // Delta range condition on single value base field should return results
     // only if just the field value is targeted.
     $this->queryResults = $this->factory->get('entity_test_mulrev')
-      ->condition("id.%delta.value", array(1,3,5), 'IN')
+      ->condition("id.%delta.value", array(1, 3, 5), 'IN')
       ->sort('id')
       ->execute();
     $this->assertResult(1, 3, 5);
     $this->queryResults = $this->factory->get('entity_test_mulrev')
-      ->condition("id.%delta.value", array(1,3,5), 'IN')
+      ->condition("id.%delta.value", array(1, 3, 5), 'IN')
       ->condition("id.%delta", 0, '=')
       ->sort('id')
       ->execute();
     $this->assertResult(1, 3, 5);
     $this->queryResults = $this->factory->get('entity_test_mulrev')
-      ->condition("id.%delta.value", array(1,3,5), 'IN')
+      ->condition("id.%delta.value", array(1, 3, 5), 'IN')
       ->condition("id.%delta", 1, '=')
       ->sort('id')
       ->execute();

Fixed on commit.

Moving back to 8.1.x for review there.

While this changes a protected method, there's nothing in semver that says you can't change protected methods in patch releases - it just says you should define your public API and stick to it whether in a patch or minor release.

Our own bc policy says that we avoid making changes to the non-public API/internal things in minor releases - this is our own choice and largely exists due to the large surface area of that non-public API.

If this is a blocker for [#1266748 (and it looks like it is) then I think it's OK to make the change in a patch release - should have a change record though. However if it did result in actual breakage somewhere, we could also only fix #1266748: Changing cardinality lower than highest existing delta causes data loss upon save in 8.2.x

Also thinking about it more, postponing the 8.1.x commit for this on the 8.2.x commit of #1266748: Changing cardinality lower than highest existing delta causes data loss upon save - they can go into 8.1.x together assuming it works out.

Looks like it's working out :)

Re-uploading the patch so it runs against 8.1.x

The test is green. Get back to RTBC.

Committed 2116b91 and pushed to 8.1.x. Thanks!

I encountered a PHP notice related to the committed code.
@see #2772531: Notice: Undefined offset in Drupal\Core\Entity\Query\Sql\Tables->addField()