dev/core#723 Fix fatal error when trying to merge contacts with a cus… #14325
Conversation
|
(Standard links)
|
eileenmcnaughton
force-pushed
the
merge_cust_field
branch
3 times, most recently
from
093072c
to
fc74ff8
Compare
eileenmcnaughton
force-pushed
the
merge_cust_field
branch
2 times, most recently
from
80bdd6a
to
495a5fc
Compare
eileenmcnaughton
force-pushed
the
merge_cust_field
branch
from
495a5fc
to
e947f02
Compare
|
@lcdservices are you able to test this? |
|
Reviewing this now. |
There was a problem hiding this comment.
(CiviCRM Review Template WORD-1.2)
- General standards
- (
r-explain) Issue: minor typos/grammar/clarification in code comments; see my review comments. - (
r-user) Pass: - (
r-doc) Pass - (
r-run) Pass:- Verified the issue exists in master, reproduced with steps in dev/core#723: observed fatal error as described.
- After applying PR, repeated same steps, and observed:
- no fatal error
- file was correctly moved to surviving contact in merge.
- (
- Developer standards
- (
r-tech) Issue:- The PR "Technical Details" mentions that "the profile.create hook would be bypassed"; does this refer to hook_civicrm_post() and hook_civicrm_pre() ? I'm not aware of the likely impact of this change, and it's hard to cr; could be worth a PSA to extension developers.
- (
r-code) Pass - (
r-maint) Pass: PR includes changes to an existing test, which is passing. - (
r-test) Pass
- (
CRM/Core/BAO/CustomField.php
Outdated
| $oldContact = civicrm_api3('Contact', 'getsingle', ['id' => $oldContactID, 'return' => $return]); | ||
| $newContact = civicrm_api3('Contact', 'getsingle', ['id' => $newContactID, 'return' => $return]); | ||
|
|
||
| // The moveaAllBelongings function has functionality to move custom fields. It doesn't work very well... |
There was a problem hiding this comment.
typo "moveaAllBelongings" should be "moveAllBelongings" ?
CRM/Dedupe/Merger.php
Outdated
| @@ -2146,29 +2148,36 @@ protected static function swapOutFieldsAffectedByQFZeroBug(&$migrationInfo) { | |||
| /** | |||
| * Honestly - what DOES this do - hopefully some refactoring will reveal it's purpose. | |||
| * | |||
| * Update this is formatting fields to be processed through 'ProfileContactCreate action | |||
There was a problem hiding this comment.
I'm unsure of the meaning of this line/sentence. Could it be edited for clarity?
eileenmcnaughton
force-pushed
the
merge_cust_field
branch
from
e947f02
to
517a184
Compare
…tom file field Technical comments here https://lab.civicrm.org/dev/core/issues/723#note_16231
eileenmcnaughton
force-pushed
the
merge_cust_field
branch
from
517a184
to
1600a9c
Compare
|
@twomice thanks - I've update the comments you pointed to. Regarding switching to not using profile create - this PR doesn't actually make that change but yes perhaps if once soon does we should send a PSA |
|
@eileenmcnaughton Thanks for the work on this -- I am very happy (and surely it's not just me) to see this getting resolved. Thanks for explaining about the hook; I just assumed I was missing something there. So I'm approving this as officially as I can here. Anything else I can do to help get this merged? |
|
Thanks @twomice - I'll merge it - I'm gonna take this a bit further so hopefully you will review the follow ups too |
|
Yes, hopefully. |
Overview
Fixes a fatal error when trying to merge 2 contacts with custom fields of type file
Before
Fatal error
After
Merge succeeds
Technical Details
As discussed by @lcdservices in gitlab we have a problem where custom fields are dealt with in multiple places - & on at least 2 types (file & country) they are not successfullly merged. My big picture proposal is to move all handling of custom field moves to a new 'move' function. This would mean that we no longer create & format a $submitted array for them, removing a lot of code, and probably we would be able to stop calling the profile.create function - removing complexity in favour of a simple api call for the contact fields.
One downside is the profile.create hook would be bypassed. I think that's OK - I have never heard anyone say they use it. But we could consider making the move function a pattern with it's own hook . - I also think it should be api-exposed since 'arg I want to move a contribution from this contact to that' is a 'thing' & the BAO doesn't provide generic helpers as yet
Comments