Change DAO's that have 'default' => 'NULL' into 'default' => NULL, i.e. true NULL

[demeritcowboy]

Overview

It's been a few days since I tried breaking something...

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

[demeritcowboy]

Interesting.

Where this came from was #20476 where in api4 it will set a default value based on this field:

civicrm-core/Civi/Api4/Service/Spec/SpecFormatter.php

Line 80 in dd77834

$field->setDefaultValue($data['default'] ?? NULL);

. So when it's the literal string 'NULL' it sets it to that, not NULL.

I haven't tried running this anywhere yet to see what happens in practice.

[colemanw]

Wow, tests pass!
One possible gotcha would be if this value ever gets passed to $dao->save() because that treats the strings 'NULL' (or 'null' it's case-insensitive) not as a string but as the actual value NULL.

[eileenmcnaughton]

@demeritcowboy you must be losing your touch - green tick - what's with that!

[demeritcowboy]

He he... well it is marked WIP...

[agh1]

I left a comment on #20476 (comment) to the effect that this seems like it could be quietly and broadly disruptive change even without tests failing. I'm mainly concerned from the experience with CRM-15522 / civicrm/civicrm-packages#79. The string 'NULL' is used to proactively null out a value, while the value NULL is generally ignored by DB_DataObject (and the string 'Null' is used for our friend Mr. Null in CRM-15522).

I'm eager to dehackify Civi, so if we can get away with doing this change, I'm all for it, but it should absolutely be done independently and well in advance of the event timezone thing in #20476. @agileware-justin asked me to update this PR to say so.

Assuming this is the same stuff as is mixed in 5432709 with unrelated event timezone changes (I'm going to rely on @agileware-fj to verify that), I think the best way forward would be to tidy up this PR with a full explanation (and remove the WIP), do a lot of r-run on it, and see about merging it soon after a new release in order to maximize the exposure. Unfortunately I think merging this week would be waaay too rushed, so this would be merging sometime after 5.44 is branched in early November.

At that point, #20476 could be rebased and just include the event timezone stuff, and anyone looking to review and test it could have a much better handle on it all. It would also mean that it isn't mixed in with this NULL business, helping everyone diagnose bugs a lot easier.

[demeritcowboy]

I could be persuaded that it is a good thing

Ok: It's a good thing.

Seriously, I do agree it needs some evaluation not just relying on unit tests. However so far in my analysis and clicking on things I do think it's a different type of string 'NULL' than the ones that have been encountered before and baked into PEAR::DB.

Here's a list of where I think it's used besides the api4 usage in #20476. While I haven't looked deeply at all of them I don't think any of them change anything for NULL vs 'NULL', which at least partly explains unit tests.

• CRM/Core/BAO/SchemaHandler.php: if (!empty($params['default']) &&
• CRM/Core/BAO/SchemaHandler.php: $sql .= " DEFAULT {$params['default']}";
• CRM/Core/CodeGen/I18n.php: $default = $field['default'] ? ' DEFAULT ' . $field['default'] : '';
• CRM/Core/CodeGen/Specification.php: $field['default'] = $this->value('default', $fieldXML);
  • this is interesting, since combined with the dao.tpl it explains why in the xml there are some (non-null ones) that are quoted within the xml. It ensures the sql in sql/civicrm.mysql is valid, e.g. civicrm_action_schedule default "Email"
• CRM/Core/DAO.php: if (isset($fieldDef['default'])) {
• CRM/Core/DAO.php: $this->$dbName = $fieldDef['default'];
• CRM/Core/DAO.php: if (isset($fieldDef['default'])) {
• CRM/Core/DAO.php: $this->$dbName = $fieldDef['default'];
• CRM/Event/BAO/Participant.php: $participant->status_id = $participant->status_id ?: self::fields()['participant_status_id']['default'];
• CRM/Pledge/BAO/Pledge.php: 'installments' => (int) self::fields()['installments']['default'],

while the value NULL is generally ignored by DB_DataObject

Yes, this is something to look at. Offhand if the schema default is NULL, and then later passing in NULL gets ignored, then theoretically I'd expect it to end up NULL anyway. But yes something to look at.

[agh1]

Ok: It's a good thing.

Yeah, I think this overall is good. The only scenario I can actively think of is if a database field is set to default to something other than NULL even though the current schema expects it to be NULL. This would presumably be an old site where the schema changed.

Under the status quo, going to save a row, leaving that field unspecified, would presumably cause the field to pick up the value 'NULL', and the DB layer will take that to mean that the value should be set to NULL. My concern is that with this change, going to save a row, leaving that value unspecified, would cause the field to pick up the value NULL, and the DB layer would take that to mean the value should not be set. The result would be that MySQL's idea of the default is what would be used. I suspect that will be accurate most of the time (the same schema is what defined the table), but we may not be used to relying on that in certain places.

[agh1]

Looking at what you're doing here, and looking back through the history of this file, it seems pretty clear that this was simply an artifact of the XML having NULL in there and not needing to distinguish between that and a string because Pear::DB cleaned up after us. I'm now enthusiastic about this, but I'm still nervous.

[demeritcowboy]

Adding a note that I compared the before and after db schema dumps, with trigger-based logging enabled to see the log tables too, and they're identical. mysqldump -u user -p --routines --triggers --no-data dbname

Custom field table creation seems unaffected.

[agileware-justin]

@demeritcowboy can we get this added to the next CiviCRM RC please? This should help with getting more people involved with testing. Thanks!

[demeritcowboy]

I think what you're asking is for someone to merge this into 5.43 because people are currently testing out 5.43? I could rebase it if there's some consensus it should be backported, but it would still need someone to test/review it BEFORE it gets merged, so I'm not sure the version makes a difference in terms of helping the timezone PR (which would be in 5.44).

[agileware-justin]

@demeritcowboy let's just keep the ball rolling so things don't stall.

[demeritcowboy]

@agileware-justin I just saw your note in chat which I assume applies to the timezone PR but FYI every PR has a test site that gets autobuilt that can be used by anyone with a github account, and the link to it appears in the first comment by civibot near the top of the PR. It disappears after a week but can be rebuilt by typing "jenkins test this please". The timezone one has conflicts so it won't build, but I've just rebased this one which will rebuild this test site here.

[agileware-justin]

@demeritcowboy thanks mate, y'know I've never really looked at what that CiviBot comment was about. It always seems to say the same thing.

So when it says this:
"If you are reviewing this pull-request, you may wish to consult the test sites and the Review Standards (long template, short template)."

It means:
"Login to the TEST site here which has a copy of this PR applied. Use this to perform your review without having to manually apply the PR to your own instance"

Good to know 😄

[eileenmcnaughton]

We had a chat about this & there was a general feeling this should be safe

[agileware-fj]

Noice.

[agileware-justin]

Good chat 😄

[agileware-justin]

@demeritcowboy just want to give you a special shout out for your excellent work here, great job and much appreciated. Legend.

👍 👍 👍 👍 👍