Does this suppose to help setting the a custom session id using setId($some_id) function of Symfony\Component\HttpFoundation\Session\Session ??

I think all decorators should implement this, isn't it?

Woops missed your comment.

I think all decorators should implement this, isn't it?

I guess they could 🤔 else you’re required to put your SessionIdInterface on top of the stack. I guess this isn’t an issue since you approved this PR?

Eeer after reading https://symfony.com/doc/current/session.html#session-proxies I wonder if this PR makes sense as you’re supposed to extend SessionHandlerProxy rather than decorate the inner handler.

Okay, just tried to do that and I got a ServiceCircularReferenceException… Does the issue come from Symfony or the documentation?

I'm a bit lost between the handler, the proxy, etc. I'll let you investigate if you don't mind :)

I’m still lost but I have some leads: session proxies purpose was originally to ease the migration from PHP 5.3 where SessionHandlerInterface did not exist. They were deprecated then removed, but AbstractProxy and SessionHandlerProxy escaped death thanks to #24523 because the latter would turns any handler into an instance of AbstractSessionHandler. As it doesn’t seem to be the case maybe I’m misunderstanding something 🤔

The SessionHandlerProxy always has been documented as an extension point (like in https://symfony.com/doc/current/session.html#session-proxies). I don’t know if it used to work at some point, but now doing as documented results in a ServiceCircularReferenceException because the framework.session.handler_id service is aliased by session.handler, which is aliased by SessionHandlerInterface (you can fix this issue by wiring your wrapped handler yourself though). Also by doing this you couldn’t use decorators like the MarshallingSessionHandler.

That’s why I thought you were supposed to decorate session.handler instead, which requires less configuration but more boilerplate code to proxy all of SessionHandlerInterface, SessionUpdateTimestampHandlerInterface (and SessionIdInterface?) methods.

Closing, since there already is a way to configure a SessionIdInterface handler.

I have been struggling for most of the day trying to figure out why the create_sid() method was never being called from my custom session handler. I finally found it was because even though I am specifying a class that implements \SessionHandlerInterface, \SessionIdInterface, \SessionUpdateTimestampHandlerInterface as the framework.session.handler_id value, it is being replaced/proxied by the \Symfony\Component\HttpFoundation\Session\Storage\Proxy\SessionHandlerProxy class. Which, as mentioned in this PR does not implement the \SessionIdInterface.

I think it would be beneficial to reconsider implementing this.

@MatTheCat Is right that you can decorate the SessionHandlerProxy to implement \SessionIdInterface. But I think it is more straightforward to have the \SessionIdInterface implemented in SessionHandlerProxy as initially submitted in this PR. Then we have an implementation of SessionHandlerProxy that handles all possible PHP session interfaces, which could help prevent the assumptions that having a handler with \SessionIdInterface being wrong.

If it is decided that it's best not to implement this, we may want to consider deprecating passing a service that does not extend SessionHandlerProxy as a value to framework.session.handler_id , or at least update the documentation to discourage using a handler that isn't an instance of that class.

The SessionHandlerProxy always has been documented as an extension point (like in https://symfony.com/doc/current/session.html#session-proxies). I don’t know if it used to work at some point, but now doing as documented results in a ServiceCircularReferenceException because the framework.session.handler_id service is aliased by session.handler, which is aliased by SessionHandlerInterface (you can fix this issue by wiring your wrapped handler yourself though). Also by doing this you couldn’t use decorators like the MarshallingSessionHandler.

This is another big reason that this PR should be reconsidered. It's awkward to work around the current implementation due to the circular dependency @MatTheCat mentioned above. You have to either have two classes, one for the majority of the custom handler implementation that implements \SessionHandlerInterface and \SessionUpdateTimestampHandlerInterface. And another class that extends SessionHandlerProxy and implements \SessionIdInterface. Then you have to explicitly wire the main handler to the proxied handler doing something like this:

framework:
  session:
    enabled: true
    storage_factory_id: session.storage.factory.native
    handler_id: App\SessionHandlerProxy

services:
  App\SessionHandlerProxy:
    arguments:
      $handler: App\SessionHandler

You could implement all the handler functionality to the App\SessionHandlerProxy, but it still needs a \SessionHandlerInterface in it's constructor. You could pass something like the native handler, but then if you forget to override on of the session handler methods, it would defer to that handler, which just adds confusion and may cause unexpected behavior.

Hey @jdevinemt 👋

Glad to see you concerned, but closed PRs don't usually generate traction; opening an issue may be better.

Thanks @MatTheCat. I had a feeling that might be the case. I just finished my implementation to work around this restriction in my current project. I'm working through writing tests to cover my handler and handler proxy services. Once I'm done with that I'll open a new issue and create a PR if I can figure out the best way to implement this interface.