Skip to content

Conversation

@xabbuh
Copy link
Member

@xabbuh xabbuh commented Jun 27, 2016

Q A
Branch? master
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets
License MIT
Doc PR TODO

Currently, we ship a set of specific session storage handlers with the HttpFoundation aimed to support different storage backends. I suggest to add a new handler that is capable to work with an arbitrary PSR-6 cache item pool to be able to use other backends currently not supported by the core.

public function open($savePath, $sessionId)
{
try {
$this->cachePool->getItem($sessionId);
Copy link
Contributor

Choose a reason for hiding this comment

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

not sure if this is necessary. this mean every session reads the cache twice

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, that's debatable. I am not sure either. The thing is that read() cannot indicate that it failed. Another possible solution is to keep the read cache item in memory here and thus be able to avoid the second call to getItem() in read().

Copy link
Contributor

@Tobion Tobion Jun 27, 2016

Choose a reason for hiding this comment

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

I mean it can only fail for invalid session id chars. And since there is no fallback solution like encoding chars, I don't see the point of indicating failure. So it would be the same as non-existing session.

@nicolas-grekas
Copy link
Member

nicolas-grekas commented Jun 27, 2016

The design of a cache pool makes it extremely unreliable for session handling to me. Think of error handling: psr-6 explicitly says that "false" is the only way to report errors.
I think that by design, a cache isn't suited to session handling, even if technically we might think so. In the details, that's not the case.
A cache can rightfully loose items. A session handler can't.

👎

@xabbuh
Copy link
Member Author

xabbuh commented Jun 27, 2016

@nicolas-grekas We already have handlers for Memcache(d) and there are other implementations outside the core too (for example, for Redis). And as long as you make sure that you do not flush the cache when not needed this works as expected.

@nicolas-grekas
Copy link
Member

nicolas-grekas commented Jun 27, 2016

What's the purpose of this adapter? We're perfectly able to provide native handler that fit the session domain. I see the domain mismatch, but I don't see the benefit that overcomes this issue.

@xabbuh xabbuh closed this Jun 30, 2016
@xabbuh xabbuh deleted the psr6-session-handler branch June 30, 2016 16:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants