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

Make TracingDriverForV32 implements VersionAwarePlatformDriver to handle platform version #731

Merged
merged 1 commit into from
Jun 17, 2023
Merged

Make TracingDriverForV32 implements VersionAwarePlatformDriver to handle platform version #731

merged 1 commit into from
Jun 17, 2023

Conversation

jdecool
Copy link
Contributor

@jdecool jdecool commented Jun 16, 2023

Fix #730

As discussed in the issue, I had the createDatabasePlatformForVersion method to handle the platform version.

@ste93cry
Copy link
Collaborator

We need to implement the method without implementing the interface, otherwise we trigger again the deprecation. Please also add some tests for the new method 🙏

@jdecool
Copy link
Contributor Author

jdecool commented Jun 16, 2023

Not sure about the second test testCreateDatabasePlatformForVersionWhenDriverDefinedCreateDatabasePlatformForVersion.

I've used the VersionAwarePlatformDriverInterface for simplicity. Do you prefer I've implement the test in an other way ?

@ste93cry
Copy link
Collaborator

Do you prefer I've implement the test in an other way ?

Since the whole point of this class is to decouple from the VersionAwarePlatformDriver deprecated interface, I don't think we should use it in the tests either. What about mocking the Driver interface using getMockBuilder() and then using addMethods() on it to configure the createDatabasePlatformForVersion() method? Or, we could create a fake interface with that method declared and then simply use createMock() on it, similar to what we did in TracingDriverForV2

@jdecool
Copy link
Contributor Author

jdecool commented Jun 16, 2023

I choose to create a fake interface.

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.

Apart from a small nitpick, it looks good to me. Do you mind testing that it works well in your project and confirm it?

tests/Tracing/Doctrine/DBAL/TracingDriverForV32Test.php Outdated Show resolved Hide resolved
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.

Good work, thank you!

@ste93cry ste93cry merged commit 58ec963 into getsentry:master Jun 17, 2023
@jdecool jdecool deleted the fix-TracingDriverForV32-aware-platform-version branch June 17, 2023 11:39
@jdecool
Copy link
Contributor Author

jdecool commented Jun 17, 2023

Thanks @ste93cry for your help 🙏🏼

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.

4.9.0 version doesn't load the correct DBAL Platform
2 participants