Upgrade scripts disable foreign key checks

[jantzenw]

Magento\Framework\DB\Adapter\Pdo\Mysql::startSetup is disabling foreign key checks, causing deletions in upgrade scripts to not propagate. This behavior was introduced in Magento CE 1.6 (August 2011) and should be revisited.

Preconditions

Magento CE 2.1.6 for below example, but the behavior seems to have existed since Magento CE 1.6+.

Steps to reproduce

This can be reproduced with deletion of any entity in an upgrade script, but here is one example:

1. Create a new customer group.
2. Add product prices and catalog price rules that use this customer group.
3. Attempt to delete the customer group(s) in an upgrade script. Example:

$setup->startSetup();
...
$this->groupRepository->delete($customerGroup);
...
$setup->endSetup();

Expected result

The customer group is deleted, along with all database entries that have a foreign key on this customer group. This includes entries in tables like catalog_product_price_index, catalog_product_index_price_idx, catalogrule_customer_group and salesrule_customer_group, among others.

Actual result

The customer group is deleted, but entries remain in the above tables with foreign keys, causing severe downstream issues. Using the above example, deleting a customer group causes errors when saving products because a foreign key constraint fails:

SQLSTATE[23000]: Integrity constraint violation: 1452 Cannot add or update a child row: a foreign key constraint fails (`catalogrule_group_website`, CONSTRAINT `CATRULE_GROUP_WS_CSTR_GROUP_ID_CSTR_GROUP_CSTR_GROUP_ID` FOREIGN KEY (`customer_group_id`) REFERENCES `customer_group` (`customer_group_id`), query was: INSERT INTO `catalogrule_group_website` (`rule_id`, `customer_group_id`, `website_id`) SELECT DISTINCT  `catalogrule_product`.`rule_id`, `catalogrule_product`.`customer_group_id`, `catalogrule_product`.`website_id` FROM `catalogrule_product` WHERE (1495049620 >= from_time AND ((1495049620 <= to_time AND to_time > 0) OR to_time = 0)) ON DUPLICATE KEY UPDATE `rule_id` = VALUES(`rule_id`), `customer_group_id` = VALUES(`customer_group_id`), `website_id` = VALUES(`website_id`)

Root Cause

The issue is ultimately due to this code:

// \Magento\Framework\DB\Adapter\Pdo\Mysql::startSetup
public function startSetup()
{
	...
	$this->rawQuery("SET @OLD_FOREIGN_KEY_CHECKS=@@FOREIGN_KEY_CHECKS, FOREIGN_KEY_CHECKS=0");
	...
}

// \Magento\Framework\DB\Adapter\Pdo\Mysql::endSetup
public function endSetup()
{
	...
	$this->rawQuery("SET FOREIGN_KEY_CHECKS=IF(@OLD_FOREIGN_KEY_CHECKS=0, 0, 1)");
	...
}

startSetup disables foreign key checks.

Workaround

Enable foreign key checks before deleting the entities in the upgrade script:

...
$setup->getConnection()->query('SET @TEMPORARY_FOREIGN_KEY_CHECKS=@@FOREIGN_KEY_CHECKS, FOREIGN_KEY_CHECKS=1');
$this->groupRepository->delete($customerGroup);
$setup->getConnection()->query('SET FOREIGN_KEY_CHECKS=IF(@TEMPORARY_FOREIGN_KEY_CHECKS=0, 0, 1)');
...

Summary

It seems that disabling foreign key checks should be the responsibility of the developer writing the upgrade script, because it would be immediately obvious that they would need to do so to get their script to complete. With the current behavior, unaware developers can delete data with foreign keys disabled, possibly causing severe issues that are not immediately noticeable and are far downstream from the root cause.

This unexpected behavior can cause severe issues and should be discussed. If it is not possible to modify this behavior, an explanation would be appreciated.

[magento-engcom-team]

@jantzenw, thank you for your report.
We've created internal ticket(s) MAGETWO-80614 to track progress on the issue.

[hostep]

@jantzenw, just for information, there was a blogpost about this issue on Magento's Devblog a couple of weeks after you created this issue: https://community.magento.com/t5/Magento-DevBlog/Mysterious-startSetup-and-endSetup-Methods/ba-p/68483
I think the recommendations made in that article are correct, just don't use the startSetup and endSetup methods when you don't need them.

Unfortunately, I've seen them being used all over the place, in third party extensions, in tutorials, in blogposts, and I've used them myself because I thought you were supposed to use them, ...

[lfolco]

I'll take this one #squashtoberfest

[okorshenko]

@lfolco please accept the invite to magento2 repo

[lfolco]

So to confirm the purpose of this ticket is to remove the foreign key checks? Should we consider adding any new methods that add the foreign key checks in case someone needs them?

[jantzenw]

Perhaps nothing should change and developers should simply not call startSetup and endSetup unless they need to. But almost everyone does, and almost no one should. I think it is because these functions sound necessary, so most developers are fooled.

Perhaps it could be less misleading, possibly by deprecating these functions and replacing their logic with functions named like this:

disableForeignKeyChecks
disableAutoValueOnZero
resetForeignKeyChecks
resetAutoValueOnZero

[josephmcdermott]

I agree with @jantzenw, these methods "sound necessary" due to their awful naming: startSetup and endSetup. The Magento 2 core codebase is even riddled with them when they are not always necessary, so whenever a new Developer looks for an example to base his or her Install/Upgrade on they would naturally assume to include the startSetup and endSetup.

1. The startSetup and endMethod methods should be deprecated/removed.
2. Each usage of these methods should be removed from the core codebase.
3. In (2) if the functionality is in fact required, one of the four explicit methods should be called as suggested by @jantzenw to achieve the specific purpose.

[magento-engcom-team]

@jantzenw We are closing this issue now, as this seems to be a correct behavior.
@hostep @josephmcdermott Thanks for collaboration!

[josephmcdermott]

@magento-engcom-team it's a "correct behaviour" but that's not the point on this one, the fact is it is a misleading method that should be refactored as per the suggestion from @jantzenw - is this in progress or is this issue now "resolved" because its not a bug per se?

[hostep]

@josephmcdermott & @jantzenw: if you want these things to happen, I think it's best if you submit a PR with the changes you guys are suggesting, PR's are getting much more attention from the Magento devs then discussions in issues unfortunately.

[josephmcdermott]

Noted, thanks @hostep, let's see how the PR fares 👍
#14843