Disable foreign key checks on purge for all mysql versions #407

croensch · 2022-10-02T11:31:45Z

For your consideration, see #272. I don't know if the discussion was a yes/no to this feature.

I can push a little refactor of getTruncateTableSQL() to make it fit better.

... when purge mode is truncate * add test for change of foreign key check when using mysql * add test for not set foreign key check when using sqlite

lib/Doctrine/Common/DataFixtures/Purger/ORMPurger.php

tests/Doctrine/Tests/Common/DataFixtures/Purger/ORMPurgerForeignKeyCheckTest.php

as suggested Co-authored-by: Grégoire Paris <postmaster@greg0ire.fr>

croensch · 2022-10-09T10:59:52Z

I initially picked up and rebased this PR to revive or end the discussion around this problem. Now that i looked into it, i understand a few things better:

The ORMPurger touches all tables anyway, so for MySQL where the TRUNCATE is a drop/re-create internally, it should theoretically not be much faster than the current workaround of running the drop & create commands. Still, it would be nice if this feature just works on MySQL too.
In AliceDataFixtures they implemented a fix referencing the original PR. We should not break that, i don't think it does, but it got me thinking... https://github.com/theofidry/AliceDataFixtures/blob/master/src/Bridge/Doctrine/Purger/Purger.php
So i like how DBUnit restores the variables state, perfectly fine pattern to be least-invasive. https://github.com/sebastianbergmann/dbunit/blob/master/src/Operation/Truncate.php
In DBUnit but also this PR i don't like how it's done around every TRUNCATE statement as it may spam the logger. I would rewrite it to call a method (like in DBUnit) before and after the tables loop.
I still think it's a good idea to sniff the connection instead of the platform since only AbstractMySQLDriver is the same since 3.0 while the AbstractMySQLPlatform is only available since DBAL 3.3, but 3.0 is the minimum dependency.

So would you mind if i rewrite it to address all this? In the same PR?

greg0ire · 2022-10-09T16:34:43Z

DBAL 3.3, but 3.0 is the minimum dependency.

I think it's fine to bump the minimum dependency to 3.3, sniffing the ~~connection~~ driver (why did you mention the connection?) is not a great idea because of middlewares that can wrap the driver in an arbitrary number of decorators.

So would you mind if i rewrite it to address all this? In the same PR?

Sure, go ahead 🙂

- sniff platform instead of driver - restore variable in MySQL session - only enable/disable once per purge

lib/Doctrine/Common/DataFixtures/Purger/ORMPurger.php

tests/Doctrine/Tests/Common/DataFixtures/Purger/ORMPurgerForeignKeyCheckTest.php

lib/Doctrine/Common/DataFixtures/Purger/ORMPurger.php

as suggested Co-authored-by: Grégoire Paris <postmaster@greg0ire.fr>

tests/Doctrine/Tests/Common/DataFixtures/Purger/ORMPurgerForeignKeyCheckTest.php

Co-authored-by: Grégoire Paris <postmaster@greg0ire.fr>

croensch · 2022-10-12T19:25:39Z

Sorry for the spam. I got nothing to add or change - it's finished for me.

Regarding the DBAL (Common?) dependency i also check the previous class in a few easy-to-delete lines. Just in case you want to postpone upping the dependency.

greg0ire · 2022-10-12T21:12:29Z

Reviewers: to be squash-merged

croensch · 2023-11-08T15:30:34Z

@SenseException @derrabus can we get another look at this?

SenseException

Having a functional test with the foreign keys would be a nice-to-have, even though I don't expect breaking changes in the methods.

derrabus

The 1.5.x branch is closed for features. We need to target 1.7.x now.

derrabus · 2023-11-13T18:50:55Z

lib/Doctrine/Common/DataFixtures/Purger/ORMPurger.php

+        $platform = $connection->getDatabasePlatform();
+
+        if (! class_exists(AbstractMySQLPlatform::class)) {
+            // before DBAL 3.3


We don't need to support ancient DBAL versions anymore.

derrabus · 2023-11-13T18:55:13Z

lib/Doctrine/Common/DataFixtures/Purger/ORMPurger.php

@@ -163,6 +169,8 @@ public function purge()
                $connection->executeStatement($platform->getTruncateTableSQL($tbl, true));
            }
        }
+
+        $this->enableForeignKeyChecksForMySQL($connection);


If anything inside the foreach throws, we leave the connection in a state of disabled foreign key checks, don't we? We should do something about that.

derrabus · 2023-11-13T18:56:15Z

tests/Doctrine/Tests/Common/DataFixtures/Purger/ORMPurgerForeignKeyCheckTest.php

+    public const TEST_TABLE_NAME = 'link';
+
+    /** @return MappingDriver&MockObject */
+    protected function getMockMetadataDriver(): MappingDriver


Suggested change

protected function getMockMetadataDriver(): MappingDriver

private function getMockMetadataDriver(): MappingDriver

derrabus · 2023-11-13T18:56:24Z

tests/Doctrine/Tests/Common/DataFixtures/Purger/ORMPurgerForeignKeyCheckTest.php

+    }
+
+    /** @return Connection&MockObject */
+    protected function getMockConnectionForPlatform(AbstractPlatform $platform): Connection


Suggested change

protected function getMockConnectionForPlatform(AbstractPlatform $platform): Connection

private function getMockConnectionForPlatform(AbstractPlatform $platform): Connection

disable foreign key checks on purge for all mysql versions

dcc8b39

... when purge mode is truncate * add test for change of foreign key check when using mysql * add test for not set foreign key check when using sqlite

greg0ire reviewed Oct 2, 2022

View reviewed changes

croensch and others added 4 commits October 2, 2022 20:11

fix types and original test

d416e02

fix types

4d610a3

as suggested Co-authored-by: Grégoire Paris <postmaster@greg0ire.fr>

fix cs errors

10cd4ef

add comment similar to mysql docs

ba2ee12

croensch added 2 commits October 10, 2022 23:27

refactor completely

b5639d3

- sniff platform instead of driver - restore variable in MySQL session - only enable/disable once per purge

restore CASCADE

1c3282e

greg0ire reviewed Oct 10, 2022

View reviewed changes

croensch commented Oct 10, 2022

View reviewed changes

tests/Doctrine/Tests/Common/DataFixtures/Purger/ORMPurgerForeignKeyCheckTest.php Outdated Show resolved Hide resolved

lib/Doctrine/Common/DataFixtures/Purger/ORMPurger.php Show resolved Hide resolved

croensch and others added 2 commits October 11, 2022 00:08

fix types and hints

0809429

as suggested Co-authored-by: Grégoire Paris <postmaster@greg0ire.fr>

fix mistake

dd9bf71

greg0ire reviewed Oct 12, 2022

View reviewed changes

tests/Doctrine/Tests/Common/DataFixtures/Purger/ORMPurgerForeignKeyCheckTest.php Outdated Show resolved Hide resolved

tests/Doctrine/Tests/Common/DataFixtures/Purger/ORMPurgerForeignKeyCheckTest.php Outdated Show resolved Hide resolved

apply suggestions on test

578459f

Co-authored-by: Grégoire Paris <postmaster@greg0ire.fr>

greg0ire approved these changes Oct 12, 2022

View reviewed changes

greg0ire requested review from derrabus and SenseException October 12, 2022 21:12

SenseException approved these changes Nov 8, 2023

View reviewed changes

derrabus changed the base branch from 1.5.x to 1.7.x November 13, 2023 18:51

derrabus changed the title ~~disable foreign key checks on purge for all mysql versions [rebased onto 1.5.x]~~ Disable foreign key checks on purge for all mysql versions Nov 13, 2023

derrabus reviewed Nov 13, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Disable foreign key checks on purge for all mysql versions #407

Disable foreign key checks on purge for all mysql versions #407

croensch commented Oct 2, 2022

croensch commented Oct 9, 2022

greg0ire commented Oct 9, 2022 •

edited

croensch commented Oct 12, 2022

greg0ire commented Oct 12, 2022

croensch commented Nov 8, 2023

SenseException left a comment

derrabus left a comment

derrabus Nov 13, 2023

derrabus Nov 13, 2023

derrabus Nov 13, 2023

derrabus Nov 13, 2023

	protected function getMockMetadataDriver(): MappingDriver
	private function getMockMetadataDriver(): MappingDriver

	protected function getMockConnectionForPlatform(AbstractPlatform $platform): Connection
	private function getMockConnectionForPlatform(AbstractPlatform $platform): Connection

Disable foreign key checks on purge for all mysql versions #407

Are you sure you want to change the base?

Disable foreign key checks on purge for all mysql versions #407

Conversation

croensch commented Oct 2, 2022

croensch commented Oct 9, 2022

greg0ire commented Oct 9, 2022 • edited

croensch commented Oct 12, 2022

greg0ire commented Oct 12, 2022

croensch commented Nov 8, 2023

SenseException left a comment

Choose a reason for hiding this comment

derrabus left a comment

Choose a reason for hiding this comment

derrabus Nov 13, 2023

Choose a reason for hiding this comment

derrabus Nov 13, 2023

Choose a reason for hiding this comment

derrabus Nov 13, 2023

Choose a reason for hiding this comment

derrabus Nov 13, 2023

Choose a reason for hiding this comment

greg0ire commented Oct 9, 2022 •

edited