8000 [HttpFoundation] MongoDbSessionHandler supports auto expiry via configurable expiry_field by catchamonkey · Pull Request #11510 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

[HttpFoundation] MongoDbSessionHandler supports auto expiry via configurable expiry_field #11510

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.

Sign up for GitHub

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

Closed
wants to merge 11 commits into from
Closed

[HttpFoundation] MongoDbSessionHandler supports auto expiry via configurable expiry_field #11510

Changes from 1 commit
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
bb336a5
[HttpFOundation] Added support for auto expiry in MongoSbSessionHandler
catchamonkey Jul 29, 2014
796b18a
Fixes an issue in the mongo handler where the wrong option was used f…
catchamonkey Jul 29, 2014
2d8b2bd
[HttpFoundation] Fixes unit test following the recent update
catchamonkey Jul 29, 2014
4d42f32
Fixes usage of short array syntax to support 5.3
catchamonkey Jul 29, 2014
cb5e562
Use strict comparisons on boolean checks
catchamonkey Jul 29, 2014
d748743
Switched to using yoda expressions
catchamonkey Jul 30, 2014
2f8742e
Return early from the gc method when expiry indexes are used
catchamonkey Aug 1, 2014
fc962db
Modifies the gc tests to use positive values
catchamonkey Aug 1, 2014
b55c63b
Now longer creating the index during write, but have documented it
catchamonkey Aug 4, 2014
88b256c
Corrects error in comment
catchamonkey Aug 4, 2014
9744f61
Removes will and with expectations from method tests that are never c…
catchamonkey Aug 11, 2014
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Fixes an issue in the mongo handler where the wrong option was used f… 
…or expiry checks
  • Loading branch information
@catchamonkey
catchamonkey committed Jul 29, 2014
commit 796b18a50d2c91ab42d3e3a966f1f201a0beb47d
2 changes: 1 addition & 1 deletion src/Symfony/Component/HttpFoundation/Session/Storage/Handler/MongoDbSessionHandler.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -137,7 +137,7 @@ public function write($sessionId, $data)
$fields[$this->options['expiry_field']] = $expiry;

$this->getCollection()->createIndex(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this line will be called on each call to write? I'm not sure, but I thinks you should use ensureIndex, and move this part in gc. What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ensureIndex is deprecated, createIndex actually does the same thing but is the new one to use.
See: http://php.net/manual/en/mongocollection.ensureindex.php

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you

[$this->options['time_field'] => 1],
[$this->options['expiry_field'] => 1],
['expireAfterSeconds' => 0]
);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand the reasoning for putting this here, but issuing a createIndexes command with each session write is not a good idea. We're also not using the background option here, which would be desirable for index creation on a live production system. There can be additional impact if the collection already has data (we'll block until it completes) and/or is replicated to secondary servers (which are not done in the background by default until 2.6).

Small side note: this might also lead to unexpected behavior if someone on a very old MongoDB version tries to use expiry_field and TTL indexes aren't supported (I'm not sure if 2.0.x would ignore the option or raise an error).

I assume there will be a PR against symfony-docs to complement this. We should just point users to the MongoDB documentation so that they can create the TTL index themselves if necessary.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK I see the concern, I was really just trying to make using this option as simple as possible and have the indexes handled automatically for you.
As I understand it, running createIndex (was ensureIndex) on each write is a fairly common approach, and is useful as it avoids having to effectively do a DB migration when you deploy such a change.

There isn't actually anything in the docs about using the MongoDbSessionHandler in general that I can add this to.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

running createIndex (was ensureIndex) on each write is a fairly common approach

Do you have citations for that? I'd be curious to know what other open-source projects are doing this.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you have citations for that? I'd be curious to know what other open-source projects are doing this.

No, to be honest I can't go any further with it that applications I have been involved in.

Some reference, but nothing hard I guess. http://stackoverflow.com/questions/7000777/mongodb-when-to-call-ensureindex

It can be a documented edit and I'll remove from this code, at least the developer is in control I guess?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It can be a documented edit and I'll remove from this code, at least the developer is in control I guess?

I'd suggest leaving it in a comment (as I did for the ZF class) or in the documentation (if this ever gets an entry).

Anecdotally, index creation for ODM is left to console commands, as we consider it part of the schema creation process. The best practice is definitely to leave index creation as part of the deployment process. There are some references in that Stack Overflow thread to doing it when the application starts, but that's likely for Java and the like, where the application and app server are one and the same (vs. PHP, where we'd be doing it during a web request).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 Have updated it.

}
Expand Down
0