-
Notifications
You must be signed in to change notification settings - Fork 1.4k
PHPORM-56 Replace Collection proxy class with Driver monitoring #3137
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
Conversation
6db1052 to
0f2f584
Compare
a187abc to
2b02660
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.
Requires mongodb/mongo-php-library#1195
2b02660 to
5ca02d2
Compare
5ca02d2 to
21d8454
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.
Might apply to other BC breaks as well, but do we want to leverage deprecations in 1.x to announce the break?
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.
We can mark the class as deprecated in 1.x. Unless we add an option to switch the monitoring feature from the proxy class to the monitoring events, there will be no way to disable the deprecation.
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.
Got it - not sure we need to do this, just wanted to check 👍
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 never checked the performance of this, but why not use str_starts_with?
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.
The array key is never empty, and I need to check only 1 char. This skips 1 method call.
public function benchStrStartsWith() { $key = '$db'; return str_starts_with($key, '$'); } public function benchStrictCmp() { $key = '$db'; return $key[0] === '$'; }
benchStrStartsWith......................I2 - Mo0.185μs (±0.26%)
benchStrictCmp..........................I2 - Mo0.135μs (±0.60%)
Co-authored-by: Andreas Braun <git@alcaeus.org>
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.
Unregistering the subscriber seems correct, but I'm more generally curious how the disconnect() method is used in Laravel. Wouldn't it be misleading to users given that PHPC persists the connections by default?
I assume Laravel never uses the disableClientPersistence driver option.
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'm just making sure that the objects are deleted without requiring a garbage collector cycle.
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 understand why you made the additions to this method. It was a more general question about what the purpose of disconnect() was for the Laravel MongoDB library. Is it something we're obligated to implement for Eloquent?
Uh oh!
There was an error while loading. Please reload this page.
Fix PHPORM-56
Use MongoDB driver monitoring feature to log all commands.
Benefit: any 3rd party class that accepts a
MongoDB\Collectioninstance can be used and still having query logs in Laravel.Upgrade guide:
MongoDB\Laravel\CollectiontoMongoDB\Collection.Checklist