-
-
Notifications
You must be signed in to change notification settings - Fork 825
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
dev/core#961 - Contribution page including 2 email fields does not re… #14252
Conversation
(Standard links)
|
CRM/Dedupe/Finder.php
Outdated
@@ -281,6 +281,8 @@ public static function formatParams($fields, $ctype) { | |||
if (preg_match('/(.*)-(Primary-[\d+])$|(.*)-(\d+|Primary)$/', $key, $matches)) { | |||
$return = array_values(array_filter($matches)); | |||
$flat[$return[1]] = $value; | |||
// make sure the first occurrence is kept, not the last | |||
$flat[$return[1]] = !isset($flat[$return[1]]) ? $value : $flat[$return[1]]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jitendrapurohit what happens if the first is empty & the second isn't?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @eileenmcnaughton. On testing the above scenario, I noticed it was still breaking if only the second field has the value in it. Fixed it and also extended the unit test to assert this additional behavior.
…spect dedupe rule.
test this please |
1 similar comment
test this please |
OK I stepped through this & it makes sense - there is STILL a logic flaw IMHO in that it will only check one of the passed in emails - without this patch it will check the last, regardless of whether it is empty or not. With this patch it will match the first non-empty one. I think the new approach is better but not 'quite right'. The unit test sells me on merging this as we will lock in fixing the tested scenario. The patch feels low risk to me based on digging into the code. Having multiple of the same type of location in a form seems relatively uncommon & I think it's unlikely people would have built flows around the assumption that one is more 'important' than the other if they have |
…spect dedupe rule.
Overview
Unsupervised rule fails if we have 2 email fields to match on with latter set as empty.
Before
To replicate -
After
Payment gets created on the existing contact. Dedupe match works correctly.
Comments
Gitlab - https://lab.civicrm.org/dev/core/issues/961