Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.6k
[HttpFoundation][Lock] Makes MongoDB adapters usable withext-mongodb
directly#52336
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
src/Symfony/Component/HttpFoundation/Session/Storage/Handler/MongoDbSessionHandler.php OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
Some test failures are related in the integration job apparently. |
Thanks. I fixed the tests, the CI uses an old version of the ext-mongodb. But that's fine, we don't set version constraint. |
...Symfony/Component/HttpFoundation/Tests/Session/Storage/Handler/MongoDbSessionHandlerTest.php OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
src/Symfony/Component/HttpFoundation/Session/Storage/Handler/MongoDbSessionHandler.php OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
src/Symfony/Component/HttpFoundation/Session/Storage/Handler/MongoDbSessionHandler.php OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
src/Symfony/Component/HttpFoundation/Session/Storage/Handler/MongoDbSessionHandler.php OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
src/Symfony/Component/HttpFoundation/Session/Storage/Handler/MongoDbSessionHandler.php OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
@@ -142,6 +149,10 @@ public function __construct(Collection|Client|string $mongo, array $options = [] | |||
*/ | |||
private function skimUri(string $uri): string |
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.
Leaving this here: note that as per theConnection Strings documentation, the path in the connection string refers to the default authentication database to be used if theauthSource
does not define one. While most people consider this to be the default database, I wanted to point out the difference. Not necessarily an issue of this PR as it's already the current behaviour, so feel free to resolve this comment after reading if you think this isn't worth changing.
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.
Sure, that would be better if the optiondatabase
had precedence over the database in the path of the URI.
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.
Note thatauthSource should never be assumed to be a database name. For some auth mechanisms it might be set to$external
(see:authSource docs).
Uh oh!
There was an error while loading.Please reload this page.
ff8e9ed
tod30e3a9
CompareThere 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.
LGTM for 6.4, I like that this makes the CI simpler !
dbefd97
toa2d9ef1
Compare...Symfony/Component/HttpFoundation/Tests/Session/Storage/Handler/MongoDbSessionHandlerTest.phpShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
...Symfony/Component/HttpFoundation/Tests/Session/Storage/Handler/MongoDbSessionHandlerTest.php OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
...Symfony/Component/HttpFoundation/Tests/Session/Storage/Handler/MongoDbSessionHandlerTest.php OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
...Symfony/Component/HttpFoundation/Tests/Session/Storage/Handler/MongoDbSessionHandlerTest.php OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
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.
Thanks for the review@stof. I removed clock related code.
...Symfony/Component/HttpFoundation/Tests/Session/Storage/Handler/MongoDbSessionHandlerTest.php OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
...Symfony/Component/HttpFoundation/Tests/Session/Storage/Handler/MongoDbSessionHandlerTest.php OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
...Symfony/Component/HttpFoundation/Tests/Session/Storage/Handler/MongoDbSessionHandlerTest.phpShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
...Symfony/Component/HttpFoundation/Tests/Session/Storage/Handler/MongoDbSessionHandlerTest.php OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
src/Symfony/Component/HttpFoundation/Session/Storage/Handler/MongoDbSessionHandler.phpShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
src/Symfony/Component/HttpFoundation/Session/Storage/Handler/MongoDbSessionHandler.php OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
src/Symfony/Component/HttpFoundation/Session/Storage/Handler/MongoDbSessionHandler.php OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
if (null === $dbData) { | ||
$this->options['expiry_field'] => ['$gte' => $this->getUTCDateTime()], | ||
], [ | ||
'projection' => [ |
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.
Does anything enforce that$this->options['id_field']
is actually_id
or even a field with a unique index?
I assume this will only ever return one or no documents, but I noticed you uselimit: 1
for the delete operation indoDestroy()
, but omit the limit here. I don't disagree with an explicit limit on the delete operation, since the default is to remove all matching documents.
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 guess this options were copied fromPdoSessionHandler
when this provider was created. This provider has aconfigureSchema
orcreateTable
methods to init the table. We can add a similar method for MongoDB. But since the collection already have an unique index on_id
, I think it's fine to let people that would like to change the id field, initialize the collection themselve.
src/Symfony/Component/HttpFoundation/Session/Storage/Handler/MongoDbSessionHandler.php OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
@@ -142,6 +149,10 @@ public function __construct(Collection|Client|string $mongo, array $options = [] | |||
*/ | |||
private function skimUri(string $uri): string |
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.
Note thatauthSource should never be assumed to be a database name. For some auth mechanisms it might be set to$external
(see:authSource docs).
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
); | ||
return $this->collection; | ||
return $this->manager ??= new Manager($this->uri, $this->options['uriOptions'], $this->options['driverOptions']); |
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.
Offhand, is there anything in these Symfony adapters that sets driver information like we do for Doctrine ODM and Laravel? If so, should we consider doing so in a subsequent PR? /cc@alcaeus
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 connection will mostly come from Doctrine, but I can add user-agent for standalone usage.
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 think it makes sense if you're constructing the Manager yourself, but no need to hold up this PR to add that if you want to address it down the line.
5e567e8
toa3478ca
Compare...Symfony/Component/HttpFoundation/Tests/Session/Storage/Handler/MongoDbSessionHandlerTest.php OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
7ba9819
to264d3a2
CompareThank you@GromNaN. |
ext-mongodb
onlyext-mongodb
directlyThis PR was merged into the 6.4 branch.Discussion----------[Lock] Ensure compatibility with ext-mongodb v2| Q | A| ------------- | ---| Branch? | 6.4| Bug fix? | yes| New feature? | no| Deprecations? | no| Issues | -| License | MITSame as#58619 for branch 6.4 that was reworked by#52336Commits-------cd3b778 Ensure compatibility with ext-mongodb v2
Uh oh!
There was an error while loading.Please reload this page.
mongodb/mongodb
is complex to handle for libraries with optional support of MongoDB, as it requiresext-mongodb
. In order to reduce complexity for maintainers, I reimplemented the session and lock adapters to use only the C driver classes.Some features of
MongoDB\Client
are missing (server selection, session, transaction). But they are not necessary to store Sessions and Lock.Changes:
MongoDB\Driver\Manager
time()
andUTCDatetime
objects.mongodb/mongodb
not needed in the CIAnd of course also allows developers to use this adapters without installing
mongodb/mongodb
if they want, with the same features as before.