Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

[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

Merged
fabpot merged 1 commit intosymfony:6.4fromGromNaN:ext-mongodb2
Nov 2, 2023

Conversation

GromNaN
Copy link
Member

@GromNaNGromNaN commentedOct 27, 2023
edited
Loading

QA
Branch?6.4
Bug fix?no
New feature?yes
Deprecations?no
Issues-
LicenseMIT

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 ofMongoDB\Client are missing (server selection, session, transaction). But they are not necessary to store Sessions and Lock.

Changes:

  • Lock & Session accept aMongoDB\Driver\Manager
  • The lock uses exclusively UTC date. Before, there was a mix oftime() andUTCDatetime 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 installingmongodb/mongodb if they want, with the same features as before.

@nicolas-grekas
Copy link
Member

Some test failures are related in the integration job apparently.

GromNaN reacted with eyes emoji

@GromNaN
Copy link
MemberAuthor

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.

@@ -142,6 +149,10 @@ public function __construct(Collection|Client|string $mongo, array $options = []
*/
private function skimUri(string $uri): string
Copy link
Contributor

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.

jmikola reacted with thumbs up emoji
Copy link
MemberAuthor

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.

Copy link
Contributor

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).

@GromNaNGromNaNforce-pushed theext-mongodb2 branch 7 times, most recently fromff8e9ed tod30e3a9CompareOctober 31, 2023 14:18
Copy link
Member

@nicolas-grekasnicolas-grekas left a 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 !

GromNaN reacted with heart emoji
@nicolas-grekasnicolas-grekas added the ❄️ Feature FreezeImportant Pull Requests to finish before the next Symfony "feature freeze" labelOct 31, 2023
@GromNaNGromNaNforce-pushed theext-mongodb2 branch 2 times, most recently fromdbefd97 toa2d9ef1CompareOctober 31, 2023 15:05
Copy link
MemberAuthor

@GromNaNGromNaN left a 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.

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

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.

Copy link
MemberAuthor

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.

jmikola reacted with thumbs up emoji
@@ -142,6 +149,10 @@ public function __construct(Collection|Client|string $mongo, array $options = []
*/
private function skimUri(string $uri): string
Copy link
Contributor

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).

);

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

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

Copy link
MemberAuthor

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.

jmikola reacted with thumbs up emoji
Copy link
Contributor

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.

@GromNaNGromNaNforce-pushed theext-mongodb2 branch 2 times, most recently from5e567e8 toa3478caCompareNovember 2, 2023 20:12
@fabpot
Copy link
Member

Thank you@GromNaN.

@fabpotfabpot merged commit8a69f67 intosymfony:6.4Nov 2, 2023
@GromNaNGromNaN deleted the ext-mongodb2 branchNovember 2, 2023 20:46
@fabpotfabpot mentioned this pull requestNov 10, 2023
@GromNaNGromNaN changed the title[HttpFoundation][Lock] Makes MongoDB adapters usable withext-mongodb only[HttpFoundation][Lock] Makes MongoDB adapters usable withext-mongodb directlyNov 10, 2023
@fabpotfabpot mentioned this pull requestNov 10, 2023
@xabbuhxabbuh mentioned this pull requestNov 25, 2023
xabbuh added a commit that referenced this pull requestNov 25, 2023
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       | MITthis was forgotten in#52336Commits-------a4e82c9 fix GitHub workflows
nicolas-grekas added a commit that referenced this pull requestOct 25, 2024
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       | MITSame as#58619 for branch 6.4 that was reworked by#52336Commits-------cd3b778 Ensure compatibility with ext-mongodb v2
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@alcaeusalcaeusalcaeus left review comments

@nicolas-grekasnicolas-grekasnicolas-grekas approved these changes

@jmikolajmikolajmikola approved these changes

@jderussejderusseAwaiting requested review from jderussejderusse is a code owner

@stofstofAwaiting requested review from stof

@OskarStarkOskarStarkAwaiting requested review from OskarStark

Assignees
No one assigned
Labels
FeatureHttpFoundationLock❄️ Feature FreezeImportant Pull Requests to finish before the next Symfony "feature freeze"Status: Reviewed
Projects
None yet
Milestone
6.4
Development

Successfully merging this pull request may close these issues.

8 participants
@GromNaN@nicolas-grekas@fabpot@jmikola@alcaeus@stof@OskarStark@carsonbot

[8]ページ先頭

©2009-2025 Movatter.jp