Unfortunately have to call this stuff here manually again, unlike what the Laravel trait does (which is to call its own beginTransaction method here), as we are using the with_transaction helper function and passing the callback. The helper function, in turn, calls WithTransaction::execute which handles starting and committing the actual transaction in the session.

Calling beginTransaction here instead of calling DatabaseTransactionsManager::begin directly would therefore result in attempting to start the transaction twice.

Same goes for the commit call

Per my earlier comment in beginTransaction(), should you actually pass 1 as the transaction level? Also, if you can't call beginTransaction() yourself perhaps you'd also want to manually assign $this->transactions = 1 immediately before as well:

$this->transactions = 1;
$this->transactionsManager?->begin($this->getName(), $this->transactions);

Since you're deviating from the base implementation, I think a block comment explaining why this inline duplication is necessary would help.

Wasn't sure if if it was worth not hard-coding this for future proofing reasons in case we ever start supporting nested transactions? 😬

Echoing my comments above about keeping this in sync with any changes to the commit() method and ensuring that $this->transactions also gets set back to 0. I'd suggest using the same temporary variable for clarity:

$levelBeingCommitted = $this->transactions;
$this->transactions = 0;

Noted that the second parameter here is int $level per DatabaseTransactionsManager::begin().

In the base ManagesTransactions::begin() implementation, the level is incremented before invoking DatabaseTransactionsManager::begin(). I'm not certain, but I expect that 1 would be the smallest value passed in that case. Here, you're passing 0 but setting $this->transactions to 1 a few lines below.

Also, it may be a subtle change, but it looks like the base implementation invokes PDO::commit() before DatabaseTransactionsManager::begin(). Therefore, would it make more sense to invoke Session::startTransaction() first here?

Consider:

$this->getSessionOrCreate()->startTransaction($options);
$this->transactions = 1;
$this->transactionsManager?->begin($this->getName(), $this->transactions);

The flow here does match [ManagesTransactions::commit()](https://github.com/laravel/framework/blob/v12.28.1/src/Illuminate/Database/Concerns/ManagesTransactions.php#L203] in that you invoke the driver's commit method first (analogous to PDO::commit()).

DatabaseTransactionsManager::commit() might expect two different values for int $levelBeingCommitted and int $newTransactionLevel, though. Consider the following in the base method:

[$levelBeingCommitted, $this->transactions] = [
 $this->transactions,
 max(0, $this->transactions - 1),
];

Here, you pass 0 as both the committed and new level, which is a subtle behavioral difference. While this could be replaced with 1, 0 literals, I think the code would be more self-documenting if copied the above assignment or instead did something like:

$levelBeingCommitted = $this->transactions;
$this->transactions = 0;

Either way, a comment to remind readers that MongoDB only supports a single transaction level might be helpful (if not here then perhaps in begin() where you manually assign 1 instead of incrementing).

Yeah, I figured it would be appropriate to pas 0 here twice as we have no levels, and the levels wouldn't dip below 0. But it did feel a bit icky to me so I might do what you've suggested. Also just in case we somehow do start supporting nested transactions many years down the line, this won't become one of those things causing weird behavior because we forgot about these hardcoded values.

I'm not sure if the current transaction level gets passed to the event listeners, but that was actually what prompted my comment (not so much the possibility of nested transactions in MongoDB 12.0—don't quote me on that). Something downstream of these events might be surprised to see a transaction get committed at level zero.

good point

Per my earlier comment in beginTransaction(), should you actually pass 1 as the transaction level? Also, if you can't call beginTransaction() yourself perhaps you'd also want to manually assign $this->transactions = 1 immediately before as well:

$this->transactions = 1;
$this->transactionsManager?->begin($this->getName(), $this->transactions);

Since you're deviating from the base implementation, I think a block comment explaining why this inline duplication is necessary would help.

Echoing my comments above about keeping this in sync with any changes to the commit() method and ensuring that $this->transactions also gets set back to 0. I'd suggest using the same temporary variable for clarity:

$levelBeingCommitted = $this->transactions;
$this->transactions = 0;

Looking at Laravel's ManagesTransactions, the call to transactionsManager?->begin is done after the transaction is started.

https://github.com/laravel/framework/blob/d09f1345e25d8d04c67ffa4cdd3656f5ebbce57a/src/Illuminate/Database/Concerns/ManagesTransactions.php#L124-L136

That can work for this method, but not for the transaction method, as WithTransaction::execute() both starts the transaction and calls the callback, which shouldn't happen (and is what this call handles)

So I'm struggling to think if we should keep the order consistent on our library side or have one method consistent with Laravel and the other not...

Should the $this->fireConnectionEvent('beganTransaction'); also be called at like in Laravel: https://github.com/laravel/framework/blob/d09f1345e25d8d04c67ffa4cdd3656f5ebbce57a/src/Illuminate/Database/Concerns/ManagesTransactions.php#L138

That was added in Laravel 5.4, I don't know which feature it is.

Ah, yes, forgot that one!

Note that there are a few other fireConnectionEvent calls for committing, committed, and rollingBack. Since we create our own callback in transaction(), which wraps the user's callback, I think you'll be able to work this into the code path that uses with_transaction as well.