PostgreSQL: Fix taxonomy\Tests\TermTest

The tests don't want to ignore it, the tests make sure that it isn't case sensitive. Term names are considered case insensitive.

We treat string/text fields (entity fields) as case insensitive by default, unless specified otherwise.

See StringItemBase::defaultStorageSettings(): 'case_sensitive' => FALSE

The binary flag is set based on this (in String::schema() for example), mostly for mysql I think, *and* entity query's Tables.php checks that to mark those fields as case insensitive, which results in a corresponding operator in Condition::translateCondition()

Sounds like something there isn't working as it needs for PostgreSQL?

EntityQueryTest should cover all the combinations that there are, surprised that doesn't fail then? We have #2443657: PostgreSQL: Fix system\Tests\Entity\EntityQueryTest, but that's about sort, apparently?

Maybe we could add a query tag to those queries specifically so drivers can address them?

This patch is a subset of the patch from #2464481-31: PostgreSQL: deal with case insensitivity. Because the subset of the original patch does not fixes case insensitivity functionality, but it does fixes this issue, I have posted it here.

There is also the problem that we have until the first of July to get PostgreSQL to pass all the tests. So I do not want to wait to get #2464481: PostgreSQL: deal with case insensitivity completely fixed.

The tests that failed for the original patch. Do not fail for this patch. These are:

• Drupal\aggregator\Tests\ImportOpmlTest
• Drupal\comment\Tests\CommentLanguageTest
• Drupal\system\Tests\Entity\EntityDefinitionUpdateTest
• Drupal\system\Tests\Entity\EntityQueryTest

@bzrudi71: Can you test this patch with your testbot to see if there are so unforeseen problems?

I'm concerned that we're not able to make changes to driver itself to be able to fix this. Changing core/lib/Drupal/Core/Entity/Query/Sql/Condition.php feels wrong. Isn't #2464481: PostgreSQL: deal with case insensitivity the correct fix here?

I agree with alexpott that we should not make database specific changes to code that is not in the specific database driver.
The problem is that the database driver has to be able to change all of the SQL statements that are generated by Drupal. And not all code that generates SQL statements is in the database driver. As with core/lib/Drupal/Core/Entity/Query/Sql/Condition.php this class generates SQL statements. The SQL statements in this class are MySQL specific. Any other database that works a (little) different then MySQL has a problem.
If you want to make a database driver for Microsoft SQL server, Oracle DB, a no-SQL database like MongoDB or a cloud specific database you have a big problem. You cannot change what SQL statements are generated. The problem also exists for db_query() and views SQL. My idea is to create a new issue for moving all SQL statement generating code to the database drivers. I do not know for what Drupal version this would be (8.0, 8.1 or 9.0) or with what priority.

The problem with fixing #2464481: PostgreSQL: deal with case insensitivity is that you need a testbot with a PostgreSQL database. There is work being done for such a testbot, but it is not here in time for the July first deadline.

So if we want to get #2157455: [Meta] Make Drupal 8 work with PostgreSQL or remove support from core before release in before the July first deadline I see no other way then with the solution offered by the current patch.

I have talked to alexpott on IRC. Adding a PostgreSQL exception to the file core/lib/Drupal/Core/Entity/Query/Sql/Condition.php is for him totally not acceptable. And if we want to be able to let database drivers in contrib to also make changes to this file then we should

create a class called EntityQueryCondition.php in the database driver with two static methods and the override this class in PostgreSQL

.

First try in the way that alexpott wants it.

+++ b/core/lib/Drupal/Core/Entity/Query/Sql/Condition.php
@@ -28,6 +29,10 @@ class Condition extends ConditionBase {
+    $db_type = Database::getConnection()->databaseType();
+    $entityQueryCondition = "Drupal\\Core\\Database\\Driver\\{$db_type}\\EntityQueryCondition";

I think we can use Database::getConnection()->getDriverClass('EntityQueryCondition') here. And it'd be nice if the it was EntityQuery\Condition.

Second try. See if it passes the testbot.

@alexpott: Thank you for your review.

Added the Database::getConnection()->getDriverClass('EntityQueryCondition').

Todo: And it'd be nice if the it was EntityQuery\Condition.

+++ b/core/lib/Drupal/Core/Entity/Query/Sql/Condition.php
@@ -28,6 +29,8 @@ class Condition extends ConditionBase {
+    $entityQueryCondition = Database::getConnection()->getDriverClass('EntityQueryCondition');

I think we should namespace further EntityQuery\Condition

We do this in system.install...

$class = Database::getConnection()->getDriverClass('Install\\Tasks');

Added the EntityQuery\Condition change.

I think this may require a change record if committed after 2015.07.24 per https://groups.drupal.org/node/472163.

It may also be nice to have a change record because this does change the API by adding to the API.

Does this need any additional sign-offs?

1. @daffie nice work - this is exactly how I think this should work - DB specific code in the the driver - lovely!

2. +++ b/core/lib/Drupal/Core/Database/Driver/mysql/EntityQuery/Condition.php
   @@ -0,0 +1,12 @@
   +<?php
   +
   +/**
   + * @file
   + * Definition of Drupal\Core\Database\Driver\mysql\EntityQuery\Condition
   + */
   +
   +namespace Drupal\Core\Database\Driver\mysql\EntityQuery;
   +
   +use Drupal\Core\Database\EntityQuery\Condition as EntityQueryCondition;
   +
   +class Condition extends EntityQueryCondition { }
   
   +++ b/core/lib/Drupal/Core/Database/Driver/sqlite/EntityQuery/Condition.php
   @@ -0,0 +1,12 @@
   +<?php
   +
   +/**
   + * @file
   + * Definition of Drupal\Core\Database\Driver\sqlite\EntityQuery\Condition
   + */
   +
   +namespace Drupal\Core\Database\Driver\sqlite\EntityQuery;
   +
   +use Drupal\Core\Database\EntityQuery\Condition as EntityQueryCondition;
   +
   +class Condition extends EntityQueryCondition { }
   

   I don't think these empty classes are required. I think it'll fallback to using the default class.

3. +++ b/core/lib/Drupal/Core/Database/Driver/pgsql/EntityQuery/Condition.php
   @@ -0,0 +1,47 @@
   + * @file
   + * Definition of Drupal\Core\Database\Driver\pgsql\EntityQuery\Condition
   

   Contains instead of Defininition of

4. +++ b/core/lib/Drupal/Core/Database/Driver/pgsql/EntityQuery/Condition.php
   @@ -0,0 +1,47 @@
   +class Condition extends EntityQueryCondition {
   

   There is no need for the extends here and I think we should have an interface as well. When we implement the interface it'd be nice if we type hinted the parameters on the methods too.

5. +++ b/core/lib/Drupal/Core/Database/EntityQuery/Condition.php
   @@ -0,0 +1,39 @@
   +  public static function translateCondition(&$condition, $case_sensitive) {  }
   

   Let's document that this is a no-op and why.

6. +++ b/core/lib/Drupal/Core/Database/EntityQuery/Condition.php
   @@ -0,0 +1,39 @@
   +  /**
   +   * Adds the condition to the condition container.
   +   *
   +   * @param $query
   +   *   The query object this conditional clause belongs to.
   +   * @param string|\Drupal\Core\Entity\Query\ConditionInterface $field
   +   * @param array $condition
   +   *   The condition array.
   +   */
   

   These @param's don't look quite right. Also I think this should be on a interface. See above.

@bzrudi71: That would be great. I will review your changes. Can you review #2477853: PostgreSQL: Add support for reserved field/column names?

@alexpott: Thank you for you review. It really helps to get this issue fixed. :-)

Fixed the way alexpott wants it to.
For #32.2: Removed the parent-class (Drupal\Core\Database\EntityQuery\Condition), because the child-classes cannot be removed (class not found exceptions). The other classes in the database driver uses parent-classes and empty child-classes. For me that would be the better solution (no double code).
For me it is RTBC.

@bzrudi71: Good work!

Sorry I've just realised that we should have explicit tests for this in EntityQueryTest because taxonomy\Tests\TermTest could change and then we'd lose our coverage.

@alexpott: What about:

For #32.2: Removed the parent-class (Drupal\Core\Database\EntityQuery\Condition), because the child-classes cannot be removed (class not found exceptions). The other classes in the database driver uses parent-classes and empty child-classes. For me that would be the better solution (no double code).

Is it not better to fix the testing in #2464481: PostgreSQL: deal with case insensitivity?

It would be better to add the tests related to changes to EntityQuery when the change is being made rather than later.

I think we're required to have a change record now as it's the 6/24.

Will the additional class be picked up automagically by the class loader on existing beta sites, or do we need to do a cache-rebuild in update.php now as well?

Edit: June is not the 7th month of the year. :D

@mradcliffe The additional call will be picked up automagically.

@daffie the tests are to prove we don't regress unexpectedly in this area - it's important.

It took me a little bit to get the test data correct (data provider via bits hah), but I added tests for both testing IN query and (not) case sensitive IN queries to EntityQueryTest test cases.

Let's see how mysql likes this. PostgreSQL passes for me. Will need to run through sqlite as well.

Also, is there a reason that the case sensitive tests get the service directly from Drupal object instead of the factory method that the other tests use? It looks like that was done in #2068655: Entity fields do not support case sensitive queries, but the factory method use came from 2013 or so.

I wrote a draft change record here: https://www.drupal.org/node/2511626

I can confirm that the test passes with SQLIte.

I will a longer review later.

Are you sure you want to add /core/lib/Drupal/Core/Database/Driver/pgsql/Condition.php this file? It is not in your interdiff-file.

@daffie nice spot - that looks completely extraneous :)

So there is something that has always bothered me about this patch...

+++ b/core/lib/Drupal/Core/Database/Driver/pgsql/EntityQuery/Condition.php
@@ -0,0 +1,50 @@
+    // For PostgreSQL all the condition arguments need to have case
+    // lowered to support not case sensitive fields.
+    if (is_array($condition['value']) && $case_sensitive === FALSE) {
+      $condition['where'] = 'LOWER(' . $condition['real_field'] . ') ' . $condition['operator'] . ' (';
+      $condition['where_args'] = [];
+
+      $n = 1;
+      foreach ($condition['value'] as $key => $value) {
+        $condition['where'] .= 'LOWER(:value' . $n . '),';
+        $condition['where_args'][':value' . $n] = $value;
+        $n++;
+      }
+      $condition['where'] = trim($condition['where'], ',');
+      $condition['where'] .= ')';
+    }

So we do all this munging of the condition here... why do we also have to do the work in addConditionToContainer?

I have reviewed the patch from #50. It looks good with one exceptions:

+++ b/core/modules/system/src/Tests/Entity/EntityQueryTest.php
@@ -210,6 +210,15 @@ function testEntityQuery() {
+    // An entity might have either red or blue figure.
+    $query = $this->factory->get('entity_test_mulrev');
+    $this->queryResults = $this->factory->get('entity_test_mulrev')
+      ->condition("$figures.color", array('blue', 'red'), 'IN')
+      ->sort('id')
+      ->execute();
+    // Bit 0 or 1 is on.
+    $this->assertResult(1, 2, 3, 5, 6, 7, 9, 10, 11, 13, 14, 15);
+

Why is this test necessary for this issue? And if we do need this. What is the variable $query for, it is not used anywhere!

The other tests are the ones that we need for this patch. And they look good to me.

The tests and the change record are added.

@bzrudi71: Can you test your patch also with a PostgreSQL and a SQLite database.

Are you sure you want to add /core/lib/Drupal/Core/Database/Driver/pgsql/Condition.php this file? It is not in your interdiff-file.

Thanks for catching this, daffie. I had a file that I was working on a few weeks ago to try and do stuff in the database driver. When I applied your patch, I did git add and added that one in too since it was still lying around untracked by git. And then the interdiff created with git diff didn't catch it.

@bzrudi71: It looks good to me. If you can say both bots with this patch passes it will get a RTBC from me.

Good catch bzrudi71.

+++ b/core/lib/Drupal/Core/Entity/Query/Sql/Condition.php
@@ -28,6 +29,7 @@ class Condition extends ConditionBase {
+

It is a nitpick, but can we remove this empty line.

It all looks good to me.
It passes all tests on MySQL, PostgreSQL and SQLite.
Just one nitpick that I hope the committer can remove.

+++ b/core/lib/Drupal/Core/Entity/Query/Sql/Condition.php
@@ -28,6 +29,7 @@ class Condition extends ConditionBase {
+

1. +++ b/core/lib/Drupal/Core/Database/Driver/pgsql/EntityQuery/Condition.php
   @@ -0,0 +1,38 @@
   +    // For PostgreSQL all the condition arguments need to have case
   +    // lowered to support not case sensitive fields.
   +    if (is_array($condition['value']) && $case_sensitive === FALSE) {
   +      $condition['where'] = 'LOWER(' . $condition['real_field'] . ') ' . $condition['operator'] . ' (';
   +      $condition['where_args'] = [];
   

   We need to be really, really careful about SQL injection here.

   Pretty sure that we need to escapeField() real_field here and likely have a test for this somehow as well.

2. +++ b/core/lib/Drupal/Core/Database/Driver/pgsql/EntityQuery/Condition.php
   @@ -0,0 +1,38 @@
   +      $n = 1;
   +      foreach ($condition['value'] as $key => $value) {
   +        $condition['where'] .= 'LOWER(:value' . $n . '),';
   +        $condition['where_args'][':value' . $n] = $value;
   +        $n++;
   +      }
   

   $key seems to be unused here. Either leave it out or use array_values($condition['value']) so that you can safely use the key.

   What happens if there are two conditions that are replaced liked this? don't have a clash here? There is a lot of code in the db_select() query build to avoid exactly that. Maybe this should be a static property that we increase?

For #65

@Berdir, regarding #2 use of array_values(), I followed the pattern from the fix in SA-CORE-2014-005. A test for multiple IN queries should solve the rest of #2 as well.

Re: #1. Agreed. There could be a contrib module that allows Entity Query to be done by a site builder/user where the user could add in fields/properties to the condition. I don't think this is an issue in core itself. Although I guess maybe a user with config import access could add a YML file with keys that generate SQL injection... Hmm... I wonder if that's possible now already.

I tried to write some tests for the cases in #65, but I was not able to get any fails with the following tests for both multiple conditions using the same field and SQL injection of the field name in the condition method.

I may not be creating the SQL injection correctly so any help is appreciated.

Edit: if I hack in a message in the test to display the exception message I get "1 ; -- not found" suggesting that something's being escaped or I'm doing something improperly. Maybe I'm doing something MySQL specific ?

@chx helped on IRC to point me at the Tables class, which I think is catching potential SQL injection earlier as the fields will be added with addField() before the query is compiled and run

I need to do some further reading, but escapeField($condition['real_field']) is still a good idea for future proofing any changes to the way Entity Query SQL works.

I think we do need to add escapeField() because we should not rely on Tables::ensureEntityTable() always throwing an exception, but at the same time I do not think we can write specific SimpleTest coverage because of Tables::ensureEntityTable() does what it is doing. The current coverage should suffice in my opinion.

I needed to rebase some code from one of my local branches the other day, and so here is a patch that adds the escape field call to escape the field identifier to compare against, and more accurately matches the pattern from Connection::expandArguments() with a comment explanation.

Someone other than me should do postgresql drupalci testbot run.

I really do not like to set this issue on needs work. :(

+++ b/core/lib/Drupal/Core/Database/Driver/pgsql/EntityQuery/Condition.php
@@ -0,0 +1,44 @@
+      $n = 1;
+      // Only use the array values in case an associative array is passed as an
+      // argument following similar pattern in
+      // \Drupal\Core\Database\Connection::expandArguments().
+      foreach (array_values($condition['value']) as $i => $value) {
+        $condition['where'] .= 'LOWER(:value' . $n . '),';
+        $condition['where_args'][':value' . $n] = $value;
+        $n++;
+      }

This is not the solution that berdir in comment #65.2 intended. We can do:

+++ b/core/lib/Drupal/Core/Database/Driver/pgsql/EntityQuery/Condition.php
@@ -0,0 +1,44 @@
+      $n = 1;
+      // Only use the array values in case an associative array is passed as an
+      // argument following similar pattern in
+      // \Drupal\Core\Database\Connection::expandArguments().
+      foreach ($condition['value'] as $value) {
+        $condition['where'] .= 'LOWER(:value' . $n . '),';
+        $condition['where_args'][':value' . $n] = $value;
+        $n++;
+      }

Or we can do:

+++ b/core/lib/Drupal/Core/Database/Driver/pgsql/EntityQuery/Condition.php
@@ -0,0 +1,44 @@
+      // Only use the array values in case an associative array is passed as an
+      // argument following similar pattern in
+      // \Drupal\Core\Database\Connection::expandArguments().
+      foreach (array_values($condition['value']) as $i => $value) {
+        $condition['where'] .= 'LOWER(:value' . $i . '),';
+        $condition['where_args'][':value' . $i] = $value;
+      }

Berdir said in comment #65 that we should choose a solution. So I did.

The empty classes for sqlite/postgres are unavoidable, but I wonder why we don't do similar to some of the others and provide a stub class - see MySQL's merge implementation for example.

/**
 * @file
 * Contains \Drupal\Core\Database\Driver\mysql\Merge.
 */

namespace Drupal\Core\Database\Driver\mysql;

use Drupal\Core\Database\Query\Merge as QueryMerge;

class Merge extends QueryMerge { }

+++ b/core/lib/Drupal/Core/Database/EntityQuery/ConditionInterface.php
@@ -0,0 +1,30 @@
+ * Contains \Drupal\Core\Database\EntityQuery\ConditionInterface.

This isn't implementing an EntityQuery condition at all. It's just providing a feature that happens only to be used by EntityCondition. Could use a more generic name?

So I think it's fine to start this as being specific to the EntityQuery API - since this is the only use case in core. As for the base class - I think it is okay to make other SQL engines think about how to implement this if necessary - they are not common and case sensitivity is one of the tricky areas.

I'm +1 to the patch as it stands now that @Berdir's concerns have been addressed.

@daffie, I didn't really feel that the part of the code was necessary after I wrote tests for it so I didn't include it in #70. I wanted to match as closely as possible to expandArguments().

Discussed with @catch - going to proceed here - we have a fix that is explicit to the Postgres driver and if we need to extend to the basic driver functionality we can do this later.

Committed 40ae780 and pushed to 8.0.x. Thanks!

Thanks for adding the beta evaluation to the issue summary.

I published the change record and put a follow-up issue for sqlsrv driver as no longer postponed, #2512388: Entity Query Condition class will need implementation soon.

I only just spotted this via the change notice that was posted, as I don't normally monitor the postgres driver queue and it wasn't tagged. This change goes way beyond the postgres driver, though, and as DB maintainer I must ask that it be rolled back.

1) A static method on Condition forces a static call to Database::. We've actively avoided that, and have adjusted the code multiple times to eliminate it when it's happened. Adding another one here is a regression in the driver architecture, and reduces our ability to remove the Database:: class in the future. (We've been trying to do so; it was blocked by Testbot/Simpletest, not by the DB system. I want to try again later once the testing infrastructure is improved.)

2) This introduces a dependency on the Entity system(!) into the DB driver. That's a full show-stopper. That is a completely backward dependency to have. Long-term, the DB system should be spun out to its own non-Drupal-specific library, or replaced with Doctrine DBAL (not Doctrine ORM, that's something else). This approach actively blocks that. It also creates a circular dependency between the Entity and DB system, which is architecturally unacceptable.

3) We already have a condition mutation mechanism in the DB drivers, and we already handled some case sensitivity in it. Why are we not using that? If that's not sufficient, let's make that sufficient instead.

This patch adds an unacceptable amount of technical debt to the DB system. Please revert it and let's find a cleaner way. I'm happy to discuss what that is, but this approach is NOT it and is not acceptable.

@Crell before I revert can we have a concrete of how to proceed? The problem is putting driver specific code in EntityQuery is not good either.

This introduces a dependency on the Entity system(!) into the DB driver.

There's no dependency added, the new interface just has the word 'Entity' in it.

See #80 where I pushed back on the naming (since it has nothing entity-specific about it, just happens to be single-use), but let's be clear what dependencies actually are and are not.

#3 would be good, but could use a plan, I'm not sure whether the plan would be much different than renaming the interface.

I agree with @catch. The dependency is the inverse. Entity\Query\Sql depends on DBTNG, and this does extend that dependency to the Database class so that SQL database drivers have a bit more control over what is going on.

It does increase technical debt, but I do not think it would increase the effort of swapping out DBTNG later on.

I talked about this with @Crell in IRC. Because this is part of #2157455: [Meta] Make Drupal 8 work with PostgreSQL or remove support from core before release which is in turn part of #2485119: [meta] The Drupal 8.0.0-rc1 Release Checklist and therefore implicitly critical (plus tomorrow is the July 1 deadline), we agreed that instead of reverting this we could discuss what to do with it in a followup. So marking back to fixed. Is someone else able to create a followup issue?

I am sorry to come in late but I only saw the solution to this in the Drupal 8 changes feed and -- this is not the right solution.

Instead, we have the backend_overrideable mechanism and we (dawehner and me) always wanted to set the default for that to the value of driver of the default database connection and then you can make a trivial override of any service based on that. I have pointed to this at #2157455-66: [Meta] Make Drupal 8 work with PostgreSQL or remove support from core before release and as noted in this issue earlier I am available on IRC to help.

It is not right that any random code that need per database changes would change the database driver itself.

Thanks @chx! A pointer to a better solution makes me feel more comfortable with potentially reverting this. Edit: though I honestly am also still okay with having something that works but is not the best solution in HEAD temporarily, especially given the criticality.

Is there any part of the current patch we'd keep?

My understanding was that the taxonomy term test was actually surfacing a deficiency in the postgres title, so the issue title didn't describe the actual bug that was being fixed. See #88 about the naming also.

I went to lunch right before IRC conversations commenced. :(

I still do not see the risk of the approach being greater than the risks of having to write a completely different, maybe unfeasible approach. Here's what I've tried in related case-sensitivity issues:

1. Override services in service.yml / backend_overridable.

I tried to do set pgsql driver specific services in service.yml, but the test bot (current or drupalci) did not use it at all. I did not feel like trying to fix the entire Testing framework in order to get this one thing working. It was just too much effort. However I think this would be a valid follow-up.

No core driver has tried to override services despite many use cases for it. I think that is also larger task/discussion regarding the database system.

This also increases technical debt probably more so than this patch.

2. Require citext.

This requires root access and is prohibitive for hosting providers. Not the right approach.

3. Add database driver logic inside Entity\Query\Sql.

This apparently is no good even though in my opinion the sql storage layer is every part of the database system, but whatever. It's not the right approach.

4. Approach later in this issue to ave Entity\Query\Sql call a class in the database driver.

I see this as the least amount of overhead of any of the options. The entity query system is already dependent on the database system, and this allows sql database drivers to change how the query works without rewriting entity query, which I think increases technical debt even more than this patch.

Can we open up a follow-up instead about overriding services and make sure we have a testable example?

Edit: oh, none of these IRC convos happened in #drupal-contribute :(

I will fix this proper if I can get postgresql working on my laptop. Last I tried (that was approximately when dinosaurs roamed the Earth) I failed (there was some funky auth business I could never figure out) but I hope it's easier now, the Arch Wiki usually is helpful.

Edit: well, this time it worked. I am on the case.

@chx: The current solution is chosen so that there are is no reference to a PostgreSQL specific solution in Drupal core outside the PostgreSQL database driver. At the moment there are a lot more of those references. Not only for PostgreSQL but also for MySQL and SQLite. My question is can you take a look at those references as well and say if your fix can be used to fix those references as well. And thanks for helping us with this problem. :)

> The current solution is chosen so that there are is no reference to a PostgreSQL specific solution in Drupal core outside the PostgreSQL database driver.

#2302617: Define a standard mechaism for backend-aware service overrides started down the exact opposite road even if mysql specific drivers didn't happen yet -- they should. The approach I will be taking is a) add 'default_backend': $database['driver'] to settings.yml in SiteSettingsForm::submitForm (which was postponed from that issue but since then the services.yml file has been added to install_check_requirements) b) add a pgsql.entity.query.sql service

Follow-up (not related to the latest discussion above): #2528160: Broken assertion introduced in #2443679

If you read the patch you ought to wonder: how does this even work, how do the relevant classes get picked up? Entity query has been preparing for this for years via the QueryBase::getClass method and the namespaces being passed around. However, this is the first time this extensibility is put to use so obviously it turns out some small fixes are necessary to get ConditionAggregate work as the rest:

1. QueryBase::conditionGroupFactory was passing in $namespaces but the ConditionFundamentals::__construct was not picking it up. 1 line fix in constructor + define a property.
2. QueryAggregate::conditionAggregateGroupFactory was not using the same pattern as QueryBase::conditionGroupFactory. Trivial fix: copy over the 2 lines necessary and change Condition to ConditionAggregate.
3. Now that we have namespaces available for all conditions we can remove the hardwired class call from ConditionAggregate::compile and replace it with QueryBase::getClass. Another 2 line fix.

With these fixes in place, the only thing to introduce the pgsql.entity.query.sql service is an empty query factory class. The rest is magically picked up as intended from the start. If we deem it necessary to introduce a sqlite.entity.query.sql service, now it'll be just the service definition + empty factory class.

The rest of the changes are just rolling back / moving the code. Also, I needed to expose the escapeField method to Select queries.

Edit: Per https://dispatcher.drupalci.org/job/default/2534/console TermTest passes on PHP 5.5 & PostgreSQL 9.1.

Its nice that the design actually works out!

With these fixes in place, the only thing to introduce the pgsql.entity.query.sql service is an empty query factory class. The rest is magically picked up as intended from the start. If we deem it necessary to introduce a sqlite.entity.query.sql service, now it'll be just the service definition + empty factory class.

Do you mind on that class how it works, so its clear how other backends might be able to achieve something similar.

Nice!

1. Lovely work and it's great that we're improving the API by using it in core.

2. +++ b/core/lib/Drupal/Core/Database/Query/SelectInterface.php
   @@ -141,6 +141,18 @@ public function &getUnion();
   +  public function escapeField($string);
   

   Missing @param for $string

3. +++ b/core/lib/Drupal/Core/Entity/Query/Sql/pgsql/Condition.php
   @@ -0,0 +1,35 @@
   +
   +class Condition extends BaseCondition {
   +
   +  public static function translateCondition(&$condition, SelectInterface $sql_query, $case_sensitive) {
   

   Missing documentation for class and method.

Oh yeah I ignored that on my initial review on purpose and then forgot about it :)

I've run the test locally on postgres and sqlite and can confirm we have passes. Yay!

> Missing @param for $string

Filed #2540674: Missing @param doxygen on Connection::escape* . This is simply a copy-paste and while I would've fixed in Connection::escapeField but there are three more methods there so this is out of scope.

Fixed the points that alexpott made. So back to RTBC.

This looks way better, and uses the tools available in the way they were meant to be used. Thanks, chx!

Only one nit, and IMO it's not commit-blocking so not changing status:

+++ b/core/lib/Drupal/Core/Entity/Query/Sql/pgsql/Condition.php
@@ -0,0 +1,41 @@
+      $n = 1;
+      // Only use the array values in case an associative array is passed as an
+      // argument following similar pattern in
+      // \Drupal\Core\Database\Connection::expandArguments().
+      foreach ($condition['value'] as $value) {
+        $condition['where'] .= 'LOWER(:value' . $n . '),';
+        $condition['where_args'][':value' . $n] = $value;
+        $n++;
+      }

Is there a reason to not use array_values($condition['value']), like the referenced routine does? Seems a bit cleaner that way. (Good catch that this is needed, though.)

I plead the fifth: that code is moved without change from HEAD. I have no idea what it does, why it does, what else could be done.

This issue addresses a major bug and is allowed per https://www.drupal.org/core/beta-changes. Committed 9fb5187 and pushed to 8.0.x. Thanks!

I changed https://www.drupal.org/node/2511626 to draft. Not sure what to do really. Publish another CR saying "nah, you don't need to"? Or just leave it ? It's been only a month and db drivers are rare.

I'd say just leave it unpublished. The total number of people outside of core that would care is about 3, and chx is one of them. :-)

I've deleted it - a CR with incorrect info has no worth. And if we leave it unpublished someone might come along and published it.