API4: Allow save() to match on null values

[highfalutin]

Before

Code was trying to allow save() to match on null values, but was shooting itself in the foot.

After

Matching on fields works even when their value is null.

[civibot]

(Standard links)

• If you are reviewing this pull-request, you may wish to consult the test sites and the Review Standards (long template, short template).

[colemanw]

Code was trying to allow save() to match on null values

I'm not entirely sure that it was. But I'm not against updating it to do so, with a unit test, and I think we ought to add an additional safety-guard that a positive match must include at least one non-null value.

[highfalutin]

@colemanw, the already-merged PR #22882 (@mattwire) intended to allow matching on null -- see the comment. I'm keen on having this work properly.

I added a test. I also added in the "safety guard" (this commit) but after thinking about it, I took it back out. I don't know why we'd assume people won't want to match on a set of empty values. The function already aborts if the number of matches > 1, and that seems like enough of a safety guard.

[colemanw]

Wow, that was fast!
Ok, to clarify (the test is a little hard to grok), is NULL being treated like a wildcard that matches any value? Or does NULL only match NULL?

[highfalutin]

NULL or empty-string only matches NULL or empty-string (using the IS EMPTY operator). And I agree, the test was impressively clever, but almost impossible to read, so I've rewritten it from scratch.

[highfalutin]

I could break the test up into a few smaller tests to make it even more grokkable, @colemanw,
but does this work well enough?

[colemanw]

It was thoughtful of you to add this cleanup at the end, but that's not how we do test cleanup :)
For one thing, it's a pain for test authors to remember to do this every time, and, more importantly, if the test fails then it will never get to this step!

Best practice these days is to use $this->createTestRecord() instead of the api. As the name implies, the function creates test records... but it also automatically cleans them up afterwards, even if the test crashes halfway through!

[colemanw]

@highfalutin test is fine now can you just remove the bit at the end and use $this->createTestRecord() instead of the api.

[colemanw]

@highfalutin are you able to update this? It would be great to get it merged.

[highfalutin]

@colemanw a busy few weeks here but I'll get to it. IIRC, we can't take out the bit at the end and still have a passing test, but I'll take another look

[highfalutin]

@colemanw I got rid of the "delete" action, and switched to relying on TransactionalInterface to handle cleanup. Back to using a data provider. But hopefully the test is still not too hard to grok.

[highfalutin]

I believe the test failure is unrelated.

[civicrm-builder]

Can one of the admins verify this patch?

[highfalutin]

civibot, test this please

[highfalutin]

@colemanw can we get this into the new rc?

[colemanw]

@civicrm-builder retest this please

[colemanw]

Yes, thanks for your work on this @highfalutin