Skip to content
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

enforce required fields on Contact.duplicatecheck #22741

Merged

Conversation

MegaphoneJon
Copy link
Contributor

https://lab.civicrm.org/dev/core/-/issues/3065

Overview

Duplicate checking now requires arguments to be passed which previously were optional (at the BAO level).

Before

cv api Contact.duplicatecheck throws a fatal error when you don't pass required values.

After

Exceptions are thrown if you're missing match criteria or a contact type. Rule type is once again optional.

Comments

This is a 5.46 regression; see #22394.

I checked all the other calls to CRM_Contact_BAO_Contact::getDuplicateContacts(). This is the only one that can pass values that don't pass the strict type checking.

@civibot
Copy link

civibot bot commented Feb 9, 2022

(Standard links)

@civibot civibot bot added the master label Feb 9, 2022
@eileenmcnaughton
Copy link
Contributor

Thanks @MegaphoneJon - can you flip to be against the rc?

@MegaphoneJon MegaphoneJon changed the base branch from master to 5.47 February 9, 2022 21:24
@civibot civibot bot added 5.47 and removed master labels Feb 9, 2022
@MegaphoneJon MegaphoneJon force-pushed the fix-duplicate-contact-typing branch from add6a37 to 7a94342 Compare February 9, 2022 21:28
@MegaphoneJon
Copy link
Contributor Author

@eileenmcnaughton I'll get it right one day.

$dupes = CRM_Contact_BAO_Contact::getDuplicateContacts(
$params['match'],
$params['match']['contact_type'],
$params['rule_type'],
$params['rule_type'] ?? '',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would $params['rule_type'] ?: 'Unsupervised' be more correct?

Also - there IS a default for the field

  $params['rule_type'] = [
    'title' => 'Dedupe Rule Type',
    'description' => 'If no rule id specified, pass "Unsupervised" or "Supervised"',
    'type' => CRM_Utils_Type::T_STRING,
    'api.default' => 'Unsupervised',
  ];

So why isn't that kicking in?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it only seems to kick in when the parameter isn't in the parameters passed to the API call and it seems that at least webform civicrm may be deliberately passing in NULL for this API call https://github.com/civicrm/civicrm-core/blame/master/api/v3/Contact.php#L1608

@seamuslee001 seamuslee001 added the merge ready PR will be merged after a few days if there are no objections label Feb 9, 2022
@seamuslee001
Copy link
Contributor

This seems the right fix to me just flagging as merge ready but I can't think of a reason not to merge this

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
5.47 merge ready PR will be merged after a few days if there are no objections
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants