File custom fields cause a fatal error when trying to merge

added Concept approved prioritised sig:bug triaged type:request labels

I ran into this issue as well, and it seems like there are several issues. I've a proposed patch, but would like some discussion before I file a PR: https://gist.github.com/lcdservices/f1ca7018479a6b99261e03e8c4bf432b

The first issue is that moveAllBelongings() calls moveContactBelongings() before it processes custom file fields. moveContactBelongings() deletes the custom data row for the duplicate record, which means the file field processing can't get the data to update in the retained record. I considered just reordering the code, but I think the flaw is in moveContactBelongings, with how it handles custom data. You can see in the first snippet of the linked patch that it skips single-record custom data tables UNLESS they are actually relevant to the merge, in which case it handles them similar to how it handles other special tables processed by the merge. The problem is that this results in the following statements being run (an example from my test site):

[76] => UPDATE IGNORE civicrm_value_logistics_9 SET entity_id = 96628 WHERE entity_id = 96629
[77] => DELETE FROM civicrm_value_logistics_9 WHERE entity_id = 96629

This transfers the data from the duplicate record to the retained record without any regard to the selected fields (if the retained record doesn't have a row yet) and then deletes the duplicate record. Again -- the problem as described above is that now the duplicate record is deleted, we can't transfer data to the retained record. But I think the logic here is flawed to begin with. Why would we transfer and delete on a table-row level, when we need to be able to pick and choose fields per the merge interface? My patch skips single-row custom tables altogether and lets them be handled in moveAllBelongings().

The second chunk in the patch addresses a problem where deleteFileReferences() throws a fatal error if no file ID is passed -- which will happen if the duplicate record has a file reference but the retained record does not (it is calling the deletion on the retained record). This just adds a condition so we don't call that unless we actually have a file ID to call it on.

The third chunk addresses cleanup. We need to remove the file reference from the duplicate contact now that it's been moved to the retained record.

Anyway -- it's really the first piece that is the chunk would benefit from some other reviewer input, as it changes how custom data rows are handled.

@lcdweb I have an interest in this code as I would like to be able to add a Contact.getconflicts api call but the conflict stuff is pretty buried at the moment so I thought I'd look at this as a way in. I managed to replicate it in a test - which is usually half the challenge but in this case the code is so awful (although mostly well tested) I feel I can't proceed without addressing it a bit first - I created

https://github.com/civicrm/civicrm-core/pull/14140 as a first very minor step in the right direction & will try a couple more minor steps on top

Here is the test

+  /**
+   * Test merging 2 contacts with custom fields.
+   */
+  public function testMergeCustomFields() {
+    $ids = $this->entityCustomGroupWithSingleFieldCreate(__FUNCTION__, __File__);
+    $fileField = $this->customFieldCreate([
+     'custom_group_id' => $ids['custom_group_id'],
+     'data_type' => 'File',
+     'html_type' => 'File',
+     'default_value' => '',
+   ]);
+    $contact1 = $this->individualCreate();
+
+     // Not sure this is quite right but it does get it into the file table
+    $file = $this->callAPISuccess('Attachment', 'create', [
+      'name' => 'header.txt',
+      'mime_type' => 'text/plain',
+      'description' => 'My test description',
+      'content' => 'My test content',
+      'entity_table' => 'civicrm_contact',
+      'entity_id' => $contact1,
+    ]);
+    $this->callAPISuccess('Contact', 'create', ['id' => $contact1, 'custom_' . $fileField['id'] => $file['id']]);
+
+    $contact2 = $this->individualCreate();
+
+    $this->callAPISuccess('contact', 'merge', [
+      'to_keep_id' => $contact2,
+      'to_remove_id' => $contact1,
+      'auto_flip' => FALSE,
+    ]);
+
+    $contact = $this->callAPISuccessGetSingle('Contact', ['id' => $contact2, 'return' => ['custom_' . $fileField['id'], 'custom_' . $ids['custom_field_id']]]);
+    $this->assertEquals($file['id'], $contact['custom_' . $fileField['id']]);
+  }
+

@lcdweb OK- after a handful of small tidy ups I feel I have made enough sense of this code to pass an official verdict - and it is..... WTF!!!

From what I can see when we delete the file in CRM_Core_BAO_File::deleteFileReferences IF it is against the contact we are going to keep regardless of whether we are carrying across a record from the other contact or not. There were previous attempts to fix this - but before I had enforced my regime of raising the bar on test requirements. Still the most recent one in this whackamole I did ....

There seems to be code that says 'delete the file - but only if you want to keep it.....'

@lcdweb I nearly agreed with your approach but then backed away - the reason being that I'm not confident that the chunk of code that just pushes the custom field value into the profile.create function works for ALL custom fields - or whether some rely on the 'whack-em-in' approach. I suspect a bit more cleanup on the function will help but with the state of the code & the whack-a-mole history here (of which only the most recent I was able to force tests on) we have a fair bit of risk

Related issues /links https://github.com/civicrm/civicrm-core/pull/9784 https://issues.civicrm.org/jira/browse/CRM-19702 https://issues.civicrm.org/jira/browse/CRM-12261 https://issues.civicrm.org/jira/browse/CRM-21773 https://github.com/civicrm/civicrm-core/pull/11691 https://issues.civicrm.org/jira/browse/CRM-20858

Tidy ups....

https://github.com/civicrm/civicrm-core/pull/14144 https://github.com/civicrm/civicrm-core/pull/14142 https://github.com/civicrm/civicrm-core/pull/14140

mentioned in commit 07664dc9

mentioned in commit 2c409890

mentioned in commit 87497b2f

mentioned in commit ee22e172

closed

changed milestone to %5.15.0

mentioned in commit 1600a9c0

mentioned in commit c3f693f2

File custom fields cause a fatal error when trying to merge

Child items ...

Activity