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

[REF] Cleanup BAO::del() functions with unnecessary FK checks (dev/core#2757) #21199

Merged
merged 1 commit into from
Sep 6, 2021

Conversation

colemanw
Copy link
Member

@colemanw colemanw commented Aug 20, 2021

Overview

Broken off from a larger cleanup PR #21200. These are the non-no-brainers which a reviewer should double-check for accuracy.

Before

These were using PHP to enforce key constraints better taken care of by MySQL.

After

Just leave it to MySQL to do its thing with foreign key checks and deprecate the functions in favor of generics.

Comments

A reviewer should note that civicrm_entity_tag uses ON DELETE CASCADE, making the php function here redundant. And PremiumsProduct does NOT cascade, again making this php function redundant.

These were using PHP to enforce key constraints better taken care of
by MySQL.
@civibot
Copy link

civibot bot commented Aug 20, 2021

(Standard links)

@civibot civibot bot added the master label Aug 20, 2021
@@ -957,6 +957,7 @@ public static function deleteRecord(array $record) {
if (empty($record['id'])) {
throw new CRM_Core_Exception("Cannot delete {$entityName} with no id.");
}
CRM_Utils_Type::validate($record['id'], 'Positive');
Copy link
Member Author

Choose a reason for hiding this comment

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

This is the only thing I moved into the generic function from Tag::del() which seems like a reasonable sanity check.

* @throws \CRM_Core_Exception
*/
public static function del($productID) {
//check dependencies
Copy link
Contributor

Choose a reason for hiding this comment

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

OK - so to confirm this one it's a case of doing an r-run to verify that I can't delete a product if a premiums product entry exists.

I believe that the premiums product is the fact that someone 'bought' that product but I have to confirm that !

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, something like that :)

@colemanw colemanw changed the title Core - Cleanup BAO::del() functions with unnecessary FK checks [REF] Cleanup BAO::del() functions with unnecessary FK checks Aug 22, 2021
@colemanw colemanw changed the title [REF] Cleanup BAO::del() functions with unnecessary FK checks [REF] Cleanup BAO::del() functions with unnecessary FK checks (dev/core#2757) Aug 22, 2021
@eileenmcnaughton
Copy link
Contributor

I tested both of these (r-run using apiv4) & confirmed the product wouldn't delete when assigned & the entity tags were deleted

@eileenmcnaughton eileenmcnaughton merged commit c4a677f into civicrm:master Sep 6, 2021
@eileenmcnaughton eileenmcnaughton deleted the fkDeleteBetter branch September 6, 2021 05:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants