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

Improve the compatibility layer for Doctrine DBAL to avoid side-effects of polyfilling #553

Merged
merged 3 commits into from
Sep 17, 2021

Conversation

ste93cry
Copy link
Collaborator

This is an attempt at fixing #483 and #484. I decided to split the TracingDriver class into two versions, each compatible with just one of the major versions of Doctrine DBAL we support (2.x and 3.x)

@ste93cry ste93cry added this to the 4.2 milestone Sep 11, 2021
@ste93cry ste93cry force-pushed the fix/fix-doctrine-deprecation-notices branch 4 times, most recently from 7745826 to b9c21c9 Compare September 12, 2021 09:40
@ste93cry
Copy link
Collaborator Author

After thinking again about this, and while we have some clear advantages of splitting the implementation into two, e.g. the fact that we can add typehints to more things and evolve the two classes independently without worrying about keeping the compatibility with both major versions at the same time, I'm pretty sure this does not solve #483 and there is no way to solve it. Basically, the DebugClassLoader of Symfony blindly parse the annotations on the classes that it loads and as soon as it finds one that is marked as deprecated and that is loaded from something outside of its vendor it reports the deprecation. In our case, we need to implement those deprecated interfaces to expose the fact that the decorated driver implements them, since it's quite common to use an instanceof check in this context. I still think that my comment about Symfony not making his own business applies perfectly in this case, the thing that the DebugClassLoader is doing would be better suited for a static analysis tool which has far more understanding of the context around how the code is called and how it works

@ste93cry ste93cry changed the title Improve the compatibility layer for Doctrine DBAL to avoid deprecations Improve the compatibility layer for Doctrine DBAL to avoid side-effects of polyfilling Sep 14, 2021
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.

LGTM 👍 splitting is surely a better approach.

Maybe we should declare those implementations @internal?

@ste93cry ste93cry force-pushed the fix/fix-doctrine-deprecation-notices branch from d6c63b2 to 91b3320 Compare September 16, 2021 19:31
@ste93cry
Copy link
Collaborator Author

Maybe we should declare those implementations @internal?

Yes, that's a good idea!

@Jean85 Jean85 merged commit be6339f into master Sep 17, 2021
@Jean85 Jean85 deleted the fix/fix-doctrine-deprecation-notices branch September 17, 2021 16:15
@starred-gijs
Copy link
Contributor

I got an E_WARNING:

Stack trace
E_WARNING: Cannot declare class Sentry\SentryBundle\Tracing\Doctrine\DBAL\TracingStatement, because the name is already in use
in class_alias called at /app/vendor/sentry/sentry-symfony/src/aliases.php (93)
in require called at /app/vendor/composer/autoload_real.php (71)
in composerRequire4d23fd274a12f9ca19dfa7bc29152b16 called at /app/vendor/composer/autoload_real.php (61)
in ComposerAutoloaderInit4d23fd274a12f9ca19dfa7bc29152b16::getLoader called at /app/vendor/autoload.php (12)
in require called at /app/public/index.php (7)

@starred-gijs
Copy link
Contributor

This PR fixed it already I guess #552

@Jean85
Copy link
Collaborator

Jean85 commented Sep 29, 2021

#552 is released as 4.2.3, you can test it out.

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

3 participants