Skip to content
This repository was archived by the owner on Jan 31, 2020. It is now read-only.

Conversation

freax
Copy link
Contributor

@freax freax commented Jun 27, 2015

Sometimes there is no need to regenerate session id every request or regenerate session id by some condition.
This PR provides to control ability to regeneration session id.
I will add some testcases if you find it useful.

@gianarb
Copy link
Contributor

gianarb commented Jun 27, 2015

Hi! Thanks for this PR!
can you try to add a few tests about this new feature?

@samsonasik
Copy link
Contributor

@gianarb tests added by @freax

@gianarb
Copy link
Contributor

gianarb commented Jul 2, 2015

👍

@mwillbanks
Copy link
Contributor

I understand the concept here; however, i think it is rather confusing to add this and could lead to a large WTF factor down the road. Think if you toggle that bit in your application and someone needs to fix a bug due to it not regenerating.. they would look at that and wonder what in the world is going on here? I'm calling regenerateId and it is not regenerating.

The session component itself does not force regenerating the id on each request, and therefore you likely have something else that is causing this to happen. The one area that this does happen in is: https://github.com/zendframework/zend-session/blob/master/src/SessionManager.php#L416 where we need to send an updated cookie.

@mwillbanks
Copy link
Contributor

@freax I'm thinking this might be better implemented by handling this more so when setting the setSessionCookieLifetime more so I do believe this would be better implemented by checking for a change as the configuration essentially just sets the parameters. The workflow happens only when calling "rememberMe" or "forgetMe" on the SessionManager. This is the only place that regenerateId is actually forced and a second parameter might be useful.

@Mezzle
Copy link
Contributor

Mezzle commented Aug 11, 2015

I agree that this would cause a massive WTF factor.

@mwillbanks mwillbanks closed this Sep 2, 2015
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants