Entity fields do not support case sensitive queries

I also prefer the operation solution than adding a new parameter to the condition() method to allow configuration.

I've discussed this with @chx on IRC and concluded that from my point of view, the problem as described sounds significant, but is difficult/untestable due to mysql only testbots. And should be addressed before release - I think the status of major is correct. I believe that the only changes will be API additions.

@@ -0,0 +1,49 @@
+ *   id = "string_field",

Wrong id.

@@ -109,6 +111,15 @@ public function addField($field, $type, $langcode) {
+        $entity = entity_create($entity_type, $values);
+        $propertyDefinitions = $entity->{$field['field_name']}->getPropertyDefinitions();

Hm, BC mode getting in the way again?

(The bc decorator removal patch for nodes is green, which means this can go away soon! Please review)

If I understand correctly, the exec summary here is "document what MySQL is already doing as the API, and then add code to make other DB backends behave like MySQL already is". Is that an accurate summary? If so, I'm OK with that in concept.

1. +++ b/core/lib/Drupal/Core/Database/Query/SelectInterface.php
   @@ -124,6 +124,13 @@ public function &getTables();
      /**
   +   * Returns the connection object this SELECT query works on.
   +   *
   +   * @return \Drupal\Core\Database\Connection
   +   */
   +  public function getConnection();
   +
   +  /**
   

   I see why this is being added. I'd really like to see if there's an alternative, though. This just feels wrong to me to expose an internal detail. I don't know what the alternative is off hand (after midnight, brain shut down), but I'd like to at least try to find one.

2. +++ b/core/lib/Drupal/Core/Entity/Plugin/DataType/BinaryStringItem.php
   @@ -0,0 +1,49 @@
   +class BinaryStringItem extends FieldItemBase {
   

   Being a DB-savvy guy, I get that "binary" in this case means "stored using a BLOB field in the DB so behaves in a case-sensitive fashion". To the the entire world not counting the dozen or so DB-sensitive people around, "Binary String" likely means "Legal values are 'Yes' and 'No'". We probably want a better name here. And then for the "binary" property, too.

3. +++ b/core/lib/Drupal/Core/Entity/Plugin/DataType/BinaryStringItem.php
   @@ -0,0 +1,49 @@
   +        'binary' => TRUE,
   

   My knee-jerk thought here is that you shouldn't make this a flag but an interface on the class. Then you can do an instanceof CaseSensitiveInterface or something.

Thanks, chx. I'm not RTBCing because I don't know EFQ well enough to vet those parts, but the DBTNG parts look good to me now.

One other nit:

+++ b/core/lib/Drupal/Core/Entity/Query/Sql/Condition.php
@@ -68,23 +68,38 @@ public function notExists($field, $langcode = NULL) {
+  protected function translateCondition(&$condition, SelectInterface $sqlQuery) {

$sqlQuery should be $sql_query. It's a local variable, not an object property. (This is the case in a few places.)

Nitpick review incoming...

+++ b/core/lib/Drupal/Core/Database/Query/Select.php
@@ -353,6 +353,10 @@ public function &getUnion() {
 
+  public function escapeLike($string) {
+    return $this->connection->escapeLike($string);
+  }

+++ b/core/lib/Drupal/Core/Database/Query/SelectExtender.php
@@ -171,6 +171,10 @@ public function &getUnion() {
+  }

Missing @inheritdoc.

+++ b/core/lib/Drupal/Core/Database/Query/SelectInterface.php
@@ -124,6 +124,18 @@ public function &getTables();
+   * Escapes characters that work as wildcard characters in a LIKE pattern.

+++ b/core/lib/Drupal/Core/Database/Query/SelectInterface.php
@@ -124,6 +124,18 @@ public function &getTables();
+   *
+   * @return
+   *   The escaped string.
+   * @see \Drupal\Core\Database\Connection::escapeLike

+++ b/core/lib/Drupal/Core/Entity/Plugin/DataType/CaseSensitiveStringItem.php
@@ -0,0 +1,49 @@
+ *   id = "case_sensitive_string_field",
+ *   label = @Translation("Binary string field item"),
+ *   description = @Translation("An entity field containing a binary string (case sensitive) value."),

+++ b/core/lib/Drupal/Core/Entity/Plugin/DataType/CaseSensitiveStringItem.php
@@ -0,0 +1,49 @@
+   * Implements \Drupal\Core\TypedData\ComplexDataInterface::getPropertyDefinitions().

+++ b/core/lib/Drupal/Core/Entity/Query/QueryInterface.php
@@ -57,12 +57,24 @@ public function getEntityType();
+   *   When $value is a string the behavior depends on the field definition

+++ b/core/lib/Drupal/Core/Entity/Query/Sql/ConditionAggregate.php
@@ -7,6 +7,7 @@
 
+use Drupal\Core\Database\Connection;
 use Drupal\Core\Database\Query\SelectInterface;

Should we also handle case sensitivity for config entity query in this issue or should I open another one?

Try this code in the standard profile which should have a view named 'Content':

$query = \Drupal::entityQuery('view');
$query->condition('label', 'C', 'CONTAINS');
$result = $query->execute();
debug($result);

1. +++ b/core/lib/Drupal/Core/Entity/Query/Sql/Condition.php
   @@ -30,20 +30,20 @@ public function compile($conditionContainer) {
   +        $sqlCondition->sqlQuery = $sql_query;
   

   You may want to change also the $sqlCondition variable to $sql_condition for consistency.

2. +++ b/core/lib/Drupal/Core/Entity/Query/Sql/Tables.php
   @@ -109,6 +113,20 @@ public function addField($field, $type, $langcode) {
   +          $propertyDefinitions = $entity->getNGEntity()->{$field['field_name']}->getPropertyDefinitions();
   

   This patch definitely needs a reroll :)

This a per-column property, not a per-class property so, sorry, we can't do that.

We can do interface, as have classes for those properties also, e.g. lookup "string". You can access the object by doing $item->get('value'). Also, we could do a new data type in addition to "string".

One idea that came to my mind was going with "string" for the binary variant and "text" for the other one, as the collation enabled matching should apply to all written text, right? Does that make sense or does it sound too arbitrary? Of course it would have to be documented on the respective classes/interfaces.

"case sensitive" sounds wrong to me, as it's not only about the case, but the whole collation thing right?

Updated issue summary.

#2068329: Convert user SQL queries to the Entity Query API exposes this bug for non MySQL databases, so bumping to critical.

Attempted to reroll this, let's see how it goes.

46: 2068655-typed-data-case-sensitive-46.patch queued for re-testing.

+++ b/core/lib/Drupal/Core/Entity/Query/Sql/TablesInterface.php
@@ -15,13 +15,14 @@
+   * @param array &$field_definition

no need to add &

Reroll.

Corrected some flaws introduced by me in #50

OptionsFieldUITest now passes locally - lets see what testbot makes of the others.

ForumUninstallTest now works ( with more of my mangling removed )

@chx Thanks that made me see my error!

In local tests exceptions removed.

PS applied fix from #48

Patch needs to be updated for #2247991: [May 27] Move all module code from …/lib/Drupal/… to …/src/… for PSR-4 (it also does not apply before that change).

+++ b/core/lib/Drupal/Core/Entity/Plugin/DataType/CaseSensitiveStringItem.php
@@ -0,0 +1,49 @@
+class CaseSensitiveStringItem extends FieldItemBase {

I'm not sure why this adds a separate field item for strings while it adds a setting for text items. I think we should the same for both, i.e. add a setting.

Attempt at a reroll. I believe this preserves the intended functionality. Def needs review.

triggering testbot

My mistake see below

I can see files in the patch with was following structure ...

/core/lib/Drupal/Core/Database/Connection.php

here are the instructions for rerolling psr4 patches

https://groups.drupal.org/node/424758

Those are Drupal\Core classes, they still use the lib folder

Let's see how this gets on.

Also, I think we might want to think about the fact that in field settings we are using 'case_sensitive' and in the query conditions we are using 'case sensitive'.

May not be the right solution, but it should get the tests to run.

+          if ($entity->getFieldDefinition($field_name)) {
+            $propertyDefinitions = $entity->getFieldDefinition($field_name)->getPropertyDefinitions();
+          }

I don't know if this is the right solution, but it could be written this way:

+          if ($definition = $entity->getFieldDefinition($field_name)) {
+            $propertyDefinitions = $definition->getPropertyDefinitions();
+          }

Which avoid calling getFieldDefinition() and its logic twice.
But that's a really tiny nitpick.

No opinion on the approach atm, just dumb code remarks :

1. +++ b/core/lib/Drupal/Core/Entity/Plugin/DataType/CaseSensitiveStringItem.php
   @@ -0,0 +1,49 @@
   +class CaseSensitiveStringItem extends FieldItemBase {
   

   This FieldItem class provides no schema() ?? This stores nothing ?

2. +++ b/core/modules/text/src/Plugin/Field/FieldType/TextItem.php
   @@ -15,7 +15,14 @@
   + *   settings = {
   + *     "max_length" = "255",
   + *     "case_sensitive" = "0"
   + *   },
   + *   instance_settings = {
   + *     "text_processing" = "0"
   + *   },
   
   +++ b/core/modules/text/src/Plugin/Field/FieldType/TextLongItem.php
   @@ -16,6 +16,12 @@
   + *   settings = {
   + *     "case_sensitive" = "0"
   + *   },
   + *   instance_settings = {
   + *     "text_processing" = "0"
   + *   },
   
   +++ b/core/modules/text/src/Plugin/Field/FieldType/TextWithSummaryItem.php
   @@ -17,6 +17,13 @@
   + *   settings = {
   + *     "case_sensitive" = "0"
   + *   },
   + *   instance_settings = {
   + *     "text_processing" = "1",
   + *     "display_summary" = "0"
   + *   },
   

   Settings now are defined in static methods, not in the annotation.

3. +++ b/core/vendor/symfony/http-foundation/Symfony/Component/HttpFoundation/Session/Storage/Proxy/AbstractProxy.php
   @@ -120,7 +120,7 @@ public function getId()
   -            throw new \LogicException('Cannot change the ID of an active session');
   +            //throw new \LogicException('Cannot change the ID of an active session');
   

   Debug leftover

I have no idea if this will work properly, we'll see.

Cleanup and test fixes.

The patch matches the description in the issue summary.

Overall it looks great to me.

Question: That there have been missing escapeLike calls, would that have any consequences for Drupal 7, too? From this patch?

Found two things that I think we can remove now. That should give us some more integration test coverage, for example for the binary flag of varchar fields.

Also added the setting and schema logic to the new string field types.

Fixing config schema, string field types don't have any schema, various other core field types are probably missing it too.

Rerolled added the missing tests.

The schema exception is trigged because StringLongItem inherits the max length setting from StringItem. Found another bug during debugging. The storage settings for StringItem are hidden because the method name is wrong. Filled #2380391: Fix storage settings StringLongItem to fix both bugs.

Yeah, the patch from #2380391: Fix storage settings StringLongItem got in and resolved the schema exception.

Rerolled, merged config schema and removed LIKE/db_like(). Tests still pass.

@chx: db_like() allows to escape part of the LIKE condition, you can say something like '%' . db_like($user_string) . '%fixedstring'. We have CONTAINS/STARTS_WITH and so on instead to support a limited subset of what you could do with a LIKE. I think those are already case insensitive automatically as the implementation uses LIKE there obviously, so we should be good? Can you confirm that?

I've reviewed the patch. Imo, the approach is architecturally sound, I found a few glitches in the code though:

1. +++ b/core/lib/Drupal/Core/Database/Query/SelectExtender.php
   @@ -171,6 +171,10 @@ public function &getUnion() {
   +  public function escapeLike($string) {
   

   Missing docs.

2. +++ b/core/lib/Drupal/Core/Entity/Query/Sql/Tables.php
   @@ -144,6 +146,9 @@ public function addField($field, $type, $langcode) {
   +          $field_definition['case sensitive'] = $propertyDefinitions[$column]->getSetting('case_sensitive');
   

   Here it checks the field item property definition setting - what seems to be fine.

3. +++ b/core/lib/Drupal/Core/Entity/Query/Sql/Tables.php
   @@ -198,9 +203,10 @@ public function addField($field, $type, $langcode) {
   +        $field_definition['case sensitive'] = !empty($schema['fields'][$property]['binary']);
   

   But here it checks the schema? It should check the field item property definition as well.

4. +++ b/core/lib/Drupal/Core/Entity/Query/Sql/TablesInterface.php
   @@ -15,13 +15,14 @@
   +   *   - langcode: The language code the field values are to be shown in.
   

   The langcode is not about display, but about query only right? So the docs are wrong.

5. +++ b/core/modules/file/src/FileStorageSchema.php
   @@ -28,13 +28,6 @@ protected function getSharedTableFieldSchema(FieldStorageDefinitionInterface $st
   -        case 'uri':
   -          $this->addSharedTableFieldUniqueKey($storage_definition, $schema, TRUE);
   -          // @todo There should be a 'binary' field type or setting:
   

   This (wrongly) removes the unique key index for uri?

As discussed, some refactoring on how $case_sensitive is passed around. I also don't see how base fields worked at all...

Restored the unique column for files.

Ah, that actually hid the fact that file.uri did *not* work ;)

This fixes it and provides more test coverage, also fixed the other tests, just using a different variable name to avoid the conflict there, don't want to go into details there.

Just a quick look:

1. +++ b/core/lib/Drupal/Core/Entity/Query/Sql/Condition.php
   @@ -70,24 +71,44 @@ public function notExists($field, $langcode = NULL) {
   +  protected function translateCondition(&$condition, SelectInterface $sql_query, $case_sensitive) {
   
   +++ b/core/lib/Drupal/Core/Entity/Query/Sql/ConditionAggregate.php
   @@ -64,22 +66,42 @@ public function notExists($field, $function, $langcode = NULL) {
   +  protected function translateCondition(&$condition, SelectInterface $sql_query, $case_sensitive) {
   

   Sorry if this has just been discussed, but why doesn't $case_sensitive default to FALSE here?

2. +++ b/core/lib/Drupal/Core/Entity/Query/Sql/Query.php
   @@ -277,6 +277,7 @@ protected function getSqlField($field, $langcode) {
   +      $case_sensitive = FALSE;
          return $this->tables->addField($field, 'LEFT', $langcode);
   

   Is this being used? Are we missing test coverage?

3. +++ b/core/lib/Drupal/Core/Entity/Query/Sql/Tables.php
   @@ -144,6 +144,9 @@ public function addField($field, $type, $langcode) {
   +        if (isset($propertyDefinitions[$column])) {
   +          $case_sensitive = $propertyDefinitions[$column]->getSetting('case_sensitive');
   +        }
          }
          // This is an entity base field (non-configurable field).
          else {
   @@ -160,6 +163,15 @@ public function addField($field, $type, $langcode) {
   
   @@ -160,6 +163,15 @@ public function addField($field, $type, $langcode) {
            $entity_tables[$entity_base_table] = $this->getTableMapping($entity_base_table, $entity_type_id);
            $sql_column = $specifier;
            $table = $this->ensureEntityTable($index_prefix, $specifier, $type, $langcode, $base_table, $entity_id_field, $entity_tables);
   +
   +        if ($field_storage) {
   +          $column = $field_storage->getMainPropertyName();
   +          $base_field_property_definitions = $field_storage->getPropertyDefinitions();
   +          if (isset($base_field_property_definitions[$column])) {
   +            $case_sensitive = $base_field_property_definitions[$column]->getSetting('case_sensitive');
   +          }
   +        }
   +
          }
   

   Cannot tell from the patch: are these two branches handling single column field vs multiple column fields? And if so are we supporting case sensitivity for all columns in the IF branch?

4. +++ b/core/lib/Drupal/Core/Entity/Query/Sql/TablesInterface.php
   @@ -20,8 +20,11 @@
   +   *   (optional) Allows the implementation to decide if the condition for this field
   +   *   needs to be case sensitive or not.
   

   Comment does not wrap correctly.

1. Because it's an internal method that we only call in one place, so it doesn't really matter :) And there we have to define it first anyway.

2. The $case_sensitive = FALSE is a left-over from when there was no default value.

3. As discusses, yes, right now, entity query only supports querying base fields on the main property, opened: #2391217: Support base fields with multiple columns in entity queries

4. Fixed.

Ok, this was fun.

So I was writing tests for STARTS_WITH, ENDS_WITH and CONTAINS, and everything passed except one CONTAINS check.

So I started debugging it and found three nested bugs why it never worked in the way we think it did:

1. $propertyConditions only gets loaded when you write something like fieldname.value, so it was never loaded at least in the test and we never updated $case_sensitive.
2. Then, we checked the setting on the property definition, but we set it only on the field. So I changed the field types to pass through the setting to the relevant property.
3. Third, the translateConditions() method had a case '=', but $operator can be NULL at that point if it was not provided, so we never went into that condition unless you manually pass '=' in.

So why did the test pass you ask? Because we set binary on the schema, and then MySQL does it the way we expect to automatically, assuming you use a case insensitive collation (and if you do not, then all of this is also bogus, because it will always be case sensitive).

I also updated the string and text field types as they haven't been updated to the format changes:
- Removed the option from TextItemLong and summary as a blob/case sensitive storage with a text format seems highly unlikely.
- Moved the default setting to StringItemBase

I also updated translateConditions() a bit, we always need to call escapeLike() there.

PS: Oh, btw, my test only failed because I had a typo in the field name and use the wrong field in one case. So that's the only reason I found all this.

Untested test-only patch.

+++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldType/StringItemBase.php
@@ -20,11 +20,21 @@
+  public static function defaultStorageSettings() {
+    return array(
+      'case_sensitive' => FALSE,
+    ) + parent::defaultStorageSettings();
+  }

+++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldType/UriItem.php
@@ -33,6 +33,7 @@ class UriItem extends StringItem {
   public static function defaultStorageSettings() {
     return array(
       'max_length' => 2048,
+      'case_sensitive' => FALSE,
     ) + parent::defaultStorageSettings();

Moving this setting to StringItemBase means UriItem field type now has a 'case_sensitive' setting too ?
- That does not seem to make real sense for the uri field type, does it ?
- I don't see the corresponding config schema entry for field.storage_settings.uri ?

UriItem actually extends from StringItem, so it already have, but I did remove the default setting. The schema is there, as discussed.

PS: Oh, btw, my test only failed because I had a typo in the field name and use the wrong field in one case. So that's the only reason I found all this.

:D

Reviewed it. Patch looks good in general.

1.    * @param bool &$case_sensitive
      *   (optional) Allows the implementation to decide if the condition for this
      *   field needs to be case sensitive or not.

   That's quite confusing if you discover it. Can I now control (=pass) it or not? Maybe we can clarify better that the caller cannot control whether something is to be queried case sensitive or not.

         // @todo There should be a 'binary' field type or setting:
          //   https://www.drupal.org/node/2068655.
          $schema['fields'][$field_name]['binary'] = TRUE;

There is, so that's deprecated?

  protected function buildPropertyQuery(QueryInterface $entity_query, array $values) {
    if (isset($values['name'])) {
      $entity_query->condition('name', $values['name'], 'LIKE');
      unset($values['name']);
    }
    parent::buildPropertyQuery($entity_query, $values);
  }

Should be deprecated? If not, needs a comment why it's needed + docs.

  public static function propertyDefinitions(FieldStorageDefinitionInterface $field_definition) {
    $properties = parent::propertyDefinitions($field_definition);
    $properties['value']->setSetting('case_sensitive', $field_definition->getSetting('case_sensitive'));
    return $properties;
  }

Deprecated addition - no text field support any more? (+1 on that)

+++ b/core/lib/Drupal/Core/Entity/Query/Sql/Condition.php
@@ -70,24 +71,60 @@ public function notExists($field, $langcode = NULL) {
-  protected function translateCondition(&$condition) {
+  public static function translateCondition(&$condition, SelectInterface $sql_query, $case_sensitive) {

Making this public worries me a little bit, as it might make future changes more problematic. But in the end it should be about the same as a protected method as it's not in the interface, so ok.

6. +++ b/core/lib/Drupal/Core/Entity/Query/Sql/Tables.php
   @@ -59,7 +59,7 @@ public function __construct(SelectInterface $sql_query) {
   -  public function addField($field, $type, $langcode) {
   +  public function addField($field, $type, $langcode, &$case_sensitive = FALSE) {
   

   Is there documentation for the added parameter somewhere?

7. +   * @param bool &$case_sensitive
   +   *   (optional) Allows the implementation to decide if the condition for this
   +   *   field needs to be case sensitive or not.

   Oh it's in the interface - although it is supposed to be internal?

+++ b/core/includes/database.inc
@@ -527,6 +527,14 @@ function db_escape_field($field) {
+ * Drupal considers LIKE case insensitive and the following is often used
+ * to tell the database that case insensitive equivalence is desired:
+ * @code
+ * db_select('users')
+ *  ->condition('name', db_like($name), 'LIKE')
+ * @endcode
+ * Use 'LIKE BINARY' instead of 'LIKE' for case sensitive queries.

Sounds good, but as berdir explained to me that depends on the schema to work. So that needs to be documented here then. This sounds like a flaw in the database abstraction layer to me as it depends on the database (mysql/postgre) on whether like is case-sensitive or not. If so, shouldn't this be fixed in the database layer?

9. +++ b/core/lib/Drupal/Core/Database/Connection.php
   @@ -844,6 +844,14 @@ public function escapeAlias($field) {
   +   * Drupal considers LIKE case insensitive and the following is often used
   +   * to tell the database that case insensitive equivalence is desired:
   +   * @code
   +   * $this->connection->select('users')
   +   * ->condition('name', $this->connection->escapeLike($name), 'LIKE')
   +   * @endcode
   +   * Use 'LIKE BINARY' instead of 'LIKE' for case sensitive queries.
   

   Maybe we can do the docblock only once and point to the single one from here instead?

More fun when debugging this. The condition for $case_sensitive was the wrong way round, so it was still not correct ;)

1. Discussed with alexpott, I think we found a better API. There is a separate method now that you can call. I also made it TRUE/FALSE/NULL, so that we only change it to LIKE if we know that the field type is explicitly not case sensitive (so that we don't use LIKE on integer fields ;))
2./3.: That's why the patch is removing those ;)
4. Yeah, I left it there but agreed that we don't need it either.
5. Yes, we could also make it a trait, that's a bit more code that needs to change ;)
6. It's the same parameter that you reviewed in 1, which is now gone.
7. Still the same thing as 1. What did you do exactly ;p
8. Updated the documentation as discussed. I don't know what else we can do and @alexpott is OK with this.

Note that the interdiff is against #113, but I did include @chx's addition for LIKE BINARY in the new documentation.

This looks great to me - this should be ready given it comes back green. Just found a small issue:

+++ b/core/lib/Drupal/Core/Entity/Query/Sql/Condition.php
@@ -70,24 +70,62 @@ public function notExists($field, $langcode = NULL) {
+   * @param bool $case_sensitive
+   *   If the condition should be case sensitive or not.

This really is bool|null.

+++ b/core/lib/Drupal/Core/Database/Query/ConditionInterface.php
@@ -24,6 +24,19 @@
+   * collation. if the field is set to binary, then a LIKE condition will also

Very, very small nitpick: "if the field is ..." with capitalized "I".

Removed the setting from the migration again.

Let's do this

This looks great. Just a very small thing that can be improved with docs.

+++ b/core/lib/Drupal/Core/Entity/Query/Sql/Condition.php
@@ -70,24 +70,63 @@ public function notExists($field, $langcode = NULL) {
+        if ($case_sensitive === FALSE) {
+          $condition['value'] = $sql_query->escapeLike($condition['value']);
+          $condition['operator'] = 'LIKE';
+        }
+        break;
+      case '<>':
+        if ($case_sensitive === FALSE) {
+          $condition['value'] = $sql_query->escapeLike($condition['value']);
+          $condition['operator'] = 'NOT LIKE';
+        }
+        break;

I think we should have a bit more documentation for why there is no else for to cover the case sensitive code path. It took me a minute or two to get it.

This is the best I could come up with. Wasn't sure about documenting the other cases, but I think they're pretty clear.

Also removed the weird case '' and added a @see for the new LIKE/LIKE BINARY docs.

Looks good to me

Yep, changes look good - thanks.

Thanks for the docs improvements. This issue addresses a critical bug and is allowed per https://www.drupal.org/core/beta-changes. Committed 88a46ae and pushed to 8.0.x. Thanks!