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

[HttpFoundation] RedisSessionHandler #24433

Closed
dkarlovi opened this Issue Oct 5, 2017 · 20 comments

Comments

Projects
None yet
6 participants
@dkarlovi
Contributor

dkarlovi commented Oct 5, 2017

Q A
Bug report? no
Feature request? yes
BC Break report? no
RFC? yes
Symfony version 4.0

Add a RedisSessionHandler, same as existing ones for Memcache(d). It would allow to use Redis as a session storage without modifying php.ini and with better integration with Symfony (using env vars for configuring the save_path, for example).

@Tobion

This comment has been minimized.

Show comment
Hide comment
@Tobion

Tobion Oct 5, 2017

Member

please use the search. #14539

Member

Tobion commented Oct 5, 2017

please use the search. #14539

@dkarlovi

This comment has been minimized.

Show comment
Hide comment
@dkarlovi

dkarlovi Oct 5, 2017

Contributor

I did, must have missed it because it's closed which makes no sense to me.

Having a user handler means you can define the save path from config, using env vars, as I said. There's zero reason to have TWO Memcached-related handlers and zero Redis handlers.

Contributor

dkarlovi commented Oct 5, 2017

I did, must have missed it because it's closed which makes no sense to me.

Having a user handler means you can define the save path from config, using env vars, as I said. There's zero reason to have TWO Memcached-related handlers and zero Redis handlers.

@Tobion

This comment has been minimized.

Show comment
Hide comment
@Tobion

Tobion Oct 5, 2017

Member

There are not two memcached handlers.

Member

Tobion commented Oct 5, 2017

There are not two memcached handlers.

@dkarlovi

This comment has been minimized.

Show comment
Hide comment
@dkarlovi

dkarlovi Oct 5, 2017

Contributor

There are not two memcached handlers.

I think you know what I mean: HttpFoundation currently provides support for:

  1. MemcacheSessionHandler
  2. MemcachedSessionHandler

Reason given for rejecting previous PRs: there's a native support already. Obviously, this goes for Memcache one too, why did that get special treatment and end up in the framework?

Isn't that enough?

Obviously it's technically enough, this issue is about providing it by the framework, with all the benefits coming from the integration (like the twice-mentioned env config support out of the box). Monolog can output to STDERR, but having a minimal implementation built right into the framework is still desirable.

IMO having multiple PRs and issues on the matter should be a signal people use Redis in this way, are keen to use it in Symfony apps and having support for it shouldn't be a problem, especially since the implementation is so minimal. Please reconsider this (several times rejected) idea.

Contributor

dkarlovi commented Oct 5, 2017

There are not two memcached handlers.

I think you know what I mean: HttpFoundation currently provides support for:

  1. MemcacheSessionHandler
  2. MemcachedSessionHandler

Reason given for rejecting previous PRs: there's a native support already. Obviously, this goes for Memcache one too, why did that get special treatment and end up in the framework?

Isn't that enough?

Obviously it's technically enough, this issue is about providing it by the framework, with all the benefits coming from the integration (like the twice-mentioned env config support out of the box). Monolog can output to STDERR, but having a minimal implementation built right into the framework is still desirable.

IMO having multiple PRs and issues on the matter should be a signal people use Redis in this way, are keen to use it in Symfony apps and having support for it shouldn't be a problem, especially since the implementation is so minimal. Please reconsider this (several times rejected) idea.

@dkarlovi

This comment has been minimized.

Show comment
Hide comment
@dkarlovi

dkarlovi Oct 5, 2017

Contributor

Two side notes I've just remembered:

  1. Redis native session support is tricky and can be buggy: we had issues with it on a (non-Symfony, though) large news portal (many users, many sessions) and it can cause problems which were worked around by a user Redis handler which did the same thing in the same setup.
  2. As Symfony now offers the Cache component, maybe an acceptable idea might be to offer something like CacheSessionHandler? This would amount to the same thing, but might make it more palpable? This could mean deprecating Memcached adapters in 4.1 which would allow for new use cases while removing code.
Contributor

dkarlovi commented Oct 5, 2017

Two side notes I've just remembered:

  1. Redis native session support is tricky and can be buggy: we had issues with it on a (non-Symfony, though) large news portal (many users, many sessions) and it can cause problems which were worked around by a user Redis handler which did the same thing in the same setup.
  2. As Symfony now offers the Cache component, maybe an acceptable idea might be to offer something like CacheSessionHandler? This would amount to the same thing, but might make it more palpable? This could mean deprecating Memcached adapters in 4.1 which would allow for new use cases while removing code.
@xabbuh

This comment has been minimized.

Show comment
Hide comment
@xabbuh

xabbuh Oct 5, 2017

Member

Just for the records: Having a generic PSR-6 session handler was already rejected (see #19193 and #23321).

Member

xabbuh commented Oct 5, 2017

Just for the records: Having a generic PSR-6 session handler was already rejected (see #19193 and #23321).

@dkarlovi

This comment has been minimized.

Show comment
Hide comment
@dkarlovi

dkarlovi Oct 5, 2017

Contributor

@xabbuh thanks for the links, I actually fully agree with @nicolas-grekas arguments there about different volatility constraints for both use cases, haven't completely considered it.

Contributor

dkarlovi commented Oct 5, 2017

@xabbuh thanks for the links, I actually fully agree with @nicolas-grekas arguments there about different volatility constraints for both use cases, haven't completely considered it.

@Tobion

This comment has been minimized.

Show comment
Hide comment
@Tobion

Tobion Oct 8, 2017

Member

with all the benefits coming from the integration (like the twice-mentioned env config support out of the box)

You can just call ini_set('session.save_path', $env) with the env config. Or just use the framework session config for this which does the same thing. I don't see the relation to whether the handler is in symfony or somewhere else.

Member

Tobion commented Oct 8, 2017

with all the benefits coming from the integration (like the twice-mentioned env config support out of the box)

You can just call ini_set('session.save_path', $env) with the env config. Or just use the framework session config for this which does the same thing. I don't see the relation to whether the handler is in symfony or somewhere else.

@dkarlovi

This comment has been minimized.

Show comment
Hide comment
@dkarlovi

dkarlovi Oct 8, 2017

Contributor

I don't see the relation to whether the handler is in symfony or somewhere else.

I'm not claiming it's impossible to do it otherwise because of course it is. The proposal is to bring Redis support inline with Memcache support for this use case, as I've said. If I can

just ini_set

for Redis, I can do that for Memcache as well, what's the reason to support Memcached twice, but Redis not at all? Also, as I've said, from my experience Redis native session support can be buggy.

What's exactly your reason NOT to do it, which doesn't also apply to Memcached?

Contributor

dkarlovi commented Oct 8, 2017

I don't see the relation to whether the handler is in symfony or somewhere else.

I'm not claiming it's impossible to do it otherwise because of course it is. The proposal is to bring Redis support inline with Memcache support for this use case, as I've said. If I can

just ini_set

for Redis, I can do that for Memcache as well, what's the reason to support Memcached twice, but Redis not at all? Also, as I've said, from my experience Redis native session support can be buggy.

What's exactly your reason NOT to do it, which doesn't also apply to Memcached?

@Tobion

This comment has been minimized.

Show comment
Hide comment
@Tobion

Tobion Oct 8, 2017

Member

Maintainance costs? Not reinventing the wheel? There are alot of reasons. Feel free to provide a PR that has definite advantages like more features or better performance than the existing redis session handlers. Then we can talk more concrete.
Btw, we deprecated one of the memcache session handlers. #24443

Member

Tobion commented Oct 8, 2017

Maintainance costs? Not reinventing the wheel? There are alot of reasons. Feel free to provide a PR that has definite advantages like more features or better performance than the existing redis session handlers. Then we can talk more concrete.
Btw, we deprecated one of the memcache session handlers. #24443

@dkarlovi

This comment has been minimized.

Show comment
Hide comment
@dkarlovi

dkarlovi Oct 8, 2017

Contributor

definite advantages like more features or better performance than the existing redis session handlers

Does the existing Memcached handler satisfy those requirements, seeing it's just a wrapper around the native module? Memcached native session handler can basically be used exactly like the Redis one.

Or are you planning on deprecating MemcachedSessionHandler too?

Contributor

dkarlovi commented Oct 8, 2017

definite advantages like more features or better performance than the existing redis session handlers

Does the existing Memcached handler satisfy those requirements, seeing it's just a wrapper around the native module? Memcached native session handler can basically be used exactly like the Redis one.

Or are you planning on deprecating MemcachedSessionHandler too?

@nicolas-grekas

This comment has been minimized.

Show comment
Hide comment
@nicolas-grekas

nicolas-grekas Oct 8, 2017

Member

@dkarlovi would you have time to submit a PR as @Tobion suggested? Maybe the argument of not having to install another package to use it would convince ppl?

Member

nicolas-grekas commented Oct 8, 2017

@dkarlovi would you have time to submit a PR as @Tobion suggested? Maybe the argument of not having to install another package to use it would convince ppl?

@dkarlovi

This comment has been minimized.

Show comment
Hide comment
@dkarlovi

dkarlovi Oct 8, 2017

Contributor

@nicolas-grekas sure, but I don't see what my PR could do differently than those two rejected before.

The way I see, the argument here for not including the Redis adapter is "no, thank you" as I literally cannot understand why would you not do it if you have done the Memcached one:

  1. has native support directly in the module: (Memcached: yes, Redis: yes)
  2. has non-Symfony custom user handler(s) available (Memcached: yes, Redis: yes)
  3. has Symfony custom user handler available (Memcached: yes, Redis: no, and rejected twice?)

That's my whole point, can't see why Redis is second-class citizen for Symfony sessions. :) IMO the "use env variables for save path" (esp. with your new env vars processing infra) is reason enough to want to support Redis directly.

Contributor

dkarlovi commented Oct 8, 2017

@nicolas-grekas sure, but I don't see what my PR could do differently than those two rejected before.

The way I see, the argument here for not including the Redis adapter is "no, thank you" as I literally cannot understand why would you not do it if you have done the Memcached one:

  1. has native support directly in the module: (Memcached: yes, Redis: yes)
  2. has non-Symfony custom user handler(s) available (Memcached: yes, Redis: yes)
  3. has Symfony custom user handler available (Memcached: yes, Redis: no, and rejected twice?)

That's my whole point, can't see why Redis is second-class citizen for Symfony sessions. :) IMO the "use env variables for save path" (esp. with your new env vars processing infra) is reason enough to want to support Redis directly.

@nicolas-grekas

This comment has been minimized.

Show comment
Hide comment
@nicolas-grekas

nicolas-grekas Oct 12, 2017

Member

When #24523 will be merged, I think it'll make sense to add a Redis session handler.

Member

nicolas-grekas commented Oct 12, 2017

When #24523 will be merged, I think it'll make sense to add a Redis session handler.

@dkarlovi

This comment has been minimized.

Show comment
Hide comment
@dkarlovi

dkarlovi Oct 12, 2017

Contributor

@nicolas-grekas would we go for a new PR or resume previous ones?

Contributor

dkarlovi commented Oct 12, 2017

@nicolas-grekas would we go for a new PR or resume previous ones?

@nicolas-grekas

This comment has been minimized.

Show comment
Hide comment
@nicolas-grekas

nicolas-grekas Oct 12, 2017

Member

New PR (that could borrow code from previous ones)

Member

nicolas-grekas commented Oct 12, 2017

New PR (that could borrow code from previous ones)

@Simperfit

This comment has been minimized.

Show comment
Hide comment
@Simperfit

Simperfit Oct 30, 2017

Contributor

@dkarlovi since #24523 has been merged, do you want to implement that feature ?

Contributor

Simperfit commented Oct 30, 2017

@dkarlovi since #24523 has been merged, do you want to implement that feature ?

@dkarlovi

This comment has been minimized.

Show comment
Hide comment
@dkarlovi

dkarlovi Oct 30, 2017

Contributor

@Simperfit seeing you're doing #23910, I'll do this, yes.

Contributor

dkarlovi commented Oct 30, 2017

@Simperfit seeing you're doing #23910, I'll do this, yes.

@dkarlovi

This comment has been minimized.

Show comment
Hide comment
@dkarlovi

dkarlovi Nov 1, 2017

Contributor

There we go, hope it's OK. :)

Contributor

dkarlovi commented Nov 1, 2017

There we go, hope it's OK. :)

nicolas-grekas added a commit that referenced this issue Feb 4, 2018

feature #24781 [HttpFoundation] RedisSessionHandler (dkarlovi)
This PR was merged into the 4.1-dev branch.

Discussion
----------

[HttpFoundation] RedisSessionHandler

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | no
| New feature?  | yes
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #24433, #18233, #14539, #4538, #3498
| License       | MIT
| Doc PR        | symfony/symfony-docs#8572

Ability to use Redis as a session storage backend. Discussed in detail in linked issues / PRs.

Commits
-------

8776cce [HttpFoundation] Add RedisSessionHandler
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment