[Addition] Remove bug disabling foreign keys #33
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
By calling `startSetup` a second time, this is unknowingly disabling foreign keys. This can be proven by following the stack trace:
`$setup` or `$installer` is of type `\Magento\Framework\Setup\SchemaSetupInterface`.
```
-> % grep -rin 'implements SchemaSetupInterface' vendor/magento
vendor/magento/magento2-base/setup/src/Magento/Setup/Module/Setup.php:14:class Setup extends \Magento\Framework\Module\Setup implements SchemaSetupInterface
```
Which takes us to `Magento\Setup\Module\Setup`, but really we're after the class it extends (`\Magento\Framework\Module\Setup`) because this is where the `startSetup` method is implemented:
```
// \Magento\Framework\Module\Setup Line 173
/**
* Prepare database before install/upgrade
*
* @return $this
*/
public function startSetup()
{
$this->getConnection()->startSetup();
return $this;
}
```
The `getConnection` method here returns the type `\Magento\Framework\DB\Adapter\AdapterInterface`. After a quick grep, knowing that we are upgrading the schema of the DB we know this is coming from the MySql Adapter.
```
-> % grep -rin 'implements AdapterInterface' vendor/magento
vendor/magento/module-elasticsearch-7/SearchAdapter/Adapter.php:22:class Adapter implements AdapterInterface
vendor/magento/framework/Code/Minifier/Adapter/Js/JShrink.php:15:class JShrink implements AdapterInterface
vendor/magento/framework/Code/Minifier/Adapter/Css/CSSmin.php:14:class CSSmin implements AdapterInterface
vendor/magento/framework/Image/Adapter/AbstractAdapter.php:19:abstract class AbstractAdapter implements AdapterInterface
vendor/magento/framework/Translate/AbstractAdapter.php:12:abstract class AbstractAdapter extends \Zend_Translate_Adapter implements AdapterInterface
vendor/magento/framework/DB/Adapter/Pdo/Mysql.php:46:class Mysql extends \Zend_Db_Adapter_Pdo_Mysql implements AdapterInterface
vendor/magento/module-elasticsearch/Elasticsearch5/SearchAdapter/Adapter.php:20:class Adapter implements AdapterInterface
vendor/magento/magento2-base/setup/src/Magento/Setup/Module/I18n/Parser/Adapter/AbstractAdapter.php:14:abstract class AbstractAdapter implements AdapterInterface
vendor/magento/magento2-base/setup/src/Magento/Setup/Test/Unit/Module/I18n/Parser/ParserTest.php:184:class AdapterStub implements AdapterInterface
```
On line 2917 of the `\Magento\Framework\DB\Adapter\Pdo\Mysql` class we see the following:
```
/**
* Run additional environment before setup
*
* @return $this
*/
public function startSetup()
{
$this->rawQuery("SET SQL_MODE=''");
$this->rawQuery("SET @OLD_FOREIGN_KEY_CHECKS=@@FOREIGN_KEY_CHECKS, FOREIGN_KEY_CHECKS=0");
$this->rawQuery("SET @OLD_SQL_MODE=@@SQL_MODE, SQL_MODE='NO_AUTO_VALUE_ON_ZERO'");
return $this;
}
/**
* Run additional environment after setup
*
* @return $this
*/
public function endSetup()
{
$this->rawQuery("SET SQL_MODE=IFNULL(@OLD_SQL_MODE,'')");
$this->rawQuery("SET FOREIGN_KEY_CHECKS=IF(@OLD_FOREIGN_KEY_CHECKS=0, 0, 1)");
return $this;
}
```
The issue here is that running this twice (which is happening on the proposed lines being removed) will end up setting `OLD_FOREIGN_KEY_CHECKS=0` on the second time, so when the `endSetup` method is run `FOREIGN_KEY_CHECKS=IF(@OLD_FOREIGN_KEY_CHECKS=0, 0, 1)` will set foreign keys = 0.
This has the ability to unknowingly create major data integrity issues and should be resolved immediately.
Remove not needed endSetup call
|
@marynychsv @virtual97, could you review this PR? |
|
Hello @ihor-sviziev , We have created a ticket based on your report. Thank you. |
|
@virtual97 any updates? |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is addition to #30 - removes also engSetup statement