Migrate case activity revisions to trigger-based logging
The old description below is currently still true but this is now about migrating to trigger-based logging.
click to see old boring description
This is an old topic, but I'm making this ticket as a place to put some notes related to https://github.com/civicrm/civicrm-core/pull/16283
Background: Before detailed/trigger-based logging was introduced, civicase always had a mechanism to keep revisions of every activity. Over time a couple things happened that led to it no longer working in the UI, although it still worked if you used the api to update case activities. The revisions mechanism isn't very efficient, but it's still there.
When I started looking into it about a year ago, there didn't seem to be much interest, so I only took a quick look and what was odd was that it seemed like it SHOULD still be working. Then when the PR was posted I started fiddling and was surprised/confused by the fact that a unit test calling the form code, where I was trying to show that it should be failing, was working, and I could see the test was creating revisions! But it's clearly not working when done through the UI.
It turns out that whether or not you have any custom fields defined, in CRM_Case_Form_Activity::postProcess() the param 'hidden_custom' always comes in as '1' when run in real life, and so a line that had been put in a few years ago (and later further quickie-patched) gets run that causes the 'id' param to get set to the current activity id regardless of whether you're making a revision or not, so it never makes the new revision and just updates that activity. This line: $params = array_merge($params, $oldActivity);.
There's also, now with the above PR, some redundant stuff that causes at least one problem and I'm still checking to see if there's more. If you have regular attachments (as opposed to custom fields of type file) it ends up calling CRM_Core_BAO_File::copyEntityFile twice because of the new api call which also deals with it, which leads to duplicate db key errors.
Additionally, there are some lines that, at least as far as I can see, do nothing except prepopulate a static cache that doesn't seem to need prepopulating, and the local variable is never used: https://github.com/civicrm/civicrm-core/blob/5.21.0/CRM/Case/Form/Activity.php#L450-L456. They don't seem to cause problems, but it makes me wonder what I'm missing.
The unit test is still WIP but is at the link below, along with some other fiddlings but ignore those. Given that a difficulty with revisions is custom fields and data like attachments I'd like to expand the test to include those, otherwise it might lead to a patch-break-patch-break-patch-break situation again. https://github.com/civicrm/civicrm-core/compare/master...demeritcowboy:16283-testing#diff-d2708831d5c3b219fce5d221bc3b17c6