Remove static caching of array in mapConditionOperator()

I have no clue about Postgres, but if this comes back green, I feel comfortable RTBCing this. :)

This looks fine to remove that extra logic, so I've committed pushed it, however why are we keeping a static in that method just to save building the array each time? Moving back to active to discuss.

typo

We should get rid of the static keyword - it is not needed - the array will be set no matter what

Thanks for the patch, @pjohans. I queued up a postgresql test since this affects that driver only.

+++ b/core/lib/Drupal/Core/Database/Driver/pgsql/Connection.php
@@ -300,7 +300,7 @@ public function createDatabase($database) {
-    static $specials = array(
+    $specials = array(
       // In PostgreSQL, 'LIKE' is case-sensitive. For case-insensitive LIKE
...
       'LIKE' => array('operator' => 'ILIKE'),

Perhaps this should be a protected static variable on the Connection class instead of a scoped variable that's instantiated every time. I doubt there would be any meaningful performance leaving it without static though.

Making the specials array a class property would be in my eyes the best solution from a OOP perspective. Can we give the class variable the same name as the method that it belongs to.

The same problem exists with the Connection class from the SQLite driver and with the class Drupal\Core\Database\Query\Condition.

Also adding some docblocks to mapConditionOperator methods would not be a mistake. They are all inheritdocs.

How about this for PostgreSQL, SQLite and Condition operator?

I made the static protected variables unique to each class because the implementation might be different. I don't think we have an interface that's for all 3.

Flipped to 8.3.x because I applied to 8.3.x. The patch should still apply to 8.2.x.

Re-rolled for 8.2.x.

I am reviewing this issue for the patch from comment #13, because that is the patch for Drupal 8.3.
This issue is not a bug, so this patch cannot be applied to 8.2.

THe patch for 8.3 looks good to me. I have only one nitpick and that can be fixed on commit:

+++ b/core/lib/Drupal/Core/Database/Driver/sqlite/Connection.php
@@ -24,6 +24,19 @@ class Connection extends DatabaseConnection {
+   * We don't want to override any of the defaults.
...
+    // We don't want to override any of the defaults.

The same comment twice. Please remove the second one.

@mradcliffe: Would you be interested in reviewing the PostgreSQL issues: #2057693: PostgreSQL orderBy method adds fields it doesn't need to, leading to fatal errors when the result is used as an insert subquery and #2238253: Add bindValue to a PDO::PARAM_* type in database query arguments?

Yep. I need a bit of detox and de-jetlag. I haven't made it home yet.

This change seems fine to me as a cleanup.

1. +++ b/core/lib/Drupal/Core/Database/Driver/sqlite/Connection.php
   @@ -24,6 +24,19 @@ class Connection extends DatabaseConnection {
   +  static protected $sqliteConditionOperatorMap = [
   

   Nit, switch to protected static

   ~/www/d8/core$ ag "static public" | wc -l
         10
   ~/www/d8/core$ ag "public static" | wc -l
       3080
   ~/www/d8/core$ ag "static protected" | wc -l
         10
   ~/www/d8/core$ ag "protected static" | wc -l
        192
   

2. +++ b/core/lib/Drupal/Core/Database/Driver/sqlite/Connection.php
   @@ -370,14 +383,7 @@ public function createDatabase($database) {
   +    return isset(self::$sqliteConditionOperatorMap[$operator]) ? self::$sqliteConditionOperatorMap[$operator] : NULL;
   
   +++ b/core/lib/Drupal/Core/Database/Query/Condition.php
   @@ -297,34 +322,14 @@ function __clone() {
   +    if (isset(self::$conditionOperatorMap[$operator])) {
   +      $return = self::$conditionOperatorMap[$operator];
   ...
   +      $return = isset(self::$conditionOperatorMap[$operator]) ? self::$conditionOperatorMap[$operator] : array();
   

   Not that we'd often be extending this class, but wouldn't static:: be more useful than self::?

CNW for #21, both good changes.

Addressing #18 and #21

Manually confirmed that #18 and #21 comments were applied to the patch.

Then I guess it's RTBC again.

Committed dc5b0a4 and pushed to 8.3.x. Thanks!