only set custom field to null if it is really null, not string 'null'

[jackrabbithanna]

Overview

The string 'null' (any case) is converted to actual NULLs. Is this a feature or a bug? It's a bug for one of my clients, as they have a multi-select custom field with one of the option values being 'NULL'

Before

Custom fields cannot be inserted or updated if the value is the string 'NULL'

After

Null strings as values for custom fields are saved.

Technical Details

This may break other stuff?
Comments

Conversation about overall issue here: https://lab.civicrm.org/dev/core/issues/475

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

[eileenmcnaughton]

@jackrabbithanna I think the 'null' to NULL convention is too baked into Civi to change without terrifying me - we should probably prevent 'null' being an option value name - they still have control over the label

[jackrabbithanna]

Can't do that for my client. Only 2 tests failed for the custom table query...
I made this PR to see what would break.

Mostly I want to know, why is it baked into Civi, what are the ramifications...Nobody seems to know exactly why 'null' is converted to NULL....Don't you just want to know?

Just wait, I got a patch for the packages repo, that'll really run the test suite through the gauntlet....:) hehehe....

If you look at:
civicrm/civicrm-packages@520511d

An exception for 'Null' was made awhile back...So I'm not 100% sure it WILL break anything...

[eileenmcnaughton]

@jackrabbithanna it was baked into Civi back in 1.1 or whatever when they decided that would be a good way of handling 'null'.....ie. when you want to do $dao->copyParams or whatever it distinguishes between not set & please ignore & please update this & set it to null

[jackrabbithanna]

I wonder if it's only the lower case string 'null' that is necessary....

[eileenmcnaughton]

I think it probably is - certainly the patch implies as much

• You could try changing the option value.value field in your customer db to 'Null' & in theory that should 'work'

[jackrabbithanna]

I have no problem maintaining an little patch for my client, or making an exception for the all uppercase 'NULL' string....I'm just curious about they "why"

[jackrabbithanna]

A patch for the CustomTableQuery is still necessary, I could exclude 'Null' and 'NULL' .. andrews patch for 'Null' in the packages repo is for all entity tables, like contacts...

[eileenmcnaughton]

@jackrabbithanna so when you do $dao->save the $dao object is fully populated with properties
so it has the property 'source' regardless of whether it has been changed or not - so the fact it equals NULL doesn't tell you whether or not to set the value to NULL or ignore that field in the given update

[eileenmcnaughton]

so back in the day they decided that specifying 'null' would cover that - my feeling is that it is very specifically 'null' not 'NULL' or 'Null' that is used for that purpose

[jackrabbithanna]

Yep, adding a condition for only 'null' to turn into NULL passed

[jackrabbithanna]

@eileenmcnaughton let me know if the last commit changes your mind about letting this in, I could write some test coverage...actually it would be really simple to update that entityCustomGroupWithSingleStringMultiSelectFieldCreate used by testCustomDataGet in the SyntaxConformanceTest.php to include an option 'NULL' => 'NULL' ... which should fail when trying to set the custom field value.

Otherwise we'll just keep a client specific patch going and not worry about it.

[eileenmcnaughton]

@jackrabbithanna so does the final commit include anything else - ie. on packages repo

I note the offending line was added by @deepak-srivastava when updating merge logic (also ping @JKingsnorth ) - however I see no evidence that the strtolower was added for some specific reason (as opposed to a coding practice that seemed better at the time).

My feeling is the dedupe code coverage is pretty good and I'm comfortable merging this if you do the test change you suggested so we don't get into whack-a-mole if there is some unexpected fallout

[jackrabbithanna]

The patch here in this PR is independent of any patch in the packages repo. This is specific for custom fields query generation, handled in civicrm-core

I think a patch to the packages repo would/could/should be independent, and that is where we'd really find possible regressions...that get's into the core DAO handling of all entities...

I'll have the updated test coverage here today sometime, and we'll make sure everything passes, then I can squash the commits and have it setup for merge into master if you are ok with it, and if we don't here back from anyone about possible issues.

What I'd be worried about is this may represent a departure for people having custom code or extensions, or possible imports. If people are depending on 'NULL' turning into NULL for custom code, or maybe even stock imports via csv, then this could change behavior for them.

I don't want to force this and then have a bunch of people caught by surprise, so if others want to say "no" to this, they can make the case for it.

[eileenmcnaughton]

@jackrabbithanna I feel like 'null' has been the only thing ever supported for emptying an existing field but it's perhaps worth running through an import to check what did happen with 'NULL' in a custom field and in core field prior to this

[petednz]

maybe i am testing wrong concept but on 5.3.x

| Reason | Contact ID | Gender | XYZ :: XYZ |
| Invalid value for field(s) : XYZ::XYZ; XYZ; Gender | 2 | null | null |

[eileenmcnaughton]

@petednz what about a string field - e.g contact_source & a text custom field?

[petednz]

where the contact already has data in those fields or doesn't that matter?

[petednz]

no validation issues with Core or Custom string field - in both cases Null was accepted in the import and replaced existing content in those fields

[eileenmcnaughton]

I tested import & this doesn't affect import as it doesn't get past validation on custom fields. It didn't seem to be possible to pass in 'null' on these fields.

I'm going to merge this - there is no convention anywhere else in our code to support 'NULL' as a substitute for the magic 'null' so I think it makes sense to remove this odd support - the change was added by @deepak-srivastava for 4.7 before we got as tight about adding unit tests & insisting on breaking big PRs down into separate fixes - it appears to me the strotolower was belt & braces rather than needed

https://github.com/civicrm/civicrm-core/pull/6432/files#diff-927e742818deb9c89a00d2528e360bf0R213

I tested a UI edit & the value comes in as 'null' not NULL

I'm going to merge this as it makes sense & adds test coverage for the change - you might like to do some checks @deepak-srivastava