Skip to content
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Session ID regeneration #2

Closed
sagikazarmark opened this issue Apr 19, 2017 · 5 comments
Closed

Session ID regeneration #2

sagikazarmark opened this issue Apr 19, 2017 · 5 comments

Comments

@sagikazarmark
Copy link

Based on time or request count, session ID should be regenerated from time to time. This would be a nice addition to this middleware.

@oscarotero
Copy link
Member

I'm not sure about this. It seems a good addition, but I'm afraid to add too much logic in the middleware.

@ghost
Copy link

ghost commented May 11, 2018

@oscarotero i have forked this project and implemented this feature, are you looking to implement this feature and if so i can add in additional tests and documentation for this and PR.

@oscarotero
Copy link
Member

@sephedo Thanks for your work. I'll happily merge your PR, but I'd like to suggest some changes: instead having two methods to configure the id regeneration based in the expire time, I'd create just one method with the following signature:

public function regenerateId(int $lifetime, string $lifetimeKey = 'session-lifetime'): self

This prevent confusing cases like configuring the key used to save the expire time but not the seconds of the lifetime. Now the whole functionality is configured in one place.

I'd also recommend to change the method name to a more descriptive option. lifetime seems to be the lifetime of the session, instead the lifetime of the id, so people can think that when the lifetime is reached, the session will be invalid. I used regenerateId in the example, but other possible names could be idLifetime(). Any other suggestion is welcome.

Anyway, we can discuss these things in the PR.
Thanks

@oscarotero
Copy link
Member

v1.10 released including this change.

@sagikazarmark
Copy link
Author

Wow, this was fast. Awesome!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants