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
Allow setting session namespace and prefix #1060
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your PR - it makes sense for sure, but probably it's not good idea to allow using empty storeNamespace
...
} | ||
|
||
/** | ||
* {@inheritdoc} | ||
*/ | ||
public function clear() | ||
{ | ||
$_SESSION[$this->storeNamespace] = []; | ||
if (!$this->storeNamespace) { | ||
$_SESSION = []; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not sure if it's safe .... need to find out another way
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree. But without namespace, this is probably only an easy way. Another way may be to manually unset all keys that are set by HybridAuth.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@filips123 how about this:
we make storeNamespace
as required param - So, then we are safe.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ApacheEx Ok, I will fix this tomorrow.
Hi there guys! Are you still planning to merge this PR with the master and add it to the released version? Let me know, |
I added support for setting session namespace and prefix.