Respect php NULL; do not transform string 'null' about org.civicrm.api4 HOT 8 CLOSED

Since this undesired behavior goes all the way down to the DAO, we need to work around the core behavior when passing Api params to the BAO for create/update operations.
One idea is to patch core with a setting/property in the DAO like $dao->_nullMode = TRUE which disables the special handling of 'null'.

from org.civicrm.api4.

I used a test creating a contact with first_name "Tom" and last_name "Null".

In this test setting, contact last_name to "Null" worked fine. However changing it to "null" resulted in an empty last_name. This is because in CRM_Contact_BAO_Individual::format() explicitly checks for "null" and sets it to an empty string because:

// "null" value for example is passed by dedupe merge in order to empty.
// Display name computation shouldn't consider such values.

Even after removing this block, this last_name is still set to null. However display name is now set to "Tom null".

Inside the DataObject::insert() method there is a check for null strings. It is conditional on the disable_null_strings setting not being set. It only checks if the string is 'null' and allows 'Null'.

It is possible to change the behavior and by adding:

global $_DB_DATAOBJECT;
$_DB_DATAOBJECT['CONFIG']['disable_null_strings'] = FALSE;

anywhere before the call to CRM_Core_DAO::save() the last_name was stored as string null.

@colemanw what do you suggest doing here? If we want to do this only from api4 then we could set the global 'disable_null_stringsto FALSE before doing an API request and revert it afterwards. It seems a bit hacky though and the issue inCRM_Contact_BAO_Individual::format()` will still cause problems for contacts. Seems like this issue would be better suited to a core patch, but because of the comments on usage of string null in the dedupe merge we might run into problems in other areas of the code.

from org.civicrm.api4.

I'm in favor of your suggestion to set and unset the global. I agree it's hacky but the alternative is to go down a very deep rabbit hole in core.

As for the "feature" in CRM_Contact_BAO_Individual::format() I think a refactor in core might be possible but haven't looked at the code. What about adding a param to that function to flag the behavior, and only pass that param as true from the dedupe merger code?

Alternatively, a cheap but effective hack would be to ucfirst() the string 'null' in a contact name from within api4, and comment the hell out of it.

from org.civicrm.api4.

I've added a toggle function in the Create class which globally allows insertion of null strings. It's still failing because of the CRM_Contact_BAO_Individual::format() which actually doesn't affect the last_name, just the display_name, so if I use "Joe null" it will result in "null" for last_name, but just "Joe" for display_name.

It would be nice to understand the reasons for using string null in the dedupe classes. If we could clean this up I think it would be a much nicer option than adding yet another confusing if ($allowNullStrings) inside the CRM_Contact_BAO_Individual::format() class.

from org.civicrm.api4.

Regarding updating a value using NULL. This goes down right to the Pear DB_DataObject handling of updates.

If we update an existing value to NULL it will fail because DateObject::update() checks !isset($this->$field) before continuing e.g. $this->last_name, which will be false if it's NULL. If we changed this to update anyway it would also update every other value including those that were not included in the update request. There doesn't seem to be a way to update only certain values without using string nulls.

It would be easy to change NULL to string 'null' in api4, but then we have this situation just from this ticket:

• If creating then string null will be saved
• If updating then string null will result in unsetting the value. True NULL will be converted to string null and also unset the value.

There's no consistency there at all and it would lead to a very frustrating experience (admittedly for a small number of users). I would much prefer to discuss on how to handle this better in core, even if that means using something instead of the PEAR DB_DataObject in the future.

from org.civicrm.api4.

I've implemented a sort of compromise:

• If you set true NULL as a value it will be converted to string 'null' so the value will be unset
• If you set string 'null' as a value it will be converted to 'Null' since that's the only variation of string null that is allowed.

I've updated the test to reflect this. To summarize:

// create a contact with string null as last name
$contact = Contact::create()
    ->setValue('first_name', 'Joseph')
    ->setValue('last_name', 'null')
    ->setValue('contact_type', 'Individual')
    ->execute();

// string null is converted to "Null"
$this->assertSame('Null', $contact['last_name']);
$this->assertSame('Joseph Null', $contact['display_name']);
  
// set the last name to true NULL
$contact = Contact::create()
    ->setValue('id', $contactId)
    ->setValue('last_name', NULL)
    ->execute();

// last name is unset, display name is updated
$this->assertSame(NULL, $contact['last_name']);
$this->assertSame('Joseph', $contact['display_name']);

from org.civicrm.api4.

@mickadoo ok is there a PR for that?

from org.civicrm.api4.

@colemanw sure, it's here. I didn't want to create it while another one was open. I wanted to use the structure I created in the other PR to handle the null values. If you want you can review the other one first and I can update this one with those changes. If you do this one first it's sort of like reviewing both together.

from org.civicrm.api4.