Refresh docs about transactions #5379

greg0ire · 2022-05-05T16:20:06Z

See #4 (yes, one digit)

derrabus · 2022-05-05T16:57:57Z

docs/en/reference/transactions.rst

@@ -28,11 +28,11 @@ is functionally equivalent to the previous one:
 ::

    <?php
-    $conn->transactional(function($conn) {
+    $conn->transactional(function(Connection $conn): void {


The closure does not have to be a void, right? Isn't the returned value passed through by transactional()? Should we mention that?

Oh right sorry, I misread the code

I guess since there's no return statement in this closure and the return value isn't used anywhere, it's void. Maybe it makes sense to have two examples: one with a return value and one without (both are valid).

I added a note paraphrasing what @derrabus said, not sure if you saw it before writing that comment and if you think two examples are better, in which case I'll be happy to oblige 🙂

I started writing my comment before you addressed his comment but I submitted it after I saw your update. I still believe that two examples are better than just an explanation, they complement each other, not exclude.

@derrabus Does this resolve your question/suggestion?

greg0ire · 2022-05-05T19:03:43Z

docs/en/reference/transactions.rst

-Transaction Nesting
-------------------
+By default, transaction nesting at the SQL level with savepoints is
+disabled. The value for that setting can be set on a per-connection


I'm not sure why this is the default by the way, if someone does read the docs and nests transaction, I think they would be less surprised when seeing a savepoint than when seeing a transaction. By the way, it was a long time ago but I think I remember seeing savepoint in my logs, but I don't remember configuring them. Maybe there is (was?) a default to true in some other layer (I've checked the DoctrineBundle and the symfony recipes, and that does not seem to be the case).

I think from the perspective of the API semantics, the default behavior is potentially harmful. If a given component starts and commits its transaction, it may expect that it's committed but it may be not depending on the configuration and the current nesting level. The same is about rollback. The more I think about how it works, the less sense it makes.

Do I sense an incoming deprecation? DB2 does not support savepoints, so that would mean DB2 users would no longer have the option of having nested transactions, and this documentation block should be restored just for them:

This also means that you cannot
successfully commit some changes in an outer transaction if an
inner transaction block fails and issues a rollback, even if this
would be the desired behavior (i.e. because the nested operation is
"optional" for the purpose of the outer transaction block). To
achieve that, you need to restructure your application logic so as
to avoid nesting transaction blocks. If this is not possible
because the nested transaction blocks are in a third-party API
you're out of luck.

The default behavior should be definitely deprecated. If a platform doesn't support savepoints and there is an attempt to start a nested transaction, it should fail immediately.

The documentation block looks reasonable but it could use some cleanup in terms of language. Instead of focusing on the user ("you cannot [...] commit", "you're out of luck"), focus on the problems and the API.

Will do! (on 3.4.x)

morozov · 2022-05-05T20:30:10Z

docs/en/reference/transactions.rst

+``Doctrine\DBAL\Connection`` instance is chosen by the underlying
+platform but it is always at least ``READ_COMMITTED``.
+
+Emulated Transaction Nesting


I don't believe this mode deserves to be called an emulation. Emulation implies that the end result is achieved through some indirect means. But this mode doesn't achieve it. It's effectively a null implementation. Not sure how to name it better though.

I describe both modes under this section, but I see what you mean. Let's call the first mode "Fallback behavior"?

I'd call it a dummy mode maybe? A "fallback" implies that it's not the default behavior, and falling back still achieves the desired result (at least partially). But here it's the default mode and it only looks like nesting while it's not by any means.

E.g. (if you google "dummy"):

something designed to resemble and serve as a substitute for the real or usual thing; a counterfeit or sham.

Ok let's use that, I don't have a better suggestion right now.

docs/en/reference/transactions.rst

@derrabus

Release [3.3.7](https://github.com/doctrine/dbal/milestone/114) 3.3.7 ===== - Total issues resolved: **1** - Total pull requests resolved: **8** - Total contributors: **6** Code Style,Static Analysis -------------------------- - [5444: PHP CodeSniffer 3.7, PHPStan 1.7.13](doctrine#5444) thanks to @derrabus Static Analysis --------------- - [5426: PHPStan 1.7.9](doctrine#5426) thanks to @derrabus - [5417: Run Psalm with language level PHP 8.1](doctrine#5417) thanks to @morozov Documentation,Type Mapping -------------------------- - [5415: Update documentation for the `guid` type](doctrine#5415) thanks to @kaznovac Bug,PostgreSQL,Schema Management -------------------------------- - [5395: Check integer types via PhpIntegerMappingType](doctrine#5395) thanks to @morozov and @umherirrender Bug,PHP,PostgreSQL ------------------ - [5381: Fix using deprecated syntax](doctrine#5381) thanks to @nicolas-grekas Documentation ------------- - [5379: Refresh docs about transactions](doctrine#5379) thanks to @greg0ire CI -- - [5374: Remove CA bundle from AppVeyor](doctrine#5374) thanks to @morozov # gpg: Signature made Fri Jun 17 09:51:42 2022 # gpg: using DSA key 1BEDEE0A820BC30D858F9F0C2C3A645671828132 # gpg: Can't check signature: No public key

greg0ire added 2 commits May 5, 2022 18:19

Modernize code sample

4d1f681

Add missing words

006f227

greg0ire requested review from derrabus and morozov May 5, 2022 16:28

derrabus reviewed May 5, 2022

View reviewed changes

greg0ire commented May 5, 2022

View reviewed changes

greg0ire force-pushed the refresh-transac-docs branch from acf8dc0 to 4dceb0b Compare May 5, 2022 20:26

morozov reviewed May 5, 2022

View reviewed changes

greg0ire commented May 5, 2022

View reviewed changes

docs/en/reference/transactions.rst Outdated Show resolved Hide resolved

document transactional() behavior

a778377

greg0ire force-pushed the refresh-transac-docs branch from 4dceb0b to 1a4ca48 Compare May 5, 2022 20:35

greg0ire requested review from morozov and derrabus May 5, 2022 20:40

Document nested transactions with savepoints

a63f771

greg0ire force-pushed the refresh-transac-docs branch from 1a4ca48 to a63f771 Compare May 5, 2022 21:05

morozov approved these changes May 5, 2022

View reviewed changes

SenseException approved these changes May 5, 2022

View reviewed changes

greg0ire merged commit a1a84d1 into doctrine:3.3.x May 6, 2022

greg0ire added the Documentation label May 6, 2022

greg0ire added this to the 3.3.7 milestone May 6, 2022

github-actions bot locked as resolved and limited conversation to collaborators May 7, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refresh docs about transactions #5379

Refresh docs about transactions #5379

greg0ire commented May 5, 2022

derrabus May 5, 2022

greg0ire May 5, 2022

morozov May 5, 2022

greg0ire May 5, 2022

morozov May 5, 2022 •

edited

Loading

SenseException May 5, 2022

greg0ire May 5, 2022

morozov May 5, 2022 •

edited

Loading

greg0ire May 5, 2022

morozov May 5, 2022

greg0ire May 5, 2022 •

edited

Loading

morozov May 5, 2022 •

edited

Loading

greg0ire May 5, 2022

morozov May 5, 2022 •

edited

Loading

greg0ire May 5, 2022

Refresh docs about transactions #5379

Refresh docs about transactions #5379

Conversation

greg0ire commented May 5, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

morozov May 5, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

morozov May 5, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

greg0ire May 5, 2022 • edited Loading

Choose a reason for hiding this comment

morozov May 5, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

morozov May 5, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

morozov May 5, 2022 •

edited

Loading

morozov May 5, 2022 •

edited

Loading

greg0ire May 5, 2022 •

edited

Loading

morozov May 5, 2022 •

edited

Loading

morozov May 5, 2022 •

edited

Loading