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

New doctrine/dbal polyfills have side effects #484

Closed
dFayet opened this issue Apr 19, 2021 · 8 comments
Closed

New doctrine/dbal polyfills have side effects #484

dFayet opened this issue Apr 19, 2021 · 8 comments
Milestone

Comments

@dFayet
Copy link

dFayet commented Apr 19, 2021

Since sentry/sentry-symfony version 4.1.0, new doctrine/dbal polyfills were added:

if (interface_exists(Statement::class) && !interface_exists(Result::class)) {
/** @psalm-suppress UndefinedClass */
class_alias(Statement::class, Result::class);
}
if (interface_exists(DriverExceptionInterface::class) && !interface_exists(LegacyDriverExceptionInterface::class)) {
/** @psalm-suppress UndefinedClass */
class_alias(DriverExceptionInterface::class, LegacyDriverExceptionInterface::class);
}
if (!interface_exists(DoctrineMiddlewareInterface::class)) {
/** @psalm-suppress UndefinedClass */
class_alias(MiddlewareInterface::class, DoctrineMiddlewareInterface::class);
}
if (!interface_exists(DoctrineDriverInterface::class)) {
/** @psalm-suppress UndefinedClass */
class_alias(DriverInterface::class, DoctrineDriverInterface::class);
}
if (!interface_exists(LegacyExceptionConverterDriverInterface::class)) {
/** @psalm-suppress UndefinedClass */
class_alias(ExceptionConverterDriverInterface::class, LegacyExceptionConverterDriverInterface::class);
}

Related PR: #423

This has the effect of misleading code based on the existence, or not, of these doctrine/dbal v2 interfaces.
As basing the detection only on that is maybe not a great idea, creating polyfill for 3rd party library seems to be quite odd too.

@Jean85
Copy link
Contributor

Jean85 commented Apr 19, 2021

I agree that this approach is not great; we did something similar with the Symfony classes, in the first part of the file, but in that case we declared completely new names to avoid clashing.

@ste93cry do you think that we can do the same here?

@ste93cry
Copy link
Contributor

ste93cry commented Aug 1, 2021

@Jean85 this looks (at least partially) solved by #527, right? As far as I see only two aliases remain

@Jean85
Copy link
Contributor

Jean85 commented Aug 2, 2021

It should. Both aliases are against a classname that it's our own, so we shouldn't have this issue anymore.

@dFayet can you test our develop branch to confirm?

@dFayet
Copy link
Author

dFayet commented Aug 2, 2021

@Jean85 it reduces the numbers of errors possible, but it does not solve the issue as the alias class_alias(ExceptionConverterDriverInterface::class, LegacyExceptionConverterDriverInterface::class); is still here.

@ste93cry
Copy link
Contributor

ste93cry commented Aug 2, 2021

it does not solve the issue as the alias

Yep, that was my thinking. But to be honest I don't see how we could get rid of the remaining ones, we have to implement the interface and in this case it's Symfony not making his own business

@dFayet
Copy link
Author

dFayet commented Aug 2, 2021

it's Symfony not making his own business

I don't think this is Symfony's business here, we have problems with Doctrine.

Is the alias needed somewhere else than in Sentry\SentryBundle\Tracing\Doctrine\DBAL\TracingDriver ?
If no, wouldn't it be possible to have 2 implementations like Dbal2TracingDriver and Dbal3TracingDriver (with abstraction, trait, ...)?

@Jean85
Copy link
Contributor

Jean85 commented Aug 4, 2021

@dFayet you're welcome to try it if you want, PR are welcome.
You should target develop though, since #527 changed so much already there.

@github-actions
Copy link

This issue has gone three weeks without activity. In another week, I will close it.

But! If you comment or otherwise update it, I will reset the clock, and if you label it Status: Backlog or Status: In Progress, I will leave it alone ... forever!


"A weed is but an unloved flower." ― Ella Wheeler Wilcox 🥀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants