Introduce a Transaction object

[BenMorel]

This is a proposal for a Transaction object, as suggested by @guilhermeblanco and @beberlei in doctrine/orm#949; this is a foundation for the proposed ORM feature. This supersedes #571.

This PR makes the following changes to Connection:

• Adds the following method:
  • getTransactionManager() : returns the TransactionManager
• Deprecates the following methods, in favour of their counterparts on Transaction:
  • commit()
  • rollBack()
  • setRollbackOnly()
  • isRollbackOnly()
• Deprecates the following methods, in favour of their counterparts on TransactionManager:
  • beginTransaction()
  • isTransactionActive()
  • getTransactionNestingLevel()
  • setNestTransactionsWithSavepoints()
  • getNestTransactionsWithSavepoints()

The new way of dealing with transactions is now:

$transactionManager = $connection->getTransactionManager();
$transaction = $transactionManager->beginTransaction();
// ...
$transaction->commit();

A fluent API is available to configure a transaction:

$transaction = $transactionManager->createTransaction()
    ->withIsolationLevel(Connection::TRANSACTION_SERIALIZABLE)
    ->begin();

Calls to commit() and rollback() are automatically propagated to nested transactions:

$transaction1 = $transactionManager->beginTransaction();
$transaction2 = $transactionManager->beginTransaction();
$transaction1->commit(); // will commit $transaction2 then $transaction1

Overall, it's not a complicated change, does not introduce any BC break, and passes all existing tests.

[doctrinebot]

Hello,

thank you for creating this pull request. I have automatically opened an issue
on our Jira Bug Tracker for you. See the issue link:

http://www.doctrine-project.org/jira/browse/DBAL-947

We use Jira to track the state of pull requests and the versions they got
included in.

[Ocramius]

@BenMorel this looks very clean! Nice work!

[Ocramius]

Should probably be on the TransactionManager, no?

[BenMorel]

So far, I've considered the TransactionManager to be a lower-level class, that we should not have to access directly, as I thought it might be cumbersome to have work with a reference to the TransactionManager in addition to the Connection in our apps. But I theoretically agree with you. What do others think?

[Ocramius]

If the API needs to be exposed here, that can be done later as well. Creating the API on the TransactionManager for now would avoid adding more public methods on the Connection...

[BenMorel]

That's true. Let's remove it for now then!

[BenMorel]

Actually, there already is a createTransaction() method on the TransactionManager. This one does create the actual Transaction, whereas Connection::createTransaction() returns a TransactionDefinition that can later start the actual transaction using begin().

So there is a naming conflict here. I think TransactionManager::createTransaction() should be kept as is, because it creates the actual Transaction object from a TransactionDefinition:

public function createTransaction(TransactionDefinition $definition)

But I can't think of a good name for a method that would return the TransactionDefinition. Would something like prepareTransaction() fit?

$transactionManager->prepareTransaction()->begin();

[stof]

it could be a solution. Another suggestion could be buildTranslation (maybe renaming TransactionDefinition to TransactionBuilder as it is a builder object for the Transaction). this would match the naming used in the Symfony Validator 2.5 API for violations

[BenMorel]

The builder is a good idea, I've actually extracted the TransactionBuilder out of the TransactionDefinition: the TransactionDefinition is still there but is now immutable, and can safely be shared between transactions without carrying the risk to be altered.

Re-reading the whole thing, I've come to realize that the existing TransactionManager::createTransaction() should actually be called beginTransaction(), so I've renamed it. This method can also now be called directly if we want to begin a transaction with the defaults parameters, without using the builder:

$transactionManager->beginTransaction();

TransactionManager::createTransaction() now returns the TransactionBuilder; I've chosen to keep the name createTransaction() for now, as it feels more natural to me:

$transactionManager->createTransaction()                       // TransactionBuilder
    ->withIsolationLevel(Connection::TRANSACTION_SERIALIZABLE) // TransactionBuilder
    ->begin();                                                 // Transaction

@stof What do you think?

[Ocramius]

@BenMorel just looking at this again: do you think we can extract the newly introduced methods into interfaces? Would it makes sense?

[BenMorel]

@Ocramius What's the motivation behind this? If there is a use case, why not, otherwise I'm happy to keep it simple as it is!

[BenMorel]

As mentioned in doctrine/orm#949, I think the last change we could make here is to replace the explicit API in the TransactionBuilder:

->withIsolationLevel(Connection::TRANSACTION_SERIALIZABLE)

with a generic API like:

->with(Connection::ISOLATION_LEVEL, Connection::TRANSACTION_SERIALIZABLE);

We could now even further simplify the whole thing by replacing the TransactionDefinition with a simple associative array!

[stof]

@BenMorel -1 for the generic API. The explicit API is much easier to use for people (decovrability, IDE completion, etc...).

Regarding the ORM stuff, @guilhermeblanco was suggesting to have an Doctrine\ORM\Transaction object handling ORM-specific stuff for the transactions (it would extend the DBAL one of course)

[BenMorel]

@stof While I tend to agree with you on this point, I've weighted the pros and cons of both approaches, and I really like the simple configuration array. I've just pushed a commit that implements this, can you please have a look? I'll be happy to revert it if you are really against it.

[stof]

the goal of having a builder opposed to providing a $options argument in the constructor is precisely about providing a better API for the developer, so that he knows what can be done.

And having constants whch are a mix of config values and of config keys is not a good DX either (they can't even look at the available constants to know which settings are available).

so for me, such generic builder API is a big -1. It totally defeats its purpose

[BenMorel]

Ok, back to the drawing board then. I'll see what can be done without adding too much complexity...

[stof]

Well, I think we might drop the Configuration class and use an array here (taking care of the default values when being passed to the constructor though, which is not done in your code). But the builder is much nicer IMO

[BenMorel]

We don't need to add default values in the constructor, as every code consuming the transaction configuration will check if the given configuration is set, like in the TransactionManager:

if (isset($configuration[Connection::ISOLATION_LEVEL])) {
    $this->connection->setTransactionIsolation($configuration[Connection::ISOLATION_LEVEL]);
}

[BenMorel]

Thinking about it again, the solution that would check all the boxes would then be to keep the configuration array for its simplicity (we could still call TransactionManager::beginTransaction() directly with a configuration array if we know what we're doing), but to have TransactionBuilder exhibit an explicit API with methods such as withIsolationLevel() and internally populating the configuration array.

We would provide a base TransactionBuilder in the DBAL, accessible through the TransactionManager:

$transaction = $transactionManager->createTransaction()
    ->withIsolationLevel(Connection::TRANSACTION_SERIALIZABLE)
    ->begin();

In the ORM, we would create a ORM\TransactionBuilder extending the DBAL\TransactionBuilder, accessible through the EntityManager, and adding ORM-specific methods:

$transaction = $entityManager->createTransaction()
    ->withIsolationLevel(Connection::TRANSACTION_SERIALIZABLE)
    ->withDefaultLockMode(LockMode::PESSIMISTIC_WRITE)
    ->begin();

Is that OK for you, @stof ?

[stof]

yeah, looks good (and please don't introduce constants for the array keys)

[BenMorel]

Great then, I'll give it a go. I won't introduce constants in the Connection, but I think it would be better to have constants for all available configuration keys in the TransactionBuilder classes, otherwise we will have to repeat the configuration key strings in different places, with the risk of mistake it carries.

[BenMorel]

Here it is. It's a simple modification after all, and creating the ORM\TransactionBuilder should be as simple as:

class TransactionBuilder extends \Doctrine\DBAL\TransactionBuilder
{
    const DEFAULT_LOCK_MODE = 'orm.default-lock-mode';

    public function withDefaultLockMode($lockMode)
    {
        return $this->with(self::DEFAULT_LOCK_MODE, $lockMode);
    }
}

The generic with() method is now protected, so the consumer of the TransactionBuilder will only see the explicit methods.

[BenMorel]

Any update on this PR? If anyone has reserves about it, I would be happy to further discuss this, otherwise it would be great if we could merge this one and doctrine/orm#949.

This would really be a useful addition to Doctrine.

[Ocramius]

@BenMorel it won't land in 2.5.0 for now, we'll have to delay the merge.

[BenMorel]

@Ocramius When is 2.5 supposed to be released?

[Ocramius]

@BenMorel as soon as possible :-\

[stof]

When is 2.5 supposed to be released?

We are already in RC2, meaning it is feature-frozen

[BenMorel]

@morozov Has this been implemented elsewhere?

[morozov]

No. GitHub closed this PR because I removed the develop branch.

[guilhermeblanco]

@morozov by removing the develop branch you just broke ORM master... =(

[Ocramius]

We'll have to retarget ORM master to use DBAL 3.x 😅

…

On Mon, Nov 4, 2019, 02:13 Guilherme Blanco ***@***.***> wrote: @morozov <https://github.com/morozov> by removing the develop branch you just broke ORM master... =( — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#634?email_source=notifications&email_token=AABFVEA7GLZBS4NCXTMGZEDQR5LKVA5CNFSM4AR5GNR2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEC6AVXQ#issuecomment-549194462>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AABFVEGOPKACT5IDAVL7DMLQR5LKVANCNFSM4AR5GNRQ> .

[guilhermeblanco]

What about the work done there?

[morozov]

@guilhermeblanco develop just got merged into master. All the work is there.

[greg0ire]

So this should be retargetted to master, then reopened? Or should develop be re-created from master?

[morozov]

So this should be retargetted to master, then reopened? Or should develop be re-created from master?

Apparently, in the reverse order. Only open PR can be retargeted. I've created the develop branch temporarily to identify existing PRs and retarget them. Apart from that, develop is not needed until 3.0.0 is released.

[morozov]

Closing as stale.

[BenMorel]

@morozov Is there still any interest in this? I'm still interested in pushing this idea, but never managed to get enough traction.

[morozov]

I still don't understand what problem it solves. So it would make sense to resume the discussion with an RFC.