Respect '0' as a default when generating DAOs

[colemanw]

Overview

When generating the DAO files, set the default even if it is "0"

Before

DAO fields were missing default of '0'

After

DAO fields contain this default

Notes

The Api v4 uses this default value to determine if a field is required for api input. In addition to this PR I've fixed the logic there: civicrm/org.civicrm.api4@377f8f6

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

[JoeMurray]

I like the direction of this PR. I just want to ask about the confidence that this is actually NFC. I take it you are surfacing into the DAO what will happen via MySQL table definition if DAO provide no value for these fields.

[colemanw]

@JoeMurray assuming the tests pass, I think this is harmless, as it just helps the PHP DAO::fields() more accurately reflect what's going on in the sql database. I don't think it will affect existing code because php rarely distinguishes '0' from null. In other words, incorrect code like if ($field['default']) will continue to behave incorrectly even after this change :)

[colemanw]

Test failure looks unrelated. But just to be sure let's have @civicrm-builder retest this please.
@mlutfy do you have any idea why PR tests are now taking upwards of 5 hours to complete?

[colemanw]

Test fail still appears to be unrelated. @civicrm-builder retest this please.

[totten]

My initial reaction was the same as @JoeMurray -- liking the direction but feeling some trepidation about how to demonstrate the safety (r-tech/r-user). So I've been grepping for ideas/use-cases. The PR still seems to add up -- I couldn't find anything else that depends on this value in a surprising way. However, that kind of thing is hard to find, so I just want to leave notes in case they trigger a thought for anyone lurking here.

(1) Joe asked if this was "... surfacing into the DAO what will happen via MySQL table definition...". This appears to be accurate -- it fits with the logic of the main change (in xml/templates/dao.tpl). Additionally, I spot-checked 4-5 entities and confirmed that the new PHP lines (default => '0') correlate to existing SQL lines (DEFAULT 0).

(2) There's a few references in CRM/Core/DAO.php, but only in the context of test code (assignTestValue()). A problem there would surface quickly in the test results.

(3) There are a bunch of 'default' references in CRM/Report/, but these appear to be based on $_columns that are defined in the reports, and this smells like a different data-structure entirely. Many reports define the columns fully/explicitly. Also, after grepping the report code for reflective logic (::fields or getfields) that might copy over metadata, I couldn't find anything obvious. So all signs point to this being separate/unrelated to the XML/DAO/SQL default.

(4) There's surprisingly little reference to 'default' in APIv3 framework code -- surprising because the API calls respect defaults. But it does make sense if you recall that APIv3 defaults ('api.default') were originally layered-in as something distinct from XML/DAO/SQL default. Skimming the references to 'api.default', I couldn't find any derived from the XML/DAO/SQL default. (Each api.default appears to be a simple static value -- or a computed value from somewhere else -- but nothing that obviously points back to the XML/DAO/SQL model.)

(5) CRM/Logging/Schema.php gets defaults from SQL -- doesn't use DAOs. So it's already respecting the SQL defaults per (1) above.

(6) As far as I can see, CRM/Core/BAO/SchemaHandler.php is only used for custom-groups.

(7) CRM/**Import** has some references to ::fields and to default. I'm not super-confident about this one -- but about as confident as one can be with <10 min skimming. Basically, the calls to ::fields just seem to be directed at getting field name -- most likely they didn't touch defaults because they expected SQL to enforce the defaults. They do call some helper functions, but (in general grepping) no helper functions appeared to be using default.

I'm going to add the merge ready label -- which means we should pause a bit (in case others want to respond to the notes); but absent further discussion, it's OK to merge.

[colemanw]

@civicrm-builder retest this please

[colemanw]

This turned out to have an impact on CRM_Core_DAO::createTestObject() - which was causing CRM_Core_BAO_ActionScheduleTest::testRepetitionFrequencyUnit to fail because it now defaults to creating emails with is_primary = 0 instead of 1 (0 is the database default). But I actually think it's an improvement, and since that function is only used for unit tests, I think we're good.

Example CRM_Core_DAO::createTestObject() output:

Before	After
CRM_Contact_BAO_Contact Object	CRM_Contact_BAO_Contact Object
[id] => 4	[id] => 4
[contact_type] => Individual	[contact_type] => Individual
[contact_sub_type] => null	[contact_sub_type] => null
[do_not_email] => 1	[do_not_email] => 0
[do_not_phone] => 1	[do_not_phone] => 0
[do_not_mail] => 1	[do_not_mail] => 0
[do_not_sms] => 1	[do_not_sms] => 0
[do_not_trade] => 1	[do_not_trade] => 0
[is_opt_out] => 1	[is_opt_out] => 0
...	...

And everything else is the same. In other words, default values are better represented in the test object.

[colemanw]

We're down to 1 failing test and it is a preexisting failure not related to this PR: api_v3_ReportTemplateTest.testActivitySummary.

[eileenmcnaughton]

I've mulled. Now I'm merging