dev/core#1858 Prevent Duplicate contact records being created and har…

[seamuslee001]

…d fatal errors when creating a CMS user account from the contact action by ensuring that the contact_id field is correctly passed through for dedupe matching

Overview

Reproduction steps are on the lab ticket https://lab.civicrm.org/dev/core/-/issues/1858 but the key aspect of the reproduction steps is having an unsupervised dedupe rule for individuals that requires not just a match on email address to work

Before

DB error / duplicate contact created

After

No DB Error / no duplicate contact record created

ping @demeritcowboy @eileenmcnaughton @mlutfy @KarinG @kcristiano

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

[demeritcowboy]

• General standards
  • [r-explain] Ok
    • It's explored in the ticket that Unsupervised is involved because it calls a function that in its own words "can be called standalone". Otherwise it's confusing why Unsupervised matters.
    • Also the problem doesn't affect drupal 7 because drupal 8 always calls it and it's a different function getting called than drupal 7.
  • [r-user] PASS
  • [r-doc] PASS
  • [r-run] PASS
• Developer standards
  • [r-tech] Could use a code comment as to why
    • In light of the above and that it's a bit hard to follow what's going on and as discussed in the ticket, this seems a safer option, but it's going to be even more confusing to someone later. It could say something like // The "Create User Record" action for contacts uses a different field name.
  • [r-code] Ok
    • To be super-safe it should probably be if (empty($params['contact_id']) && !empty($params['contactID'])) {, but I don't know a path where that would come up where both would already be set AND they'd be different and then the "real" one would get overwritten with the "wrong" one.
  • [r-maint] Undecided
  • [r-test] PASS

[mattwire]

@seamuslee001 Can you add a code comment per @demeritcowboy suggestion?

[seamuslee001]

done

[demeritcowboy]

Great, thanks.

[eileenmcnaughton]

@seamuslee001 I was gonna merge this & then I read the comment :

"case from the back off contact task
// That this gets passed onto the duplicate function. "

I'm not sure that makes sense?

[seamuslee001]

@eileenmcnaughton have clarified the comment now hopefully

[eileenmcnaughton]

thanks!