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

Remove support for transaction nesting without savepoints #5401

Merged
merged 1 commit into from
May 17, 2022

Conversation

greg0ire
Copy link
Member

No description provided.

@greg0ire greg0ire force-pushed the mandatory-savepoints branch 2 times, most recently from b7d772a to c6fd2d5 Compare May 16, 2022 21:19
@@ -1045,39 +1039,37 @@ public function transactional(Closure $func): mixed
/**
* Sets if nested transactions should use savepoints.
*
* @deprecated No replacement planned
*
* @throws Exception
*/
public function setNestTransactionsWithSavepoints(bool $nestTransactionsWithSavepoints): void
Copy link
Member

Choose a reason for hiding this comment

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

If we keep the method, we should throw if false is passed.

Copy link
Member

Choose a reason for hiding this comment

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

Why deprecate it in the next major given that the dummy mode is already deprecated and is being removed?

Copy link
Member Author

Choose a reason for hiding this comment

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

In #5383, I tell people they should not call setNestTransactionsWithSavepoints(true) since false is the default for backward compatibility. If they do so, it means they will have one more action to perform right after upgrading to v4 before they have a working application. I'll leave you the judge of whether this is fine or not, I don't know if we should have some kind of rule about this. My concern is that if the list of such changes gets too big, the upgrade could become painful.

Copy link
Member

@morozov morozov May 17, 2022

Choose a reason for hiding this comment

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

I see, so there are two breaking changes necessary:

  1. Change the default mode from false to true in 4.0 (we're letting users opt in to the true mode in 3.x).
  2. Remove the API for changing the mode in 5.0 (it will exist in 4.0 only as a stub for backward compatibility with 3.x).

Makes sense.

@greg0ire greg0ire force-pushed the mandatory-savepoints branch from 78642ed to 81f04b3 Compare May 17, 2022 18:45
src/Connection.php Outdated Show resolved Hide resolved
@greg0ire greg0ire force-pushed the mandatory-savepoints branch from 81f04b3 to 818b75c Compare May 17, 2022 18:56
@greg0ire greg0ire added this to the 4.0.0 milestone May 17, 2022
@greg0ire greg0ire merged commit a930f76 into doctrine:4.0.x May 17, 2022
@greg0ire greg0ire deleted the mandatory-savepoints branch May 17, 2022 19:19
@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 18, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants