From: Tim Otten <totten@civicrm.org>
Date: Wed, 15 May 2019 08:23:37 +0000 (+0100)
Subject: CRM_Utils_SQL_* - Properly interpolate NULL values
X-Git-Url: https://vcs.fsf.org/?a=commitdiff_plain;h=3808d8b19ca407fd0c1a444de9f38421d9a75296;p=civicrm-core.git

CRM_Utils_SQL_* - Properly interpolate NULL values

Overview
--------

The `CRM_Utils_SQL_*` classes are used to generate SQL expressions with
properly escaped data. However, in many cases, the values of `NULL` are
processed in a way which clearly doesn't work, e.g.

Before
------

When passing in a parameter with a `NULL` value, the parameter *is not evaluated at all*.

```
>>> CRM_Utils_SQL_Select::from('civicrm_contact')->where('do_not_email IS @foo', ['foo' => NULL])->toSQL()
=> """
   SELECT *\n
   FROM civicrm_contact\n
   WHERE (do_not_email IS @foo)\n
   """
```

Specifically:

* `CRM_Utils_SQL_Select` - PHP NULL parameters are ignored (as if the value does not exist). No test coverage.
* `CRM_Utils_SQL_Delete` - PHP NULL parameters are ignored (as if the value does not exist). No test coverage.
* `CRM_Utils_SQL_Insert` - PHP NULL inputs are represented as SQL NULLs. No test coverage.

After
-----

When passing in a parameter with a PHP `NULL` value, the parameter is encoded as a SQL `NULL` value.

```
>>> CRM_Utils_SQL_Select::from('civicrm_contact')->where('do_not_email IS @foo', ['foo' => NULL])->toSQL()
=> """
   SELECT *\n
   FROM civicrm_contact\n
   WHERE (do_not_email IS NULL)\n
   """
```

Specifically:

* `CRM_Utils_SQL_Select` - PHP NULL inputs are represented as SQL NULLs. Has test coverage.
* `CRM_Utils_SQL_Delete` - PHP NULL inputs are represented as SQL NULLs. Has test coverage.
* `CRM_Utils_SQL_Insert` - PHP NULL inputs are represented as SQL NULLs. Has test coverage.

Comment
-------

I'm generally hesitant about changing behavior like this.  However, given
that the old behavior was clearly non-functional, I'm struggling to figure a
way where this would be problematic. Moreover, I'm a bit encouraged by the
fact that:

* There are unit-tests specifically for these classes,
* When grepping core for references to `CRM_Utils_SQL_ `, there are several
  matches involving APIv3 and ActionSchedules -- both of which have a long
  practice of unit-testing.
* When grepping my local ext's, there are several matches for things in
  `apiv4` and `civicase` - which also some test-coverage.
---

diff --git a/CRM/Utils/SQL/BaseParamQuery.php b/CRM/Utils/SQL/BaseParamQuery.php
index f48d05556c..159f56a5fa 100644
--- a/CRM/Utils/SQL/BaseParamQuery.php
+++ b/CRM/Utils/SQL/BaseParamQuery.php
@@ -95,10 +95,10 @@ class CRM_Utils_SQL_BaseParamQuery implements ArrayAccess {
 
       $select = $this;
       return preg_replace_callback('/([#!@])([a-zA-Z0-9_]+)/', function($m) use ($select, $args) {
-        if (isset($args[$m[2]])) {
+        if (array_key_exists($m[2], $args)) {
           $values = $args[$m[2]];
         }
-        elseif (isset($args[$m[1] . $m[2]])) {
+        elseif (array_key_exists($m[1] . $m[2], $args)) {
           // Backward compat. Keys in $args look like "#myNumber" or "@myString".
           $values = $args[$m[1] . $m[2]];
         }
diff --git a/tests/phpunit/CRM/Utils/SQL/DeleteTest.php b/tests/phpunit/CRM/Utils/SQL/DeleteTest.php
index 25d39722b4..f0903be19e 100644
--- a/tests/phpunit/CRM/Utils/SQL/DeleteTest.php
+++ b/tests/phpunit/CRM/Utils/SQL/DeleteTest.php
@@ -25,6 +25,15 @@ class CRM_Utils_SQL_DeleteTest extends CiviUnitTestCase {
     $this->assertLike('DELETE FROM foo WHERE (foo = "not\\"valid") AND (whiz > "in\\"valid") AND (frob != "in\\"valid")', $del->toSQL());
   }
 
+  public function testWhereNullArg() {
+    $del = CRM_Utils_SQL_Delete::from('foo')
+      ->where('foo IS @value', array('@value' => NULL))
+      ->where('nonexistent IS @nonexistent', [])
+      ->where('morenonexistent IS @nonexistent', NULL)
+      ->where('bar IS @value', array('@value' => 'null'));
+    $this->assertLike('DELETE FROM foo WHERE (foo IS NULL) AND (nonexistent IS @nonexistent) AND (morenonexistent IS @nonexistent) AND (bar IS "null")', $del->toSQL());
+  }
+
   /**
    * @param $expected
    * @param $actual
diff --git a/tests/phpunit/CRM/Utils/SQL/InsertTest.php b/tests/phpunit/CRM/Utils/SQL/InsertTest.php
index ba997cca80..36a48a060e 100644
--- a/tests/phpunit/CRM/Utils/SQL/InsertTest.php
+++ b/tests/phpunit/CRM/Utils/SQL/InsertTest.php
@@ -25,13 +25,15 @@ class CRM_Utils_SQL_InsertTest extends CiviUnitTestCase {
         array('second' => '2b', 'first' => '1b'),
         array('first' => '1c', 'second' => '2c'),
       ))
-      ->row(array('second' => '2d', 'first' => '1d'));
+      ->row(array('second' => '2d', 'first' => '1d'))
+      ->row(array('first' => NULL, 'second' => '2e'));
     $expected = '
       INSERT INTO foo (`first`,`second`) VALUES
       ("1","2"),
       ("1b","2b"),
       ("1c","2c"),
-      ("1d","2d")
+      ("1d","2d"),
+      (NULL,"2e")
     ';
     $this->assertLike($expected, $insert->toSQL());
   }
diff --git a/tests/phpunit/CRM/Utils/SQL/SelectTest.php b/tests/phpunit/CRM/Utils/SQL/SelectTest.php
index 5cecf42c6e..f9ff68d3aa 100644
--- a/tests/phpunit/CRM/Utils/SQL/SelectTest.php
+++ b/tests/phpunit/CRM/Utils/SQL/SelectTest.php
@@ -72,6 +72,15 @@ class CRM_Utils_SQL_SelectTest extends CiviUnitTestCase {
     $this->assertLike('SELECT * FROM foo WHERE (foo = "not\\"valid") AND (whiz > "in\\"valid") AND (frob != "in\\"valid")', $select->toSQL());
   }
 
+  public function testWhereNullArg() {
+    $select = CRM_Utils_SQL_Select::from('foo')
+      ->where('foo IS @value', array('@value' => NULL))
+      ->where('nonexistent IS @nonexistent', [])
+      ->where('morenonexistent IS @nonexistent', NULL)
+      ->where('bar IS @value', array('@value' => 'null'));
+    $this->assertLike('SELECT * FROM foo WHERE (foo IS NULL) AND (nonexistent IS @nonexistent) AND (morenonexistent IS @nonexistent) AND (bar IS "null")', $select->toSQL());
+  }
+
   public function testGroupByPlain() {
     $select = CRM_Utils_SQL_Select::from('foo')
       ->groupBy("bar_id")