PHPORM-56 Replace Collection proxy class with Driver monitoring #3137
Conversation
GromNaN
force-pushed
the
PHPORM-56
branch
3 times, most recently
from
6db1052
to
0f2f584
Compare
GromNaN
force-pushed
the
PHPORM-56
branch
2 times, most recently
from
a187abc
to
2b02660
Compare
| @@ -30,7 +30,7 @@ | |||
| "illuminate/database": "^11", | |||
| "illuminate/events": "^11", | |||
| "illuminate/support": "^11", | |||
| "mongodb/mongodb": "^1.15" | |||
| "mongodb/mongodb": "^1.18" | |||
There was a problem hiding this comment.
Requires mongodb/mongo-php-library#1195
| @@ -10,6 +10,7 @@ All notable changes to this project will be documented in this file. | |||
| * Remove `MongoFailedJobProvider`, replaced by Laravel `DatabaseFailedJobProvider` by @GromNaN in [#3122](https://github.com/mongodb/laravel-mongodb/pull/3122) | |||
| * Remove custom `PasswordResetServiceProvider`, use the default `DatabaseTokenRepository` by @GromNaN in [#3124](https://github.com/mongodb/laravel-mongodb/pull/3124) | |||
| * Remove `Blueprint::background()` method by @GromNaN in [#3132](https://github.com/mongodb/laravel-mongodb/pull/3132) | |||
| * Replace `Collection` proxy class with Driver monitoring by @GromNaN in [#3137]((https://github.com/mongodb/laravel-mongodb/pull/3137) | |||
There was a problem hiding this comment.
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.
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.
Got it - not sure we need to do this, just wanted to check 👍
|
|
||
| $command = []; | ||
| foreach (get_object_vars($startedEvent->getCommand()) as $key => $value) { | ||
| if ($key[0] !== '$' && ! in_array($key, ['lsid', 'txnNumber'])) { |
There was a problem hiding this comment.
I never checked the performance of this, but why not use str_starts_with?
There was a problem hiding this comment.
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>
| @@ -198,6 +203,8 @@ public function ping(): void | |||
| /** @inheritdoc */ | |||
| public function disconnect() | |||
| { | |||
| $this->connection?->removeSubscriber($this->commandSubscriber); | |||
| $this->commandSubscriber = null; | |||
There was a problem hiding this comment.
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.
I'm just making sure that the objects are deleted without requiring a garbage collector cycle.
There was a problem hiding this comment.
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?
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