doctrine1 icon indicating copy to clipboard operation
doctrine1 copied to clipboard

Update the export command to use a PHP8-safe PDO transaction commit path

Open mcgrogan91 opened this issue 1 year ago • 5 comments

Addresses an issue reported in https://github.com/FriendsOfSymfony1/doctrine1/issues/130

mcgrogan91 avatar Apr 05 '24 17:04 mcgrogan91

I'm not familiar enough with the internals of this package to build the test, but this behavior occurs when:

  • The Doctrine Transaction believes it is in a transaction (There is a nestinglevel)
  • The plain PHP PDO object (due to the change in PHP 8) knows that it is no longer in a transaction - due to the database auto-committing a DDL command
  • Commit is called on the connection/transaction.

I began writing a test but couldn't get the test suite to run on my machine, and wasn't sure how to mock the change in PDO behavior from 7.x to 8.0

I do see the task behavior is fixed on a build pointing at my forks branch, help figuring out how to build the test for it would be appreciated

mcgrogan91 avatar Apr 08 '24 18:04 mcgrogan91

This patch will fix the issue #98.

I just remembered that I wrote a failing test. Look at #103.

I will cherry-pick your patch and run the test.

alquerci avatar Apr 08 '24 18:04 alquerci

@mcgrogan91 Yes, the patch you provide works. :100:


Sadly, the current CI does not support its execution as missing a MySQL server.

How I executed the test suite?

  1. Checkout on #103 where two commits of this PR applied
  2. Merge #90
  3. Revert two commits of this PR
  4. Run tests suite tests/bin/test, expect failing test.
  5. Apply two commits of this PR
  6. Run tests suite tests/bin/test, expect test pass.

alquerci avatar Apr 08 '24 19:04 alquerci

I'm glad to hear your testing of it worked. What's the next step to get this moving? Right now we have our application pointing at my fork but i'd love to get us back onto the main repository before moving forward with a production deploy

mcgrogan91 avatar Apr 11 '24 15:04 mcgrogan91

Any update on this? Any chance of merging this PR?

szymone avatar Jun 12 '24 20:06 szymone