correct logic for handling empty-array values for checkboxes;

[ginkgomzd]

Overview

API Contact.create has an edge-case for Checkbox custom-fields that does not handle empty arrays properly. There is a comment to this effect on the function, formatCheckBoxField();

We can't rely on downstream to add separators to checkboxes so we'll check here.

Before

PHP Fatal error: Uncaught TypeError: mysqli_real_escape_string(): Argument #2 ($string) must be of type string, array given in /var/www/html/wp-content/plugins/civicrm/civicrm/vendor/pear/db/DB/mysqli.php:873
Stack trace:
#0 /var/www/html/wp-content/plugins/civicrm/civicrm/vendor/pear/db/DB/mysqli.php(873): mysqli_real_escape_string()
#1 /var/www/html/wp-content/plugins/civicrm/civicrm/packages/DB/DataObject.php(1851): DB_mysqli->escapeSimple()
#2 /var/www/html/wp-content/plugins/civicrm/civicrm/CRM/Core/DAO.php(2198): DB_DataObject->escape()
#3 /var/www/html/wp-content/plugins/civicrm/civicrm/CRM/Core/DAO.php(1735): CRM_Core_DAO::escapeString()
#4 /var/www/html/wp-content/plugins/civicrm/civicrm/CRM/Core/DAO.php(1615): CRM_Core_DAO::composeQuery()
#5 /var/www/html/wp-content/plugins/civicrm/civicrm/CRM/Core/BAO/CustomValueTable.php(271): CRM_Core_DAO::executeQuery()
#6 /var/www/html/wp-content/plugins/civicrm/civicrm/CRM/Core/BAO/CustomValueTable.php(398): CRM_Core_BAO_CustomValueTable::create()
#7 /var/www/html/wp-content/plugins/civicrm/civicrm/CRM/Contact/BAO/Contact.php(378): CRM_Core_BAO_CustomValueTable::store()
#8 /var/www/html/wp-content/plugins/civicrm/civicrm/api/v3/Contact.php(574): CRM_Contact_BAO_Contact::create()
#9 /var/www/html/wp-content/plugins/civicrm/civicrm/api/v3/Contact.php(99): _civicrm_api3_contact_update()
#10 /var/www/html/wp-content/plugins/civicrm/civicrm/Civi/API/Provider/MagicFunctionProvider.php(89): civicrm_api3_contact_create()
#11 /var/www/html/wp-content/plugins/civicrm/civicrm/Civi/API/Kernel.php(149): Civi\API\Provider\MagicFunctionProvider->invoke()
#12 /var/www/html/wp-content/plugins/civicrm/civicrm/Civi/API/Kernel.php(81): Civi\API\Kernel->runRequest()
#13 /var/www/html/wp-content/plugins/civicrm/civicrm/api/api.php(22): Civi\API\Kernel->runSafe()
#14 phar:///usr/local/bin/cv/src/Command/ApiCommand.php(74): civicrm_api()
#15 phar:///usr/local/bin/cv/vendor/symfony/console/Command/Command.php(255): Civi\Cv\Command\ApiCommand->execute()
#16 phar:///usr/local/bin/cv/vendor/symfony/console/Application.php(1009): Symfony\Component\Console\Command\Command->run()
#17 phar:///usr/local/bin/cv/vendor/symfony/console/Application.php(273): Symfony\Component\Console\Application->doRunCommand()
#18 phar:///usr/local/bin/cv/src/Application.php(59): Symfony\Component\Console\Application->doRun()
#19 phar:///usr/local/bin/cv/vendor/symfony/console/Application.php(149): Civi\Cv\Application->doRun()
#20 phar:///usr/local/bin/cv/src/Application.php(26): Symfony\Component\Console\Application->run()
#21 phar:///usr/local/bin/cv/bin/cv(27): Civi\Cv\Application::main()
#22 /usr/local/bin/cv(14): require('...')
#23 {main}
thrown in /var/www/html/wp-content/plugins/civicrm/civicrm/vendor/pear/db/DB/mysqli.php on line 873

After

Changed !empty() to isset(), and added a check for !empty() before looping through the array.

To Reproduce

Create a custom field for a contact that is Alphanumeric and type Checkbox(es).

Attempt to save an empty value:

echo '{"id":"200447","custom_212":[]}' | cv api3 --in=json Contact.create

For those unfamiliar with using the API on the command-line, some assumptions:

• you must have cv installed
• the current directory of your command line must be in htdocs so cv can find the installation
• After creating the custom field, note the ID, and replace it in the example json.
• Since we are executing api Contact.create, the id in the input-json must be a valid Contact ID.

In the above example, the custom field ID was 212, and the contact ID was 200447.
If we have a fresh-install with demo data, a likely existing contact ID, might be 99, and a custom field ID might be something lower, such as 42.

Which then would mean the command would be:

echo '{"id":"99","custom_42":[]}' | cv api3 --in=json Contact.create

[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).

[ginkgomzd]

... oh, in case it is needed to explain why !empty() is not equivalent in this case to isset()

Not-empty resolves to FALSE when the array member is an empty array, but the test of the same with isset() is TRUE.

<?php
  
$theArray = ['field' => []];

if (isset($theArray['field'])) {
        echo 'ISSET';
}

if (empty($theArray['field'])) {
        echo 'EMPTY';
}

[jaapjansma]

@ginkgomzd can you explain to non technical users on how to reproduce this? So we can review this PR next time we are doing our round of PR reviews.

[ginkgomzd]

@jaapjansma okay, I updated the "To Reproduce" section.

[ginkgomzd]

Can this get reviewed again please?

[demeritcowboy]

I'm going to take a look at this today - I think it will resolve some unit test problems I'm having with drupal 10. I also have a partial unit test that could go along with this.

I'm not sure the first part of the change here is necessary - $field is a field definition not the value being passed in.

[demeritcowboy]

Noting the patch has conflicts even though github doesn't seem to think so. Let's see what jenkins thinks - jenkins test this please.

But yes I agree with the change in formatCheckBoxField() but not the first one about $field, although in practice probably doesn't make any difference - the field will either have a definition or not.

[demeritcowboy]

Jenkins agrees the patch doesn't apply anymore. @ginkgomzd can you update and then I think it's mergeable.

[demeritcowboy]

Something was bugging me so I took another look. I think it needs to have special handling for empty arrays, because otherwise what it stores in the db is '^A^A', which is interpreted as a checkbox with the option that has value '' checked, as opposed to what happens in the UI when you uncheck all the boxes and it stores just ''. On the contact screen it looks fine, but it's not the same thing. So something like, which unfortunately looks messy as a diff but is just set it to '' and return early if an empty array

index b4b5611269..57c1400afb 100644
--- a/api/v3/utils.php
+++ b/api/v3/utils.php
@@ -1173,15 +1173,21 @@ function formatCheckBoxField(&$checkboxFieldValue, $customFieldLabel, $entity) {
   $options = $options['values'];
   $validValue = TRUE;
   if (is_array($checkboxFieldValue)) {
-    foreach ($checkboxFieldValue as $key => $value) {
-      if (!array_key_exists($key, $options)) {
-        $validValue = FALSE;
-      }
-    }
-    if ($validValue) {
-      // we have been passed an array that is already in the 'odd' custom field format
+    if (empty($checkboxFieldValue)) {
+      $checkboxFieldValue = '';
       return;
     }
+    else {
+      foreach ($checkboxFieldValue as $key => $value) {
+        if (!array_key_exists($key, $options)) {
+          $validValue = FALSE;
+        }
+      }
+      if ($validValue) {
+        // we have been passed an array that is already in the 'odd' custom field format
+        return;
+      }
+    }
   }

   // so we either have an array that is not keyed by the value or we have a string that doesn't hold separators

[ginkgomzd]

Thanks @demeritcowboy !
I rebased the branch and I think it will merge now.

I don't think you need to worry about the delimiter handling for empty values, if I'm reading the code correctly, here:

civicrm-core/api/v3/utils.php

Line 1216 in 870e748

foreach ($possibleValues as $index => $possibleValue) {

Basically, I think it should hit, $formatValue = FALSE;.

[demeritcowboy]

But if you run it and look in the db and compare to what gets stored when using the UI you'll see this doesn't give the right result.

[ginkgomzd]

ah, I see it now.
Just pushed a simplified solution. Just identify the edge-case and return early.
Can you try your unit test?

[demeritcowboy]

Yes that looks good thanks.

[MegaphoneJon]

Seems like passing FALSE to a checkbox field causes a different exception, I'll try to work that up too.

[MegaphoneJon]

On second thought, FALSE shouldn't be a valid value to pass to a checkbox custom field, so I'm going to fix that downstream.