Closes #3405976
Merge request reports
Activity
287 292 */ 288 293 public function __destruct() { 294 // Ensure all still-open transactions get auto-committed. Usually, this 295 // happens when the Transaction::__destruct() method is invoked, but during 296 // shutdown the object transaction order is unreliable. If the connection 297 // is destroyed first, we need to make sure to auto-commit all still-open 298 // transactions. 299 // Also see https://www.drupal.org/project/drupal/issues/1608374. 300 if ($this->isTransactionManagerStackActive) { 301 $this->commitTransactionOnDestruct(); 302 } 303 else { 304 foreach (array_reverse($this->transactionLayers) as $name => $active) { 305 $this->popTransaction($name); 306 } 307 } - Comment on lines +303 to +307
303 else { 304 foreach (array_reverse($this->transactionLayers) as $name => $active) { 305 $this->popTransaction($name); 306 } 307 } 303 // Ensure the deprecation layer does not have any transactions. 304 foreach (array_reverse($this->transactionLayers) as $name => $active) { 305 $this->popTransaction($name); 306 }
286 291 * Ensures that the client connection can be garbage collected. 287 292 */ 288 293 public function __destruct() { 294 // Ensure all still-open transactions get auto-committed. Usually, this 295 // happens when the Transaction::__destruct() method is invoked, but during 296 // shutdown the object transaction order is unreliable. If the connection 297 // is destroyed first, we need to make sure to auto-commit all still-open 298 // transactions. 299 // Also see https://www.drupal.org/project/drupal/issues/1608374. 300 if ($this->isTransactionManagerStackActive) { 301 $this->commitTransactionOnDestruct(); 302 } 303 else { 304 foreach (array_reverse($this->transactionLayers) as $name => $active) { Maybe add
// @phpstan-ignore-next-line
? Also to the line below. Will help avoiding to touch PHPStan baseline.See https://www.drupal.org/project/drupal/issues/3263109#comment-15207671 and later comments.
1691 1722 } 1692 1723 } 1693 1724 1725 /** 1726 * Commit on destruction if a client transaction is still active. 1727 * 1728 * This method should only be called by __destruct(). 1729 * 1730 * @internal 1731 */ 1732 protected function commitTransactionOnDestruct(): void { 1733 $this->connection->commit(); I think issue is that end of script destruction is not in a determined order so the transaction manager might already have been destroyed and be null.
Likewise if you try this from transaction manager the connection object isn’t guaranteed to be around and could already have been destroyed and therefore be null
It looks like the Connection object can have a reference to the transaction manager, which means it's not being destroyed, right? So
if ($this->$transactionManager) { ...
then use it, otherwise callconnection->commit()
. We'd look at the property rather than calling the service method ($this->transactionManager()
) because otherwise we run into to the problem mentioned... The service method could end up generating a new transaction manager. Ideal solution: Actually inject the service instead of lazily generating one, so we always have a reference to it and it's not being destroyed before the Connection object.Actually inject the service instead of lazily generating one, so we always have a reference to it and it's not being destroyed before the Connection object.
Destruct doesn’t happen in order during script shutdown so even if you had a reference to something it might have been destroyed already and become NULL.
It does now make me think you can’t actually guarantee the connection object itself is still available. Feels like all bets are off in this one.
Edited by Jason Woods
added 20 commits
-
c647edfa...3df04e71 - 19 commits from branch
project:11.x
- 462a84e3 - Merge remote-tracking branch 'origin/11.x' into 3405976-mondrake-approach
-
c647edfa...3df04e71 - 19 commits from branch
added 321 commits
-
462a84e3...03bdedd3 - 320 commits from branch
project:11.x
- b7411771 - Merge branch '11.x' into '3405976-mondrake-approach'
-
462a84e3...03bdedd3 - 320 commits from branch