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.

Sign up for GitHub

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

Jump to bottom

Rework DBAL compat layer #512

Closed
wants to merge 27 commits into from
Closed

Rework DBAL compat layer #512

wants to merge 27 commits into from

Conversation

Jean85
Copy link
Collaborator

@Jean85 Jean85 commented May 27, 2021 •
edited
Loading

WIP, this includes #517 to check thoroughly for feasibility.

@ste93cry WDYT of this different approach?

@Jean85 Jean85 mentioned this pull request May 27, 2021
Open
ste93cry
ste93cry requested changes May 27, 2021
View reviewed changes
Copy link
Collaborator

@ste93cry ste93cry left a comment

Choose a reason for hiding this comment

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

I think that by making 2.13 the minimum required version (they recently added back support for PHP 7.2) we could probably cleanup some other code because they added a forward compatibility layer

tests/Tracing/Doctrine/DBAL/TracingDriverTest.php Outdated
@@ -264,3 +254,48 @@ interface StubExceptionConverterDriverInterface extends DriverInterface
{
}
}

class MockDriver implements DriverInterface
Copy link
Collaborator

Choose a reason for hiding this comment

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

It's not clear to me why you added this class instead of mocking the interface like I was doing before. What's the benefit of doing this or the blocker that makes it impossible now to do it?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Psalm was complaining. But it didn't fix the issue anyway, so we could rollback this.

Would bumping DBAL requirement create issues?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Would bumping DBAL requirement create issues?

I don't think so, but the best thing is to try. We couldn't bump the minimum version before simply because there was no support for PHP 7.2, which is now back

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I've investigated it further. The only additional FC is doctrine/dbal#4529, which is about ResultStatement, that we do not touch, so I don't see any advantages.

I'll try to push this PR forward, and on the code side it seems to work, but the static analysis is a complete mess.

Copy link

Choose a reason for hiding this comment

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

there was no support for PHP 7.2, which is now back

But isn't 7.2 dead?

Copy link
Collaborator

Choose a reason for hiding this comment

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

It's EOL, but we still support it mainly because Sentry must run also on legacy platforms, if possible

@Jean85 Jean85 mentioned this pull request Jun 1, 2021
Merged