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

Use AbstractDriverMiddleware class instead of own implementation #810

Merged
merged 3 commits into from
Feb 19, 2024

Conversation

oleg-andreyev
Copy link
Contributor

@oleg-andreyev oleg-andreyev commented Feb 16, 2024

Use AbstractDriverMiddleware class instead of own implementation, by reusing common abstract class it's possible to unwind wrappedDriver.

I have a case DamienHarper/auditor#184 when auditor & sentry don't play well.

  • While working on fix noticed that sentry does not utilize AbstractDriverMiddleware and implements everything.
  • Also it's impossible to detect if driver was wrapped.

…reusing common abstract class it's possible to unwind wrappedDriver
Copy link
Collaborator

@Jean85 Jean85 left a comment

Choose a reason for hiding this comment

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

This seems a very interesting approach! We should check if this limits compatibility with DBAL in any way, otherwise LGTM! 👍

@ste93cry
Copy link
Collaborator

The abstract class was introduced in DBAL 3.3, while we also support previous versions. Either we raise the requirement in the composer.json (technically speaking it doesn't change anything because it's a dev dependency, but at least we make it clear to the reader that we support that minimum version), or we cannot proceed. According to Packagist, the only relevant change is that we would drop the support for Composer 1, which imho is acceptable.

Also it's impossible to detect if driver was wrapped.

I fail to understand how this change would be helpful in this case. There is no public API to get the wrapped driver, and checking whether AbstractDriverMiddlware class is extended or not has nothing to do with it.

@oleg-andreyev
Copy link
Contributor Author

While there is no public API (which is a miss of DBAL), if connection is instance of is AbstractDriverMiddleware there is $wrappedDriver and you could get it wil reflection (I know it's bad)

@oleg-andreyev
Copy link
Contributor Author

I've raised issue about missing API: doctrine/dbal#6308

@ste93cry ste93cry merged commit b42a9bb into getsentry:master Feb 19, 2024
33 checks passed
@@ -41,7 +41,7 @@
"symfony/security-http": "^4.4.20||^5.0.11||^6.0||^7.0"
},
"require-dev": {
"doctrine/dbal": "^2.13||^3.0",
"doctrine/dbal": "^2.13||^3.3",
Copy link
Member

Choose a reason for hiding this comment

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

This needs a proper discussion, we can’t just raise minimum versions.

@oleg-andreyev oleg-andreyev deleted the use-AbstractDriverMiddleware branch February 23, 2024 20:14
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.

None yet

4 participants