Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[BC] Migrate to DBAL 3 #55

Merged
merged 25 commits into from
Mar 29, 2022
Merged

[BC] Migrate to DBAL 3 #55

merged 25 commits into from
Mar 29, 2022

Conversation

Jean85
Copy link
Member

@Jean85 Jean85 commented Feb 10, 2022

This is the first attempt at a new major version, jumping directly on supporting DBAL 3.

THIS IS STILL A WIP.

There are so many BCs and various changes that I'm trying to take the chance to profoundly refactor this lib.

These are the major points:

  • Removing all drivers: some drivers are now final and cannot be extended anymore; also, DBAL 3 does not allow anymore to identify platforms by names of instaceof of the driver, but with Platform; this allows to change the approach of this lib and require only the wrapper connection, no more driver substitution
  • Removing traits: all traits are then no longer necessary, and all logic can be absorbed in the Connection class
  • Splitting retry detection logic: the old ServerGoneAwayExceptionsAwareTrait is now moved behind a dedicated GoneAwayDetector interface, that allows a basic MySQLGoneAwayDetector and the possibility to swap that out if necessary
  • Introducing Psalm: this will allow checking for types of our implementation, since now DBAL is more strictly typed

Other comments will follow inline with the code.

@Jean85 Jean85 added this to the 2.0 milestone Feb 10, 2022
@Jean85 Jean85 self-assigned this Feb 10, 2022
/** @var int */
protected $defaultFetchMode = FetchMode::ASSOCIATIVE;

public function __construct(
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Costructor is (finally!) moved from the trait into the class, and simplified.

Comment on lines +47 to +50
public function setGoneAwayDetector(GoneAwayDetector $goneAwayDetector): void
{
$this->goneAwayDetector = $goneAwayDetector;
}
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure we need this setter... Maybe it's required if we make this class final.

Comment on lines +119 to +130
public function canTryAgain(\Throwable $throwable, int $attempt, string $sql = null, bool $ignoreTransactionLevel = false): bool
{
if ($attempt >= $this->reconnectAttempts) {
return false;
}

if (! $ignoreTransactionLevel && $this->getTransactionNestingLevel() > 0) {
return false;
}

return $this->goneAwayDetector->isGoneAwayException($throwable, $sql);
}
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This method now centralizes all the retry logic, alongside with the GoneAwayDetector


namespace Facile\DoctrineMySQLComeBack\Doctrine\DBAL\Detector;

class MySQLGoneAwayDetector implements GoneAwayDetector
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is nearly the same as the old ServerGoneAwayExceptionsAwareTrait

return $this->executeWithRetry(__METHOD__, $params);
}

private function executeWithRetry(string $methodName, ...$params)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This method allows the retry in a flexible fashion. Maybe we can refactor this out or move it inside the connection?

@Jean85
Copy link
Member Author

Jean85 commented Mar 23, 2022

We should absolutely take into account doctrine/dbal#4119, since it could mean that we can intercept all disconnections at the wrapper level, simplifying the work a lot.

@Jean85
Copy link
Member Author

Jean85 commented Mar 25, 2022

Big breakthrough: with a4a70f0 I discovered that the Statement can be largely simplified, since rebinding parameters/values is no longer needed: the Driver\Statement already does that for MySQLi: https://github.com/doctrine/dbal/blob/3.3.x/src/Driver/Mysqli/Statement.php#L105-L113

While for PDO I don't know how it works, but the test is green...

Comment on lines +23 to +26
// public function execute($params = null): Result
// {
// return $this->executeWithRetry([parent::class, 'execute'], $params);
// }
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm leaving this here because we will need to re-add this method lather. I will probably try to split it into a child class, so that it could be enabled when needed, because it will trigger deprecations.

@Jean85 Jean85 marked this pull request as ready for review March 25, 2022 08:45
@Jean85 Jean85 mentioned this pull request Mar 25, 2022
@Jean85 Jean85 merged commit 5ba3551 into master Mar 29, 2022
@Jean85 Jean85 deleted the dbal_3 branch March 29, 2022 07:31
@Jean85 Jean85 mentioned this pull request Apr 1, 2022
@boesing boesing mentioned this pull request Nov 9, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants