make "use_savepoints = true" no-op with DBAL 4 #1773
Conversation
|
i'm fine with this |
| if (isset($connection['use_savepoints'])) { | ||
| // DBAL >= 4 always has savepoints enabled. So we only need to call "setNestTransactionsWithSavepoints" for DBAL < 4 | ||
| if (method_exists(Connection::class, 'getEventManager')) { | ||
| if ($connection['use_savepoints']) { |
There was a problem hiding this comment.
due to empty it was actually never calling setNestTransactionsWithSavepoints(false). So I kept this behavior the same
Hm but then we still have deprecations? The idea was to just have |
| $def->addMethodCall('setNestTransactionsWithSavepoints', [$connection['use_savepoints']]); | ||
| } | ||
| } elseif (! $connection['use_savepoints']) { | ||
| throw new LogicException('The "use_savepoints" option can only be set to "true" when using DBAL >= 4'); |
There was a problem hiding this comment.
Note that this is technically a breaking change for people on DBAL 4 using use_savepoints = false. As setNestTransactionsWithSavepoints was never called in those cases it did not result in any error. But this seems odd to me so I think having an error in those cases is the correct behavior.
Do you mean with DBAL 3 or 4? If not now, when do you plan to deprecate that configuration node? |
|
@greg0ire Has a point. There is no point having use_savepoins (neither true nor false) in DBAL 4, because it's not configurable. When would we be able to remove this config node if people are fine with using it with dbal 4? What's your struggle in configuring bundles supporting dbal3+4? With PHP config or TestKernel it should be quite easy to have dynamic switch. |
|
I was thinking to deprecate it once we drop DBAL 3 support 🤔
Yeah its doable but still a bit of a hassle 😢 |
|
I'm just thinking out loud: why don't we turn on savepoints by default? Do you think there would be some side-effects? |
Possibly there could be some behavioral changes when doing nested transactions with rollbacks or so 🤔 Checking https://github.com/doctrine/dbal/blob/db922ba9436b7b18a23d1653a0b41ff2369ca41c/src/Connection.php#L1380 @greg0ire do you think it would be fine to enable savepoints by default in a minor update? |
|
I don't think we should do that in behalf of users, no, otherwise we would have made |
|
It would be nice if new projects (using DBAL 4) will not immediately see a deprecation caused by symfony/recipes#1290 |
Co-authored-by: Grégoire Paris <postmaster@greg0ire.fr>
Maybe it's time to create a new major version of the bundle, that would drop DBAL 3 & 4, while still maintaining this one for a while, just like we do for ORM and DBAL? That way you could have a dedicated recipe. |
While working on some bundles that support DBAL 3 and DBAL 4 I noticed that its quite a struggle to activate savepoints in a deprecation free way 😕
So I agree with @derrabus (see symfony/recipes#1290 (comment)) and propose to make this no-op with DBAL 4.
Fixes #1758