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

Move DAO function to DAO class, call it from Merge class #12340

Merged
merged 1 commit into from
Jul 2, 2018

Conversation

eileenmcnaughton
Copy link
Contributor

Overview

Moves a function to a more logical place, deprecates a specific old hook usage (hook merge in cidRefs context)

Before

Code works & test CRM_Dedupe_MergerTest::getCidRefs passes. Code is less logical

After

Code works & test CRM_Dedupe_MergerTest::getCidRefs passes. Code is more logical. Poorly thought out hook call is deprecated and that same hook is no longer called from an unrelated context.

Technical Details

This change is primarily moving most of the contents of cidRefs (which is well tested already) to CRM_Core_DAO::getReferencesToContactTable. In addition addCustomTablesExtendingContactsToCidRefs is moved to CRM_Core_DAO (since otherwise the 'self::' call doesn't work.

The CRM_Dedupe_Merger::cidRefs function is retained as, in addition to calling the new function,
it calls the hook

CRM_Utils_Hook::merge('cidRefs', $contactReferences); 

That hook is poorly thought out as the merge hook is called in different contexts with completely different expectations. In this context the entityTypes hook is preferred. After a number releases with it deprecated we can remove the whole function.

In the meantime the unrelated function CRM_Logging_Schema::getLogTablesForContact() no longer calls that hook

Comments

@civibot
Copy link

civibot bot commented Jun 19, 2018

(Standard links)

@eileenmcnaughton
Copy link
Contributor Author

test this please

* schema to derive this.
*/
public static function getReferencesToContactTable() {
if (isset(\Civi::$statics[__CLASS__]) && isset(\Civi::$statics[__CLASS__]['contact_references'])) {
Copy link
Contributor Author

@eileenmcnaughton eileenmcnaughton Jun 21, 2018

Choose a reason for hiding this comment

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

@totten so I'm curious about converting this to a cache call

In theory the things that would change the list of tables here are

  1. a new custom table being created
  2. a change to core schema
  3. a change to hook-declared entity types.

3 breaks down into
a) new extension installed
b) new extension uninstalled
c) extension upgrade WITH upgrade extension script run
d) extension upgrade without any script run
e) main civicrm upgrade script runs and hooks are called in upgrade mode - causing the list of cached extension functions to be built without extension hooks being called and hence to be missing some

Copy link
Contributor Author

Choose a reason for hiding this comment

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

(note I think this PR is mergeable regardless of my tangent)

@eileenmcnaughton
Copy link
Contributor Author

@monishdeb @jitendrapurohit @seamuslee001 any of you up to review this - it's simpler than it looks - just moving code around

Copy link
Contributor

@jitendrapurohit jitendrapurohit left a comment

Choose a reason for hiding this comment

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

Manually tested the dedupe merge process and looks good to me. +1 fom me in merging this PR.

@eileenmcnaughton eileenmcnaughton merged commit 694ccbf into civicrm:master Jul 2, 2018
@eileenmcnaughton eileenmcnaughton deleted the merge_cleanup branch July 2, 2018 09:33
@eileenmcnaughton
Copy link
Contributor Author

thanks @jitendrapurohit

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants