(REF) CRM_Dedupe_BAO_DedupeRule - Support third party fields and tables in rules #23332
Conversation
|
(Standard links)
|
|
@darrick I always worry that things like this will break in the future if there is no unit test - as you are probably aware the hooks in this code are more 'jammed in' than thought out - which makes them especially vulnerable to future changes breaking the code that relies on them |
|
@eileenmcnaughton do you mean a unit test for what I'm trying to use this for? It did pass all the current tests. The documentation is pretty bad for sure. My issue is was CRM_Dedupe_BAO_DedupeRule:sql throwing an error because the table I added during the supprtedFields call isn't handled. So, just having that return NULL instead would help. But then I ran into the problem with the noRules value being incorrect. So, the wrong temptable being created. Making CRM_Dedupe_BAO_DedupeRule:sql a bit more magical on finding the contact_id key to JOIN on and if the table also requires the entity_type would also solve it. |
|
@darrick yep - a unit test on whatever would upset you if it broke! ie how you are trying to use the hook |
|
@darrick can you squash the eight commits into one? I follow the steps to do so: $ cd <civicrm-core directory>
$ git pull --rebase upstream master // resolve conflicts if any
$ git rebase -i // it will open the editor, edit the commit and put 's' in beginning of each line, and leave the first commit
$ git push -f origin dupeQuery assuming that |
Well I tried that but when I ran git rebase -i it looked like I was merging commits which weren't mine. |
darrick
force-pushed
the
dupeQuery
branch
2 times, most recently
from
311d69b to
2d54619
Compare
|
Rather then relocating the call for the 'table' op, adding a 'query' op in the CRM_Dedupe_BAO_DedupeRule:sql function seemed less disruptive. The test shows proof of concept of what I'm getting at. |
darrick
force-pushed
the
dupeQuery
branch
2 times, most recently
from
e361708 to
761f850
Compare
…rtedFields hook.
Adding unit tests for hook_civicrm_dupeQuery.
Fix coding standards.
Add query option to dupeQuery hook.
Fix typo.
Provide custom test entity to test findDuplicates.
Change contact type from Organization to Individual.
Use CRM_Core_DAO::getReferencesToContactTable() and
CRM_Core_DAO::getDynamicReferencesToTable('civicrm_contact')
to find refs for the DedupeRule query.
Simplify changes as much as possible.
darrick
changed the title
|
Thank you @darrick for rebasing the PR, that excluded the false merge commits. I agree with the patch and it fulfilled all the criteria to get this PR merged.
|
Overview
Currently if a table and field is added via hook_dupeQuery type = supportedFields. CRM_Dedupe_BAO_DedupeRule:sql throws the error "Unsupported rule_table for civicrm_dedupe_rule.id".
The CRM_Dedupe_BAO_DedupeRule:sql has a limited number of hard-coded tables. My initial attempt was to use hook_dupeQuery with the "tables" op for an extension to add that query. But the sql() function does a lot of lifting and I ended up copying most of it into the hook.
I then tried splitting the sql() in two so a user can can call the function with their own $id and $where clause. Adding a new op 'query' because it seemed like 'tables' was more for optimizing all the queries for the rule group.
I then discovered CRM_Core_DAO::getReferencesToContactTable(); and CRM_Core_DAO::getDynamicReferencesToTable('civicrm_contact'); functions which are used in the CRM_Dedupe_Merger class. And so found those could automagically determine the right contact and entity reference field_names to use to create the query for the supported field.
Before
Adding a table and field to the supporteFields list, creating a dedupeRule using that field and then running the dedupeRule results in a fatal error ("Unsupported rule_table for civicrm_dedupe_rule.id");
After
If a programmer chooses to have their field available for a dedupeRule they must implement hook_dedupeQuery with the supportedField op to add their table and fields. They also must ensure CRM_Core_DAO::getReferenceColumns() returns the link from their table to the civicrm_contact table and also the link for the entity_table if required.
The hook_dedupeQuery with op "tables" can still be used to modify the auto-generated query.
Technical Details
The hook_civicrm_dupeQuery is called in four places:
CRM_Dedupe_BAO_DedupeRuleGroup:supportedFields - allowing tables and fields to be added to the selectable field list available when editing rules.
CRM_Dedupe_BAO_DedupeRuleGroup:fillTable - allowing for queries of the dedupeRuleGroup to be modified.
CRM_Dedupe_BAO_DedupeRuleGroup:thresholdQuery - allowing for the threshold of the dedupeRuleGroup to be modified.
CRM_Contact_Form_DedupeRules:postProcess - allowing for table indexes to be defined.
Comments