Extend token date handling to 'most' date fields

[eileenmcnaughton]

Overview

Extend token date handling to 'most' date fields

Before

Only {domain.now} supports date formatting

After

Date tokens from contribution, contribution recur, participant, activity also support them

Technical Details

If this passes it has lots ot test cover to 'prove' notchange (good since we aren't adding any formatting) - but if it doesn't it will prove I haven't lost my marbles (seems unlikely that I haven't but giving definitive answers to questions like that is what unit tests are for)

Comments

[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]

@totten this is a processing order thing - it's trying to do html formatting while still an object - perhaps we make sure it isn't an object before calling html_entities?

I think the {domain.now} was set on the html tokens so we didn't hit this

[totten]

I'd be inclined to log an error and proceed with the old value?

[eileenmcnaughton]

yeah ok

[eileenmcnaughton]

This is the error

1 /home/jenkins/bknix-dfl/build/core-21584-8x994/web/sites/all/modules/civicrm/Civi/Token/TokenRow.php(275): htmlentities(Object(DateTime))
#2 /home/jenkins/bknix-dfl/build/core-21584-8x994/web/sites/all/modules/civicrm/Civi/Token/TokenProcessor.php(364): Civi\Token\TokenRow->fill('text/html')

[eileenmcnaughton]

@totten this is passing now. I had to fix the html tokens to not go through html_entities as discussed

I also made a minor output change. In order to make all tests pass I had to decide what to do about midnight. I concluded that where dates have a meaningless time (midnight) it was most intuitive to always suppress it. I guess there could maybe be an edge case where something happens bang on '00:00:00' but I think the risk of confusion is less if omitted. I suppose we have a new challenge if we add timestamps but this seems OK for now to me

[totten]

So I guess there are a few cases, depending on the type of the underlying field:

• timestamp or datetime fields (eg civicrm_contribution.receive_date) -- the token's default format should indicate both date and time
• date or time fields (eg civicrm_contact.birth_date) -- the token's default format should indicate date xor time (respectively)

My intuition is that this step here is the lossy part - ie we know the kind of MySQL $field we're looking at, but we output a PHP DateTime object. But DateTime doesn't distinguish between temporal subtypes (time/date/datetime/timestamp). The lossiness will create a quirk where most datetime/timestamp data shows the time - but there is a 1/86,400 chance of displaying without the time. (Admittedly, that's not very high, but it is harder to reason about.)

I think the only thing we could really do here is pass-through a richer object. For example, with something like https://gist.github.com/totten/185f4d7bf87e22f7fc363c85c756ba32, we could add a hint of the true type of the data:

...->tokens($entity, $field, new MaskedDateTime($fieldValue, NULL, 'date'));
...->tokens($entity, $field, new MaskedDateTime($fieldValue, NULL, 'time'));
...->tokens($entity, $field, new MaskedDateTime($fieldValue, NULL, 'datetime'));
...->tokens($entity, $field, new MaskedDateTime($fieldValue, NULL, 'timestamp'));

Or we could attach a default format with something like: https://gist.github.com/totten/878177535912382fa298f80c5e456033

...->tokens($entity, $field, new FormattedDateTime($fieldValue, NULL, $config->datetimePartial));
...->tokens($entity, $field, new FormattedDateTime($fieldValue, NULL, $config->datetimeTime));
...->tokens($entity, $field, new FormattedDateTime($fieldValue, NULL, $config->datetimeFull));

[eileenmcnaughton]

@totten yeah the other thing is that how you use the data is a better indicator of whether you want the time to show than the metadata of the field. For example it's pretty commont to exclude time when recording contributions - but if you DO include it it's likely you would want to show it.

[eileenmcnaughton]

@totten so in fact it's probably not the case that 'most datetime/timestamp data shows the time' - that would be true for timestamp but for many date time fields having no time is the norm not the exception

[totten]

The patch has six changes to the test-suite here which seem to be counter-examples... Skimming civicrm-core@master:TokenConsistency.php, it looks to me like most of the examples do have times.

The exception is that civicrm_case.start_date and end_date are pure-dates. But this makes sense because the MySQL type is date (ie those fields don't store times).

[eileenmcnaughton]

@totten I meant the norm in terms of how people actually use the database - ie it's hugely common not to record time unless it's meaningful

[eileenmcnaughton]

Just noting from chat - pretty much every database I've ever seen has a gazillion date time records where the time is not filled in - eg. pretty much all imported contribution records have 'midnight' for their time. This is common across contributions, recurrings, pledges, etc etc

On the flip side a time of 'midnight' is really only meaningful in an event that really truly does start or end at midnight - which is rare but could be the case in a multizone event. Mitigating this

• we haven't added the timezone support yet & would provide a different default if timezone info was present
• currently events don't seem to rely on the default formatting for start or end date (they have logic to figure out if the date is the same & if exclude date)

[totten]

Clicking-around to spot-check, there a few distinct behaviors:

• Activity:
  • When creating a new record, both date+time are mandatory
• Contribution:
  • When creating a new record, both date+time are mandatory for "Date Received". But for "Receipt Date", the time-component is optional. (If blank, it's saved as 12:00 AM.)
  • When viewing or editing a record, it shows the time as 12:00 AM.
  • When you cancel a record, the "Cancelled Date" is generally optional. If completely blank, it saves with current date+time. If time is omitted, the time will save as 12:00 AM.
  • When viewing or editing a cancelled record, it shows the time as 12:00 AM.
• Event:
  • When creating a new record, the "Start Date" is generally required, and the "End Date" is generally optional. In both fields, the time component is optional. (If blank, it's saved as 12:00 AM.)
  • When viewing or editing the record (with public or private pages), the 12:00 AM does show up.
  • When using "New Event Registration", the "Registration Date" is prepopulated with both date+time. However, you can delete the "time" component. (If blank, it's saved as 12:00 AM.)
  • When viewing or editing a registration, it shows the time as 12:00 AM.
• Mailing:
  • When submitting a new mailing, you choose a radio-button - either (a) now or (b) datetime. If you give datetime, then both date+time are mandatory.
• Membership:
  • This does not have any datetime fields. (It only uses date fields.)
• Pledge:
  • When creating a new record, the "Pledge Made" and "Payments Start" look like date fields. Under the hood, they are saved with 12:00 AM. This is a consistent facade - the list/view/edit screens consistently hide the time component.

(The above is about the backend UI. Separately, a very large portion of temporal data is specified under-the-hood -- eg logging (*.created_date, *.modified_date, civicrm_subscription_history, civicrm_mailing_job.start_date, ad nauseum); also, records are often processed by self-service flows which autopopulate temporal data completely.)

It seems to me that:

• The "Pledge" entity is unusual. It has datetime fields with a consistent facade -- a simulacrum of date.
• For every other entity, the datetime fields are... well... date+time (together, as whole).
• When datetime is manually entered by a staff user, the time component is sometimes optional. But this is always flaky -- because blank time is coerced to 12:00 AM (which appears in search/browse/view/edit UIs; and which is indistinguishable from true 12:00 AM).
• This is a bit of a conundrum. I don't think we should encourage the idea of 12:00 AM as a magical value - because (in practice) it doesn't get that treatment consistently, and (in principal) it's ambiguous and it complicates timezone-correctness. OTOH, I think you're right (in a probabilistic sense) that 12:00 AM is almost never literal - it is more likely to indicate a backend-user skipped that particular datum. Whatsmore, datetime fields are prone to timezone problems... so maybe, on the whole, it's more accurate to show only the date...

[eileenmcnaughton]

@totten yeah - that's where I got to with it - the other big data source of data is imports & other input forms - which have a higher chance of having no time attached. The bit about 'not much happens at midnight' means that where contributions don't have a time (1% of them in the DB I just checked) they probably didn't happen at midnight (0% in the db I just checked)

What we are setting is a default that should be somewhat intuitive from a user point of view -- but imagine if you actually did have an event that started at midnight - probably at that point we are having the conversation about
a) helping people choose a format with the timezone included and
b) getting this working #21599 so it can output in the selected timezone

• but in those cases we aren't using the defaults.

[colemanw]

My 2 cents is that for the purposes of filling tokens, it's safe to assume that a time of 00:00:00 can be treated like "date only", as we are talking about a display value here.

[totten]

@eileenmcnaughton I'll go ahead and merge based on the 'merge ready' with 2/3 comfortable with the 00:00:00 heuristic. (And, even if I'm not as comfortable with the heuristic -- I don't think it'll cause harm, as long as message-tokens are only used for their intended purpose... composing prose messages...)