Does this suppose to help setting the a custom session id usingsetId($some_id) function ofSymfony\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 yourSessionIdInterface on top of the stack. I guess this isn’t an issue since you approved this PR?

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

Okay, just tried to do that and I got aServiceCircularReferenceException… 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 whereSessionHandlerInterface did not exist. They weredeprecated thenremoved, butAbstractProxy andSessionHandlerProxy escaped death thanks to#24523 because the latter wouldturns any handler into an instance ofAbstractSessionHandler. As it doesn’t seem to be the case maybe I’m misunderstanding something 🤔

TheSessionHandlerProxy always has been documented as an extension point (like inhttps://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 aServiceCircularReferenceException because theframework.session.handler_id service is aliased bysession.handler, which is aliased bySessionHandlerInterface (you can fix this issue by wiring your wrapped handler yourself though). Also by doing this you couldn’t use decorators like theMarshallingSessionHandler.

That’s why I thought you were supposed to decoratesession.handler instead, which requires less configuration but more boilerplate code to proxy all ofSessionHandlerInterface,SessionUpdateTimestampHandlerInterface (andSessionIdInterface?) methods.

Closing, since there already is a way to configure aSessionIdInterface 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 theframework.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 theSessionHandlerProxy to implement\SessionIdInterface. But I think it is more straightforward to have the\SessionIdInterface implemented inSessionHandlerProxy as initially submitted in this PR. Then we have an implementation ofSessionHandlerProxy 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 extendSessionHandlerProxy as a value toframework.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 inhttps://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 extendsSessionHandlerProxy and implements\SessionIdInterface. Then you have to explicitly wire the main handler to the proxied handler doing something like this:

framework:session:enabled:truestorage_factory_id:session.storage.factory.nativehandler_id:App\SessionHandlerProxyservices:App\SessionHandlerProxy:arguments:$handler:App\SessionHandler

You could implement all the handler functionality to theApp\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.