[REF] Mapping - Check FK on delete with SQL instead of PHP (dev/core#2757)

[colemanw]

Overview

Part of an ongoing effort to clean up the DAO classes and use generic functions and events.

Before

For some reason, a php function was mimicking ON DELETE CASCADE in mySql.

After

Uses actual ON DELETE CASCADE in mySql.

Comments

The only reason I can fathom to do it in PHP rather than in SQL is to call pre and post hooks... which this wasn't.

[civibot]

(Standard links)

• If this is your first pull-request for CiviCRM, please browse CONTRIBUTING.md for information about the development and testing processes.
• If you are reviewing this pull-request, you may wish to consult the test sites and the Review Standards (long template, short template).

[eileenmcnaughton]

@seamuslee001 I'm getting confused here - we use CRM_Core_BAO_SchemaHandler::safeRemoveFK(...) to drop the fk I think but ?? to add one

[seamuslee001]

You could use this

civicrm-core/CRM/Core/BAO/SchemaHandler.php

Line 256 in 33f8687

public static function buildForeignKeySQL($params, $separator, $prefix, $tableName) {

but still not perfect

[colemanw]

@eileenmcnaughton @seamuslee001 does this PR require a schema regen?

[seamuslee001]

@colemanw whilst you looking at mapping I'm wondering if we need to modify this https://github.com/civicrm/civicrm-core/blob/master/xml/schema/Contact/SavedSearch.xml#L82 at all

[colemanw]

@seamuslee001 why would we need to modify it?

[seamuslee001]

@colemanw should it be cascade delete also rather than set null?

[eileenmcnaughton]

@colemanw I've pushed up changes to the upgrade script as discussed above

I tested locally upgrading from 5.42 to 5.43 with this version of the upgrade script
I think deleted a Mapping using api explorer (v4) and checked the fields were deleted - all good IMHO

I think @seamuslee001's question about search kit fk on mapping_id is out of scope - from memory it is used in search builder context and I have a feeling there might be some complications in answering the question about whether it should delete on cascade so it's not something we could 'quickly do while we are at it'

This is mergeable now IMHO, Since I altered it I'm added 'merge-ready' not 'merge-on-pass'

[colemanw]

Thanks @eileenmcnaughton - this looks good except is this one of those situations where regen.sh has to be run and new sql install files committed?

[eileenmcnaughton]

I don't think it does

[colemanw]

In that case this is good to go