[HttpFoundation][Lock] Makes MongoDB adapters usable with `ext-mongodb` directly #52336

GromNaN · 2023-10-27T22:13:20Z

Q	A
Branch?	6.4
Bug fix?	no
New feature?	yes
Deprecations?	no
Issues	-
License	MIT

mongodb/mongodb is complex to handle for libraries with optional support of MongoDB, as it requires ext-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:

Lock & Session accept a MongoDB\Driver\Manager
The lock uses exclusively UTC date. Before, there was a mix of time() and UTCDatetime objects.
Session tests require a mongo server.
mongodb/mongodb not needed in the CI

And of course also allows developers to use this adapters without installing mongodb/mongodb if they want, with the same features as before.

src/Symfony/Component/HttpFoundation/Session/Storage/Handler/MongoDbSessionHandler.php

nicolas-grekas · 2023-10-29T12:39:24Z

Some test failures are related in the integration job apparently.

GromNaN · 2023-10-29T20:51:07Z

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

src/Symfony/Component/HttpFoundation/composer.json

src/Symfony/Component/HttpFoundation/Session/Storage/Handler/MongoDbSessionHandler.php

src/Symfony/Component/Lock/Store/MongoDbStore.php

src/Symfony/Component/HttpFoundation/Session/Storage/Handler/MongoDbSessionHandler.php

alcaeus · 2023-10-30T09:00:10Z

src/Symfony/Component/Lock/Store/MongoDbStore.php

     *
     * @see https://www.php.net/manual/en/mongodb.connection-handling.php
     */
    private function skimUri(string $uri): string


Leaving this here: note that as per the Connection Strings documentation, the path in the connection string refers to the default authentication database to be used if the authSource 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.

Sure, that would be better if the option database had precedence over the database in the path of the URI.

Note that authSource should never be assumed to be a database name. For some auth mechanisms it might be set to $external (see: authSource docs).

src/Symfony/Component/Lock/Store/MongoDbStore.php

nicolas-grekas

LGTM for 6.4, I like that this makes the CI simpler !

...Symfony/Component/HttpFoundation/Tests/Session/Storage/Handler/MongoDbSessionHandlerTest.php

GromNaN

Thanks for the review @stof. I removed clock related code.

...Symfony/Component/HttpFoundation/Tests/Session/Storage/Handler/MongoDbSessionHandlerTest.php

src/Symfony/Component/HttpFoundation/Session/Storage/Handler/MongoDbSessionHandler.php

jmikola · 2023-11-01T13:59:25Z

src/Symfony/Component/HttpFoundation/Session/Storage/Handler/MongoDbSessionHandler.php

-        if (null === $dbData) {
+            $this->options['expiry_field'] => ['$gte' => $this->getUTCDateTime()],
+        ], [
+            'projection' => [


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 use limit: 1 for the delete operation in doDestroy(), 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.

I guess this options were copied from PdoSessionHandler when this provider was created. This provider has a configureSchema or createTable 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

jmikola · 2023-11-01T14:21:33Z

src/Symfony/Component/Lock/Store/MongoDbStore.php

     *
     * @see https://www.php.net/manual/en/mongodb.connection-handling.php
     */
    private function skimUri(string $uri): string


Note that authSource should never be assumed to be a database name. For some auth mechanisms it might be set to $external (see: authSource docs).

src/Symfony/Component/Lock/Store/MongoDbStore.php

jmikola · 2023-11-01T14:28:24Z

src/Symfony/Component/Lock/Store/MongoDbStore.php

-        );
-
-        return $this->collection;
+        return $this->manager ??= new Manager($this->uri, $this->options['uriOptions'], $this->options['driverOptions']);


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

The connection will mostly come from Doctrine, but I can add user-agent for standalone usage.

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.

...Symfony/Component/HttpFoundation/Tests/Session/Storage/Handler/MongoDbSessionHandlerTest.php

…b` only

fabpot · 2023-11-02T20:45:01Z

Thank you @GromNaN.

This PR was merged into the 6.4 branch. Discussion ---------- fix GitHub workflows | Q | A | ------------- | --- | Branch? | 6.4 | Bug fix? | no | New feature? | no | Deprecations? | no | Issues | | License | MIT this was forgotten in #52336 Commits ------- a4e82c9 fix GitHub workflows

This 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 | MIT Same as #58619 for branch 6.4 that was reworked by #52336 Commits ------- cd3b778 Ensure compatibility with ext-mongodb v2

GromNaN requested a review from jderusse as a code owner October 27, 2023 22:13

carsonbot added Status: Needs Review Feature HttpFoundation Lock labels Oct 27, 2023

carsonbot added this to the 6.4 milestone Oct 27, 2023

OskarStark reviewed Oct 28, 2023

View reviewed changes

src/Symfony/Component/HttpFoundation/Session/Storage/Handler/MongoDbSessionHandler.php Outdated Show resolved Hide resolved

GromNaN force-pushed the ext-mongodb2 branch from 9f0558f to 0334eb4 Compare October 29, 2023 20:57

GromNaN commented Oct 29, 2023

View reviewed changes

GromNaN force-pushed the ext-mongodb2 branch from 0334eb4 to 71f5d08 Compare October 30, 2023 08:49

GromNaN commented Oct 30, 2023

View reviewed changes

src/Symfony/Component/Lock/Store/MongoDbStore.php Outdated Show resolved Hide resolved

alcaeus reviewed Oct 30, 2023

View reviewed changes

GromNaN force-pushed the ext-mongodb2 branch 7 times, most recently from ff8e9ed to d30e3a9 Compare October 31, 2023 14:18

nicolas-grekas approved these changes Oct 31, 2023

View reviewed changes

carsonbot added Status: Reviewed and removed Status: Needs Review labels Oct 31, 2023

nicolas-grekas added the ❄️ Feature Freeze Important Pull Requests to finish before the next Symfony "feature freeze" label Oct 31, 2023

GromNaN force-pushed the ext-mongodb2 branch 2 times, most recently from dbefd97 to a2d9ef1 Compare October 31, 2023 15:05

stof reviewed Oct 31, 2023

View reviewed changes

GromNaN force-pushed the ext-mongodb2 branch from a2d9ef1 to 60df057 Compare October 31, 2023 16:04

GromNaN commented Oct 31, 2023

View reviewed changes

GromNaN requested a review from stof October 31, 2023 16:06

carsonbot removed the Status: Reviewed label Oct 31, 2023

GromNaN requested a review from OskarStark October 31, 2023 16:06

carsonbot added the Status: Needs Review label Oct 31, 2023

jmikola reviewed Nov 1, 2023

View reviewed changes

GromNaN force-pushed the ext-mongodb2 branch 2 times, most recently from 5e567e8 to a3478ca Compare November 2, 2023 20:12

jmikola approved these changes Nov 2, 2023

View reviewed changes

...Symfony/Component/HttpFoundation/Tests/Session/Storage/Handler/MongoDbSessionHandlerTest.php Outdated Show resolved Hide resolved

carsonbot added Status: Reviewed and removed Status: Needs Review labels Nov 2, 2023

GromNaN force-pushed the ext-mongodb2 branch 2 times, most recently from 7ba9819 to 264d3a2 Compare November 2, 2023 20:17

[HttpFoundation][Lock] Makes MongoDB adapters usable with `ext-mongod…

bc24cb3

…b` only

fabpot force-pushed the ext-mongodb2 branch from 264d3a2 to bc24cb3 Compare November 2, 2023 20:44

fabpot merged commit 8a69f67 into symfony:6.4 Nov 2, 2023

GromNaN deleted the ext-mongodb2 branch November 2, 2023 20:46

fabpot mentioned this pull request Nov 10, 2023

Release v6.4.0-BETA3 #52538

Merged

GromNaN changed the title ~~[HttpFoundation][Lock] Makes MongoDB adapters usable with ext-mongodb only~~ [HttpFoundation][Lock] Makes MongoDB adapters usable with ext-mongodb directly Nov 10, 2023

fabpot mentioned this pull request Nov 10, 2023

Release v7.0.0-BETA3 #52541

Merged

xabbuh mentioned this pull request Nov 25, 2023

fix GitHub workflows #52729

Merged

GromNaN mentioned this pull request Oct 25, 2024

[Lock] Ensure compatibility with ext-mongodb v2 #58668

Merged

Uh oh!

[HttpFoundation][Lock] Makes MongoDB adapters usable with ext-mongodb directly #52336

[HttpFoundation][Lock] Makes MongoDB adapters usable with ext-mongodb directly #52336

Uh oh!

Conversation

GromNaN commented Oct 27, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

nicolas-grekas commented Oct 29, 2023

Uh oh!

GromNaN commented Oct 29, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

nicolas-grekas left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

GromNaN left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

fabpot commented Nov 2, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

[HttpFoundation][Lock] Makes MongoDB adapters usable with `ext-mongodb` directly #52336

[HttpFoundation][Lock] Makes MongoDB adapters usable with `ext-mongodb` directly #52336

GromNaN commented Oct 27, 2023 •

edited

Loading