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

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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

mcgrogan91
Copy link

Addresses an issue reported in #130

Copy link

@alquerci alquerci left a comment

Choose a reason for hiding this comment

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

How can you prove that this modification fix the issue?

How can we describe the behaviour that make current code on master branch throw the error?

@mcgrogan91
Copy link
Author

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

@alquerci
Copy link

alquerci commented Apr 8, 2024

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
Copy link

alquerci commented Apr 8, 2024

@mcgrogan91 Yes, the patch you provide works. 💯


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

How I executed the test suite?

  1. Checkout on fix: Database Migration does set current version in DB #98 #103 where two commits of this PR applied
  2. Merge Fix docker images build to handle old linux distribution #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.

@mcgrogan91
Copy link
Author

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

@szymone
Copy link

szymone commented Jun 12, 2024

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

Comment on lines +4 to +5
* $Id$
*
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
* $Id$
*


$connection->commit();
}
}
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
}
}

Copy link
Member

@thePanz thePanz left a comment

Choose a reason for hiding this comment

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

Added a couple of minor suggestions.
WDYT?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants