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

Change the way the Doctrine tracing middleware is registered in DBAL >=3.x #808

Merged
merged 2 commits into from
Feb 11, 2024

Conversation

ste93cry
Copy link
Collaborator

@ste93cry ste93cry commented Feb 8, 2024

Fixes #805. The problem that was reported is that the MiddlewarePass compiler pass in DoctrineBundle adds a new call to setMiddlewares(), which means that all the middlewares added before get lost. Technically speaking, if you load SentryBundle after DoctrineBundle, it works as expected, because our code appends our middleware to the existing list instead of replacing it entirely. However, as noted by @fridtjof, a better integration approach is to tag our middleware as doctrine.middleware and let the Doctrine bundle do all the rest of the work to properly configure the connection.

@fridtjof
Copy link

fridtjof commented Feb 8, 2024

Do note that this raises the required doctrine-bundle version to 2.6 (as opposed to the current ^2.5 listed in require-dev), as their tagged middleware support was only introduced then: https://github.com/doctrine/DoctrineBundle/releases/tag/2.6.0, doctrine/DoctrineBundle#1472

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 is a very interesting approach, and also forward compatible, thank you!

PS: do we have any E2E test to check if all of this is working?

@ste93cry
Copy link
Collaborator Author

ste93cry commented Feb 9, 2024

Do note that this raises the required doctrine-bundle version to 2.6

Nice catch. Unfortunately, I realized that these changes are not compatible with version 1.12 of the bundle, which we still support. It seems that we declare the compatibility with that version mainly because of Symfony 3.4, but since we nowadays require at least version 4.4 of the framework, I think we can just drop the support for the the older 1.x...

do we have any E2E test to check if all of this is working?

No, we don't. There are some E2E tests for other stuffs, but nothing specifically tailored at asserting that the tracing integrations work as expected. It's something we could definitely improve in the future though...

@ste93cry ste93cry merged commit fe4480a into master Feb 11, 2024
33 checks passed
@ste93cry ste93cry deleted the fix-doctrine-dbal-tracing-middleware-injection branch February 11, 2024 17:20
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.

DbalTracingPass is overridden by DoctrineBundle's own MiddlewaresPass
3 participants