Skip to content
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

[RFC] Make session handlers implements SessionHandlerInterface #57

Closed
malukenho opened this issue Nov 14, 2016 · 10 comments
Closed

[RFC] Make session handlers implements SessionHandlerInterface #57

malukenho opened this issue Nov 14, 2016 · 10 comments

Comments

@malukenho
Copy link
Member

malukenho commented Nov 14, 2016

My proposal is to make PSR7-Session implements the php's SessionHandlerInterface. By doing that we can use $_SESSION directly, or whatever other library that manage the session.

Here's an exemple of what would be the set up for each handler:

$handler = new PSR7Session\StorageLessHandler();
session_set_save_handler($handler, true);

You should be able to change the handler easy and with the bare minimum modification on your application.

Yes. I'm saying that we should get rid of the most of OOP user API, and May be write a separated session manage, or just leave the user to user whatever they want.

/cc @Ocramius @lcobucci

@Ocramius
Copy link
Member

@malukenho I suggest two separate repositories:

  • session save handler repository (@necromant2005 already built one, maybe we can import it?)
  • session middleware that populates/saves the $_SESSION global variable using this repository's session storage

@Ocramius
Copy link
Member

Ocramius commented Nov 14, 2016

Note: relying on $_SESSION directly in the code is a bad idea. We'd be building it as backport/compatibility layer, not as a real feature.

@SvenRtbg
Copy link

I'd oppose this proposal as long as you cannot prove that the resulting software will behave exactly identical compared to the classic files save handler, i.e. including locking and not losing any data on concurrent requests.

If it behaves differently, it should not inherit from/extend a mechanism that people have certain expectations on how it works.

@malukenho
Copy link
Member Author

@Ocramius relying on session directly or not is up to the user IMO.

@SvenRtbg look at the interface:

interface SessionHandlerInterface
{
    /* Methods */
    public bool close();
    public bool destroy(string $session_id);
    public bool gc(int $maxlifetime);
    public bool open(string $save_path , string $name);
    public string read(string $session_id);
    public bool write(string $session_id, string $session_data);
}

That's basically what we got now. My main point in here is that it should be the most simple to use we can, and the most important thing, should be most ease to get rid of it when you want.

@Ocramius
Copy link
Member

If it behaves differently, it should not inherit from/extend a mechanism that people have certain expectations on how it works.

This is perfectly OK for apps that only need auth

@Ocramius relying on session directly or not is up to the user IMO.

Relying on a superglobal should be for legacy code, not for new code.

@SvenRtbg
Copy link

@malukenho I known that interface. It's most overlooked requirement is that open() should aquire a lock on the storage for the session id, and that close() must release that lock again. If this is not implemented (at all or correctly), things will go wrong when people assume that it is just another session save handler that can be used just like any other old school PHP session.

Various projects including Symfony and Laravel get this wrong, and it is constantly causing pain. Let's not add to this another bad implementation of SessionHandlerInterface.

See symfony/symfony#4976 or laravel/framework#7549 - there even is a dedicated wrapper-fix for Laravel: https://github.com/rairlie/laravel-locking-session

@allflame
Copy link

@SvenRtbg I'm pretty sure locks without (shared) "storage" are impossible.
There are just much more limitations to the realization of that interface that are "under the hood" (mainly the amount of data you can set in the cookie) hence there will be at most basic compatibility just "for apps that only need auth" and nothing besides that.

@SvenRtbg
Copy link

@allflame Are you supporting this proposal, or is your statement against it? From my view, I don't see a benefit, and your comment underlined it:

  1. Cookies are not able to store large amount of data.
  2. Locking is impossible.
  3. $_SESSION access is a bad idea.
  4. The use case seems to be a very limited "authentication data only" one.

If this is all true: Why implement SessionHandlerInterface if you cannot use the result just like a real PHP native session.

What is the use case that should be supported here? Who would use that interface implementation, and in what kind of situation?

@allflame
Copy link

@SvenRtbg honestly just wanted to share an opinion, as it's not up to me to decide.
I see your point and probably I tend to lean more towards it, but I can understand people who want flexible on/off switch and don't want to write another copy of wrapper code just for themselves.

@Ocramius
Copy link
Member

Ocramius commented Nov 14, 2016

@SvenRtbg the limitations of this package are described in full in docs/limitations.md. If someone wishes to use $_SESSION in a highly concurrent write scenario, they already know what they are going to hit when using this package for session management.

Other than that, writing both the session storage adapter and the legacy layer that provides the $_SESSION super-global are trivial concerns that would still widen the scope of this project to ~90% of the PHP projects out there.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants