8000 [HttpFoundation] RedisSessionHandler · Issue #24433 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

[HttpFoundation] RedisSessionHandler #24433

New issue
< 8000 p class="text-center mb-4"> Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

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

Jump to bottom
Closed
dkarlovi opened this issue Oct 5, 2017 · 20 comments
Closed

[HttpFoundation] RedisSessionHandler #24433

dkarlovi opened this issue Oct 5, 2017 · 20 comments
Labels
Feature HttpFoundation

Comments

@dkarlovi
Copy link
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
Copy link
Contributor
Tobion commented Oct 5, 2017

please use the search. #14539

@dkarlovi
Copy link
Contributor Author
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
Copy link
< 8000 /div>
Contributor
Tobion commented Oct 5, 2017

There are not two memcached handlers.

@Tobion
Copy link
Contributor
Tobion commented Oct 5, 2017 •
edited
Loading

There are at least 4 memcache(d) handlers:

And two for Redis:

Isn't that enough?

@dkarlovi
Copy link
Contributor Author
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
Copy link
Contributor Author
dkarlovi commented Oct 5, 2017 •
edited
Loading

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
Copy link
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
Copy link
Contributor Author
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
Copy link
Contributor
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
Copy link
Contributor Author
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
Copy link
Contributor
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
Copy link
Contributor Author
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
Copy link
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?

@dkarlovi
Copy link
Contributor Author
dkarlovi commented Oct 8, 2017 •
edited
Loading

@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
Copy link
Member

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

@dkarlovi
Copy link
Contributor Author

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

@nicolas-grekas
Copy link
Member

New PR (that could borrow code from previous ones)

@Simperfit
Copy link
Contributor

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

@dkarlovi
Copy link
Contributor Author

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

@dkarlovi dkarlovi mentioned this issue Nov 1, 2017
Merged
@dkarlovi
Copy link
Contributor Author
dkarlovi commented Nov 1, 2017

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

nicolas-grekas added a commit that referenced this issue Feb 4, 2018
@nicolas-grekas
feature #24781 [HttpFoundation] RedisSessionHandler (dkarlovi)
78ebc6d 
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
Assignees
No one assigned
Labels
Feature HttpFoundation
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
6 participants
@javiereguiluz @dkarlovi @nicolas-grekas @Tobion @xabbuh @Simperfit
0