CRM_Utils_SQL_* - Properly interpolate NULL values
authorTim Otten <totten@civicrm.org>
Wed, 15 May 2019 08:23:37 +0000 (09:23 +0100)
committerTim Otten <totten@civicrm.org>
Wed, 15 May 2019 08:50:22 +0000 (09:50 +0100)
commit3808d8b19ca407fd0c1a444de9f38421d9a75296
tree9d800aa114c8ef0db6de4d2854acb0fa01e09291
parentf3171f180523dcd35a6335156734ec61c0e47120
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.
CRM/Utils/SQL/BaseParamQuery.php
tests/phpunit/CRM/Utils/SQL/DeleteTest.php
tests/phpunit/CRM/Utils/SQL/InsertTest.php
tests/phpunit/CRM/Utils/SQL/SelectTest.php