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

Drop support for DBAL 2 #1570

Merged
merged 8 commits into from
Nov 7, 2022
Merged

Drop support for DBAL 2 #1570

merged 8 commits into from
Nov 7, 2022

Conversation

dmaicher
Copy link
Contributor

@dmaicher dmaicher commented Nov 1, 2022

Closes #1550

@@ -128,17 +125,6 @@ private function explainSQLServerPlatform(Connection $connection, array $query):

$stmt = $connection->executeQuery($sql, $params, $query['types']);

// DBAL 2.13 "forward compatibility" BC break handling
if ($stmt instanceof Result) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

so I guess with DBAL 3 this never worked 🤔

Copy link
Member

Choose a reason for hiding this comment

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

Then let's just throw exception in this function to make it clear this is not supported for SQLSRV? Because I'm pretty sure it ccannot work without nextRowset()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed + added an exception


class ConnectionFactoryTest extends TestCase
{
use VerifyDeprecations;

public function testContainer(): void
Copy link
Contributor Author

@dmaicher dmaicher Nov 1, 2022

Choose a reason for hiding this comment

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

I don't understand what this test actually tests. That exception throwing logic all seems DBAL related.

reading #673 I don't think its relevant anymore.

@dmaicher dmaicher force-pushed the drop_dbal_2 branch 6 times, most recently from 11ae4fe to e0dded4 Compare November 1, 2022 18:18
composer.json Outdated Show resolved Hide resolved
@dmaicher dmaicher force-pushed the drop_dbal_2 branch 4 times, most recently from b50fcd8 to 436be82 Compare November 1, 2022 19:23
@dmaicher dmaicher marked this pull request as ready for review November 1, 2022 19:25
@derrabus derrabus added this to the 2.8.0 milestone Nov 1, 2022
.github/workflows/continuous-integration.yml Show resolved Hide resolved
.github/workflows/continuous-integration.yml Outdated Show resolved Hide resolved
Controller/ProfilerController.php Show resolved Hide resolved
Resources/doc/configuration.rst Outdated Show resolved Hide resolved
Copy link
Member

@ostrolucky ostrolucky left a comment

Choose a reason for hiding this comment

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

nice work, few minor stuff

$hasPath = isset($params['path']);
$name = $hasPath ? $params['path'] : ($params['dbname'] ?? false);
if (! $name) {
throw new InvalidArgumentException("Connection does not contain a 'path' or 'dbname' parameter and cannot be created.");
}

// Need to get rid of _every_ occurrence of dbname from connection configuration and we have already extracted all relevant info from url
/** @psalm-suppress InvalidArrayOffset */
Copy link
Member

Choose a reason for hiding this comment

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

What was the error here? Does it complain about url key? Because that looks correct to me, $connection->getParams() cannot really return url key, can it? Same in DropDatabaseDoctrineCommand

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes exactly. The error was about the url key.

$connection->getParams() cannot really return url key, can it?

Actually it can 😢 I tested it. If I configure a DBAL connection using url in the bundle config then that key is also present in connection params.

Command/DoctrineCommand.php Show resolved Hide resolved
@@ -128,17 +125,6 @@ private function explainSQLServerPlatform(Connection $connection, array $query):

$stmt = $connection->executeQuery($sql, $params, $query['types']);

// DBAL 2.13 "forward compatibility" BC break handling
if ($stmt instanceof Result) {
Copy link
Member

Choose a reason for hiding this comment

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

Then let's just throw exception in this function to make it clear this is not supported for SQLSRV? Because I'm pretty sure it ccannot work without nextRowset()

Tests/DependencyInjection/DoctrineExtensionTest.php Outdated Show resolved Hide resolved
@derrabus
Copy link
Member

derrabus commented Nov 6, 2022

In composer.json, we can drop older PHPUnit versions:

"phpunit/phpunit": "^9.5.26 || ^10.0",

@derrabus derrabus merged commit 84d23e5 into doctrine:2.8.x Nov 7, 2022
@derrabus
Copy link
Member

derrabus commented Nov 7, 2022

Thank you, @dmaicher!

@dmaicher dmaicher deleted the drop_dbal_2 branch November 7, 2022 14:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants