dev/core#5433 Add legacydedupefinder #31689

eileenmcnaughton · 2025-01-02T04:54:03Z

Overview

This adds a new extension for existing dedupe finder behaviour, allowing us to support the hook that is jammed in the middle of the dedupe finder code going forwards without being blocked by it (I have an existing PR that cuts many hours off the dedupe query but it is bending over backwards to work alongside the legacy attempts to allow integration).

Before

We know how to speed up dedupe queries #30591 but are working really hard around the legacy hook and the existing PR is having to rely on crazy methodology. The hook dupeQuery() is not one that has had much adoption, nor has the idea of writing custom improvements for the reserved queries. However, these 2 things have been long-standing blockers to improving performance

After

The new extension is enabled on install and on upgrade and there is no behaviour change unless a site admin disables it. If they choose to do so then

the legacy hook & legacy reserved query wrangling will no longer be called
dedupe queries will speed up once the blocked code is also merged Fix dedupe query wrangling to combine queries where 2 fields in the same table are always used together #30591

Technical Details

The code works by adding a new hook

findExistingDuplicates(array &$duplicates, array $ruleGroupIDs, ?string $tableName, bool $checkPermissions)

The hook is implemented by the new extension legacydedupefinder with a weight of -5 and by core with a weight of -20

legacydedupefinder calls stopPropagation() meaning that if it is enabled the core code will not be hit. Any custom extensions that give no specific thought to it will wind up with a weight of 0 - meaning they get called first, which feels like it would be as expected. Once finalised I will document this.

The function in the core code and in the extension is currently the same, except the core code is passing a parameter to fillTable to block legacy hooks from being called. Once this is merged the goal is to separate out & re-write the core function.

Before that I want to look at the second code path and attempt to do the same for it - the challenge is I really want to pass out apiv4 params to the function, not v3 https://lab.civicrm.org/dev/core/-/issues/5433

Comments

The extension is currently not hidden but if I don't get to where I want to before forking I will hide it so people do not disable it until the new hooks are settled in

@mattwire @colemanw

civibot · 2025-01-02T04:54:06Z

🤖 Thank you for contributing to CiviCRM! ❤️ We will need to test and review this PR. 👷

Introduction for new contributors...

If this is your first PR, an admin will greenlight automated testing with the command ok to test or add to whitelist.
A series of tests will automatically run. You can see the results at the bottom of this page (if there are any problems, it will include a link to see what went wrong).
A demo site will be built where anyone can try out a version of CiviCRM that includes your changes.
If this process needs to be repeated, an admin will issue the command test this please to rerun tests and build a new demo site.
Before this PR can be merged, it needs to be reviewed. Please keep in mind that reviewers are volunteers, and their response time can vary from a few hours to a few weeks depending on their availability and their knowledge of this particular part of CiviCRM.
A great way to speed up this process is to "trade reviews" with someone - find an open PR that you feel able to review, and leave a comment like "I'm reviewing this now, could you please review mine?" (include a link to yours). You don't have to wait for a response to get started (and you don't have to stop at one!) the more you review, the faster this process goes for everyone 😄
To ensure that you are credited properly in the final release notes, please add yourself to contributor-key.yml
For more information about contributing, see CONTRIBUTING.md.

Quick links for reviewers...

➡️ PR Demo site (can be rebuilt with the command test this please).
📖 Review standards
🗒️ Review template (brief or verbose)

➡️ Online demo of this PR 🔗

civibot · 2025-01-02T04:54:09Z

The issue associated with the Pull Request can be viewed at https://lab.civicrm.org/dev/core/-/issues/5433

eileenmcnaughton · 2025-01-02T04:55:33Z

ext/legacydedupefinder/legacydedupefinder.php

+ */
+function legacydedupefinder_civicrm_config(&$config): void {
+  _legacydedupefinder_civix_civicrm_config($config);
+  \Civi::dispatcher()->addListener('hook_civicrm_findExistingDuplicates', ['\Civi\LegacyFinder\Finder', 'findExistingDuplicates'], -5);


I didn''t think this was still the right way - but my reading of https://docs.civicrm.org/dev/en/latest/hooks/usage/symfony/ is that unless it's a BAO class or in a very specific location then it still is

Agree the docs don't make it very clear. But don't think we should be doing it like this anymore.
I think the "correct" way would be to implement an interface, in this case `https://docs.civicrm.org/dev/en/latest/hooks/usage/symfony/#hookinterface would be fine in Civi/LegacyFinder/Finder.php and make sure you have scan-classes mixin enabled because it's in an extension

@mattwire I just tried & failed to switch over - see #31959

I tried (very reluctantly) renaming the class to a BAO class - as that seemed slightly less inaccurate than an apiv4 event subscriber class per

But no joy.

Note HookInterface doesn't seem right as it has no ability to set weight

CRM/Dedupe/BAO/DedupeRuleGroup.php

bin/regen.sh

eileenmcnaughton · 2025-01-16T23:05:12Z

test this please

mattwire · 2025-02-05T10:50:10Z

ext/legacydedupefinder/info.xml

+  </classloader>
+  <civix>
+    <namespace>CRM/Legacydedupefinder</namespace>
+    <format>24.09.1</format>


@eileenmcnaughton As this is going into core can we generate with latest civix please?

@mattwire done - I've also made it 'hidden' for now - until I've migrated the second hook

colemanw · 2025-02-05T13:48:27Z

CRM/Upgrade/Incremental/php/FiveEightyTwo.php

@@ -29,6 +29,7 @@ class CRM_Upgrade_Incremental_php_FiveEightyTwo extends CRM_Upgrade_Incremental_
   */
  public function upgrade_5_82_alpha1($rev): void {
    $this->addTask(ts('Upgrade DB to %1: SQL', [1 => $rev]), 'runSql', $rev);
+    $this->addSimpleExtensionTask(ts('enable dedupe backward compatibility'), ['legacydedupefinder']);


@eileenmcnaughton I think this needs to be moved to SixZero.

===== Overview ==== This adds a new extension for existing dedupe finder behaviour, allowing us to support the hook that is jammed in the middle of the dedupe finder code going forwards without being blocked by it (I have an existing PR that cuts many hours off the dedupe query but it is bending over backwards to work alongside the legacy attempts to allow integration). The new extension is enabled on install and on upgrade and there is no behaviour change unless a site admin disables it. If they choose to do so then 1) the legacy hook & legacy reserved query wrangling will no longer be called 2) dedupe queries will speed up once the blocked code is also merged civicrm#30591

civibot bot added the master label Jan 2, 2025

eileenmcnaughton commented Jan 2, 2025

View reviewed changes

seamuslee001 reviewed Jan 2, 2025

View reviewed changes

CRM/Dedupe/BAO/DedupeRuleGroup.php Show resolved Hide resolved

seamuslee001 reviewed Jan 2, 2025

View reviewed changes

CRM/Dedupe/BAO/DedupeRuleGroup.php Show resolved Hide resolved

seamuslee001 reviewed Jan 2, 2025

View reviewed changes

CRM/Dedupe/BAO/DedupeRuleGroup.php Outdated Show resolved Hide resolved

eileenmcnaughton force-pushed the finder_ext branch from c6314b1 to b987e2a Compare January 12, 2025 23:05

Sjord reviewed Jan 14, 2025

View reviewed changes

bin/regen.sh Outdated Show resolved Hide resolved

eileenmcnaughton force-pushed the finder_ext branch 3 times, most recently from f240f20 to c3a8dc5 Compare February 4, 2025 02:23

mattwire reviewed Feb 5, 2025

View reviewed changes

eileenmcnaughton force-pushed the finder_ext branch from c3a8dc5 to 4a89208 Compare February 5, 2025 11:21

eileenmcnaughton mentioned this pull request Feb 5, 2025

Attempt (& fail) to use Subscriber interface instead of hook_config #31959

Closed

colemanw reviewed Feb 5, 2025

View reviewed changes

eileenmcnaughton force-pushed the finder_ext branch from 4a89208 to 69dc61e Compare February 5, 2025 21:23

colemanw added the merge on pass label Feb 5, 2025

eileenmcnaughton force-pushed the finder_ext branch from 69dc61e to d95f952 Compare February 5, 2025 22:42

eileenmcnaughton mentioned this pull request Feb 5, 2025

Finder ext again #31967

Closed

eileenmcnaughton force-pushed the finder_ext branch 6 times, most recently from b775ccd to b4e1979 Compare February 6, 2025 04:49

eileenmcnaughton added 3 commits February 7, 2025 09:18

Use autoservice & crossed fingers

9f84565

Migrate the second hook

74b477a

eileenmcnaughton force-pushed the finder_ext branch from b4e1979 to 74b477a Compare February 6, 2025 20:18

eileenmcnaughton merged commit 5e0868c into civicrm:master Feb 6, 2025
1 check passed

eileenmcnaughton deleted the finder_ext branch February 6, 2025 21:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

dev/core#5433 Add legacydedupefinder #31689

dev/core#5433 Add legacydedupefinder #31689

eileenmcnaughton commented Jan 2, 2025 •

edited

Loading

civibot bot commented Jan 2, 2025

civibot bot commented Jan 2, 2025

eileenmcnaughton Jan 2, 2025

mattwire Feb 5, 2025

eileenmcnaughton Feb 5, 2025

eileenmcnaughton commented Jan 16, 2025

mattwire Feb 5, 2025

eileenmcnaughton Feb 5, 2025

colemanw Feb 5, 2025

dev/core#5433 Add legacydedupefinder #31689

dev/core#5433 Add legacydedupefinder #31689

Conversation

eileenmcnaughton commented Jan 2, 2025 • edited Loading

Overview

Before

After

Technical Details

Comments

civibot bot commented Jan 2, 2025

civibot bot commented Jan 2, 2025

eileenmcnaughton Jan 2, 2025

Choose a reason for hiding this comment

mattwire Feb 5, 2025

Choose a reason for hiding this comment

eileenmcnaughton Feb 5, 2025

Choose a reason for hiding this comment

eileenmcnaughton commented Jan 16, 2025

mattwire Feb 5, 2025

Choose a reason for hiding this comment

eileenmcnaughton Feb 5, 2025

Choose a reason for hiding this comment

colemanw Feb 5, 2025

Choose a reason for hiding this comment

eileenmcnaughton commented Jan 2, 2025 •

edited

Loading