-
Notifications
You must be signed in to change notification settings - Fork 230
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
Expose transaction setting #814
Conversation
fbc7086
to
d8b4ce1
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Configuration reference from docs should be updated in same PR :)
e5ac0e6
to
88ab999
Compare
88ab999
to
67463b6
Compare
Updated to target 5.0.x, which we may bump to 5.1.x. I've also added logic to support older ODM versions that don't support transactional flushes. We can decide whether to bump the ODM requirement at a later date. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
which we may bump to 5.1.x
Please do, it's new config switch after all.
Will do once we have a 5.1.x branch - release automation doesn't handle pre-releases very gracefully, so once 5.0.0 is tagged everything should be in order. |
Ah 5.0.0 is still in RC phase 🙈 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A suggested improvement, but SGTM as this.
|
||
public function testTransactionalFlushConfigurationWhenNotSupported(): void | ||
{ | ||
if (method_exists(ODMConfiguration::class, 'setUseTransactionalFlush')) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a weak test. If the method is renamed, the test is skipped while it should fail.
I think it would be more robust to use Composer\InstalledVersions
:
Composer\InstalledVersions::satisfies(new VersionParser, 'doctrine/mongodb-odm', '>=2.7@dev')
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated. I wasn't even aware this existed. This is simply lovely :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see 3 skipped tests, is the version contraint correct?
This complements doctrine/mongodb-odm#2586 by exposing the
use_transactional_flush
setting to the bundle configuration.