Skip to content

[Cache] remove Doctrine DBAL < 3.3 related code #52685

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

Closed
wants to merge 1 commit into from

Conversation

xabbuh
Copy link
Member

@xabbuh xabbuh commented Nov 22, 2023

Q A
Branch? 7.0
Bug fix? no
New feature? no
Deprecations? no
Issues
License MIT

following the merge up of #52459

@carsonbot
Copy link

Hey!

Thanks for your PR. You are targeting branch "7.1" but it seems your PR description refers to branch "7.0".
Could you update the PR description or change target branch? This helps core maintainers a lot.

Cheers!

Carsonbot

@xabbuh xabbuh changed the base branch from 7.1 to 7.0 November 22, 2023 09:46
@xabbuh xabbuh modified the milestones: 7.1, 7.0 Nov 22, 2023
@@ -375,8 +375,7 @@ private function getServerVersion(): string
return $this->serverVersion;
}

// The condition should be removed once support for DBAL <3.3 is dropped
$conn = method_exists($this->conn, 'getNativeConnection') ? $this->conn->getNativeConnection() : $this->conn->getWrappedConnection();
$conn = $this->conn->getNativeConnection();
if ($conn instanceof ServerInfoAwareConnection) {
Copy link
Member

Choose a reason for hiding this comment

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

Hmm, this does not really make sense. If we call getNativeConnection(), we get the native handle of the PHP extension used, e.g. a mysqli instance or an sqlsrv resource. Those objects/resources won't ever implement ServerInfoAwareConnection. Something's off here.

Copy link
Member Author

@xabbuh xabbuh Nov 24, 2023

Choose a reason for hiding this comment

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

I guess you are right. Looks like we first need to fix it in a way similar to #52715.

nicolas-grekas added a commit that referenced this pull request Nov 24, 2023
This PR was merged into the 5.4 branch.

Discussion
----------

[Cache] fix detecting the database server version

| Q             | A
| ------------- | ---
| Branch?       | 5.4
| Bug fix?      | yes
| New feature?  | no
| Deprecations? | no
| Issues        | #52685 (comment)
| License       | MIT

Commits
-------

33a65ca fix detecting the database server version
symfony-splitter pushed a commit to symfony/cache that referenced this pull request Nov 24, 2023
This PR was merged into the 5.4 branch.

Discussion
----------

[Cache] fix detecting the database server version

| Q             | A
| ------------- | ---
| Branch?       | 5.4
| Bug fix?      | yes
| New feature?  | no
| Deprecations? | no
| Issues        | symfony/symfony#52685 (comment)
| License       | MIT

Commits
-------

33a65ca9cd5 fix detecting the database server version
@derrabus
Copy link
Member

My alternative proposal: #52720.

@xabbuh
Copy link
Member Author

xabbuh commented Nov 25, 2023

closing in favour of #52720

@xabbuh xabbuh closed this Nov 25, 2023
@xabbuh xabbuh deleted the pr-52459 branch November 25, 2023 18:03
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.

3 participants