CRM_Utils_SQL_* - Properly interpolate NULL values

[totten]

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.

[civibot]

(Standard links)

• If this is your first pull-request for CiviCRM, please browse CONTRIBUTING.md for information about the development and testing processes.
• If you are reviewing this pull-request, you may wish to consult the test sites and the Review Standards (long template, short template).

[seamuslee001]

Jenkins test this please

[colemanw]

One potential gotcha is that both Api3 and Api4 get actions are built on top of CRM_Utils_SQL_Select, and Api3 has always ignored anything in $params with a value of NULL. Skimming the Api3SelectQuery class I think it is actually skipping NULL values before passing them through to CRM_Utils_SQL_Select - if that's the case then all is well.

[totten]

I think it is actually skipping NULL values before passing them through to CRM_Utils_SQL_Select - if that's the case then all is well.

Yup, that's what I'd expect -- if it tried to pass down a NULL param and do a substitution on it, then the query would look nonsensical (like WHERE (do_not_email IS @foo) above).

[JoeMurray]

It would be appropriate to support WHERE (mytextfield IS NULL) as well as WHERE (mytextfield IS NOT NULL) in the API. Once this PR is merged, could that limitation be removed?

[colemanw]

@JoeMurray the api can do that already by doing something like

$params['first_name'] = array('IS NOT NULL' => TRUE);

[eileenmcnaughton]

@colemanw is this mergeable - I saw you'd looked at it so haven't dug in myself