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

Introduce TracingDriverForV32 for DBAL >= 3.2 #723

Merged
merged 3 commits into from Jun 12, 2023
Merged

Introduce TracingDriverForV32 for DBAL >= 3.2 #723

merged 3 commits into from Jun 12, 2023

Conversation

ruudk
Copy link
Contributor

@ruudk ruudk commented Jun 6, 2023

This driver does not implement the deprecated VersionAwarePlatformDriver.

It's automatically picked when doctrine/dbal version 3.2.0 or higher is installed.

Fixes #579

@ruudk ruudk force-pushed the doctrine-dbal-3-2 branch 5 times, most recently from 73a851b to df90a7a Compare June 6, 2023 13:36
@ruudk
Copy link
Contributor Author

ruudk commented Jun 6, 2023

@cleptric Any idea why the tests are not running anymore? It worked the first push, but then stopped. I feel there is an error on the workflow (that I modified) but I cannot see it.

@ruudk ruudk force-pushed the doctrine-dbal-3-2 branch 2 times, most recently from ea8ed04 to 2223335 Compare June 6, 2023 13:47
@ruudk ruudk marked this pull request as ready for review June 6, 2023 13:47
@stayallive
Copy link
Collaborator

Seems to have ran just fine now 😄 Maybe GH was a little slow starting them.

@ruudk ruudk force-pushed the doctrine-dbal-3-2 branch 5 times, most recently from c7a33bc to cb92a10 Compare June 6, 2023 17:14
@ruudk ruudk mentioned this pull request Jun 6, 2023
Open
@ruudk
Copy link
Contributor Author

ruudk commented Jun 6, 2023

It seems that PHP-CS-Fixer is broken, as it produces many changes unrelated to my PR.

@ruudk ruudk force-pushed the doctrine-dbal-3-2 branch 3 times, most recently from 71aed08 to e69e348 Compare June 6, 2023 17:29
@cleptric
Copy link
Member

cleptric commented Jun 6, 2023

It looks like the latest version of PHPCS contains some incompatible changes with prior versions. You might pin the version to something below 3.17 to get the tests passing.

@ruudk ruudk force-pushed the doctrine-dbal-3-2 branch 2 times, most recently from 3027a7d to df8b4b8 Compare June 6, 2023 17:46
@ruudk
Copy link
Contributor Author

ruudk commented Jun 6, 2023

@cleptric Done in df8b4b8, we're green now.

@ruudk
Copy link
Contributor Author

ruudk commented Jun 7, 2023

@ste93cry @Jeroeny If you have time, I would really appreciate a review on this. Is this the direction to go? Please let me know, happy to make any changes.

ste93cry
ste93cry requested changes Jun 7, 2023
View reviewed changes
composer.json Outdated Show resolved Hide resolved
tests/Tracing/Doctrine/DBAL/TracingDriverForV3Test.php Outdated Show resolved Hide resolved
src/aliases.php Outdated Show resolved Hide resolved
src/aliases.php Outdated Show resolved Hide resolved
tests/DoctrineTestCase.php Outdated Show resolved Hide resolved
.github/workflows/tests.yaml Outdated Show resolved Hide resolved
@ruudk ruudk force-pushed the doctrine-dbal-3-2 branch 2 times, most recently from 6bb6ae2 to 5498ff4 Compare June 7, 2023 09:37
ruudk added 2 commits June 7, 2023 11:40
@ruudk
Pin PHP-CS-Fixer to 3.16.0
b7da815 
There is a problem with PHP-CS-Fixer 3.17.0

@cleptric suggested pinning it.
@ruudk
Add void type to MainCommand::configure
cc9c172 
Fixes
```
  1x: Method "Symfony\Component\Console\Command\Command::configure()" might add "void" as a native return type declaration in the future. Do the same in child class "Sentry\SentryBundle\Tests\End2End\App\Command\MainCommand" now to avoid errors or add an explicit @return annotation to suppress this message.
```
@ruudk ruudk force-pushed the doctrine-dbal-3-2 branch 3 times, most recently from 59bf5e6 to 28509fb Compare June 7, 2023 09:49
@ruudk ruudk requested review from cleptric and ste93cry June 7, 2023 09:50
@ruudk
Copy link
Contributor Author

ruudk commented Jun 7, 2023

Applied feedback, we're green again.

ste93cry
ste93cry requested changes Jun 7, 2023
View reviewed changes
tests/DoctrineTestCase.php Outdated Show resolved Hide resolved
@ruudk ruudk requested a review from ste93cry June 8, 2023 06:43
ste93cry
ste93cry approved these changes Jun 8, 2023
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.

It looks good to me. I just wonder if the TracingDriverForV3Point2 class should be renamed to TracingDriverForV32 as that is the naming convention used in the DBAL itself (and that we used too to name some test methods), but I will leave the final decision up to you.

@ruudk
Introduce TracingDriverForV32
9adbff4 
This driver does not implement the deprecated `VersionAwarePlatformDriver`.

It's automatically picked when `doctrine/dbal` version `3.2.0` or higher is installed.

Fixes getsentry#579
@ruudk ruudk changed the title Introduce TracingDriverForV3Point2 Introduce TracingDriverForV32 for DBAL >= 3.2 Jun 8, 2023
@ruudk
Copy link
Contributor Author

ruudk commented Jun 8, 2023

I didn't like the Point either... but 32 also feels weird. Anyway, since that's the convention, let's follow that.

@ste93cry
Copy link
Collaborator

ste93cry commented Jun 8, 2023

Yea, totally agree on everything you said 👍

@ruudk
Copy link
Contributor Author

ruudk commented Jun 9, 2023

Can this be merged? 🙏

@cleptric cleptric merged commit 7d73be0 into getsentry:master Jun 12, 2023
29 checks passed
@ruudk ruudk deleted the doctrine-dbal-3-2 branch June 12, 2023 06:09
@ruudk
Copy link
Contributor Author

ruudk commented Jun 12, 2023

Thanks, and tagged please 🙏

@ruudk ruudk mentioned this pull request Jun 13, 2023
Closed
@Jean85 Jean85 mentioned this pull request Jun 14, 2023
Closed
@jdecool jdecool mentioned this pull request Jun 16, 2023
Closed
cleptric added a commit that referenced this pull request Jun 19, 2023
@cleptric
Revert "Introduce TracingDriverForV32 for DBAL >= 3.2 (#723)"
f242340 
This reverts commit 7d73be0.
@cleptric cleptric mentioned this pull request Jun 19, 2023
Merged
@Kushnerevich
Copy link

Any updates? Are looking forward to

@Jean85
Copy link
Collaborator

Jean85 commented Jan 9, 2024

This is released since 4.9.0

@ste93cry
Copy link
Collaborator

ste93cry commented Jan 9, 2024

Nope, it was reverted in 4.9.2. I don't remember exactly why, but I remember there was something in DBAL 3 that was missing in previous versions to have a fully working solution that was also backwards-compatible

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ste93cry ste93cry ste93cry approved these changes

@cleptric cleptric Awaiting requested review from cleptric

Assignees
No one assigned
Labels
Type: Improvement
Projects
GitHub Issues with 👀
Archived in project
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Deprecation warning with doctrine/dbal 3.2.0
6 participants
@ruudk @stayallive @cleptric @ste93cry @Kushnerevich @Jean85