[5.0][HttpFoundation] The session must not be a service

[lyrixx]

The session is a data object, and so, like the request, must not be in the container.

[ruian]

👍

[lyrixx]

Or a least, the scope (if still exists), should be request.

[stof]

why would you have a different session for the subrequest ?

[lyrixx]

Did not think about subrequest :(
So I see 2 solutions:

• A new scope: Application
• Or remove it completely from the container.

I think I prefer the second one.

[stof]

what would be the application scope ?

[fabpot]

We don't need a new scope... as I really want to get rid of them in 3.0. So, the session is just an object embedded into the request, and as such, it is already available through the request stack. There is not need for yet another abstraction.

The only thing we need to do is to deprecate the session as a service.

[stof]

@fabpot This might require refactoring the way the session is handled though. session storages look like services.

thus, if we consider that the main access point for the session is through the request, it is a bit weird because the session is injected into the request later (and has a wider scope than the request as it is shared in subrequests)

[ricardclau]

I have mixed feelings with this.

As @stof said, the session (as a concept itself) is not only a data object, it has persistence and there are many use cases where you would want (or at least it is convenient to have) a session service injected in other services

In fact, for instance, the main class in the facebook-php-sdk expects a session object to be injected in order to handle your auth with Facebook in the subsequent requests. And this is used by people creating Facebook apps/videogames with Symfony2

So, if we go via scopes, it becomes a mess (it has more or less been rejected as per @fabpot comment) and I am not sure if deprecating it as a service is convenient at all.

Just my 2 cents, of course :)

[wouterj]

Whatever we do, I don't think we should bind it to request_stack or the Request object. The session is way more than the session provided by the Request workflow.

[ghost]

It is not possible to have different sessions for subrequests.
The session is injected to other services so I am not sure why you would want to remove it from the DIC. Otherwise one would have to inject the Request object into objects that need the session just to get at the session which would clearly add complexity.

[docteurklein]

I'm sharing my thought as it comes.

• Concerning scopes, it's far better to remove them than to add a new one!
• I agree too that a session has sense only in the context of a request, and thus it makes sense to retrieve it through the request object only (to debate).
• session storages are services but session isn't. Currently, the session knows too much about how to store itself.
  Doesn't it make you think of active record vs data mapper ?

What about a data mapper impl. of session storage ? This way the original idea of data object is kept.

[gnugat]

@docteurklein 👍 for a distinction between a session storage service and a session value object

[ghost]

@docteurklein can you explain a little more about "The session knows too much about how to store itself."

[stof]

@docteurklein your third point is abit problematic IMO because we are not totally free for the way the session system works. It depends on the PHP session system

[docteurklein]

@stof if you talk about 5.4's native handlers & co., I think it could be handled the same way as currently, but delegated to the data mapper.

@Drak to be honest, I have nothing against the way session works currently :) I just try to give potential solutions to the problem of session as a data object.
To answer you, it could totally be compared to the way doctrine2 handles persitence of entities.

Just replace the term entity with the term "session object", and you have what I talked about.

[linaori]

I think the session needs an overhaul in general. The SessionInterface along with the handlers and storage are too confusing when it comes to implementation. Imo the session is fine as service, but it's too stateful for that as it is.

[Paxton]

I think there should be strong distinction between a session storage (or a value object) and a session driver, they both should be inject-able, however the first one with very limited usage, maybe even read-only, and replaceable/editable only using a session driver.

I can see a point of injecting different session into sub-requests, and it would be a definitely a robust feature, maybe not used often but still.

I'm not sure how @fabpot will get rid of scopes, but I can see instances of services/events that are fired prior to session.

[znerol]

I assume the OP is referring to the data access object pattern and I agree that it is stupid to put DAOs in the container. However, it is completely legit for container services to provide access to DAOs, i.e. provide methods returning those objects. The problem with the current SessionInterface is that it is half DAO and half service. I think this is the primary source of confusion here.

In many cases a consumer of the session is interested in its DAO interface, i.e. the key-value collection. In those cases it is nice if the collection simply can be accessed using $request->getSession()->get('key');. There is also the alternative way via $container->get('session')->get('key'); which is clearly wrong, and that's probably what lead to this RFC.

However, things get more complicated when working with session bags. Is it more appropriate to call $request->getSession()->getBag('mybag'); rather than $container->get('session')->getBag('mybag')? The bag is clearly a DAO and the session a service in this case. It is rather strange that in this case a method on the request returns some kind of a service instance. Also note that there are methods like start() and migrate() on the SessionInterface which are not really appropriate for a DAO.

Splitting up the SessionInterface into two parts leads to new problems. E.g., what should be returned from Request::getSession()? Just the attribute bag (which implements the DAO)? At the least it could take an optional parameter such that other bags could also be specified.

#6840 proposed to move the session out of HttpFoundation into its own component. This would also lead to [get|set|has]Session() being removed from the request. Instead session bags could be accessed by means of a service and the request could be passed as a parameter, e.g.:

$session_provider->get($request, 'mybag')->get('key');

Removing Request::getSession() and delegating this to a service which even could be in another component comes with the following benefits:

• Only one way to access session data.
• Reduced responsibility of the Request, reduced clutter in HttpFoundation ("session" has two mentions in RFC 2616 and "flash" none).
• Namespacing of session data with less effort.
• Enhanced extensibility of the session service (see the flash bag issues).

[ghost]

I like to change values in the session in my own service. Should I always pass the current request as parameter? Or should I only pass the session (SessionInterface)?

As I understand I should not get session from the container anymore.

[nicolas-grekas]

What about adding RequestStack::getSession() to fix this issue?

[lyrixx]

@nicolas-grekas Initially, I thought about SessionStorage::getSession(), but the RequestStack is a very good idea 👍

[linaori]

The Session is bound to your HTTP layer, so whenever you need it somewhere, you can always pass it as method argument if you really need to modify the session contents in another layer. If you really, really want to modify the session in listeners that do not have a direct access to the request, you can always retrieve it from the current request via the request stack.

If the events do not contain the request object while being part of the request lifecycle, perhaps they can be added. As far as I can see, the Kernel events have a request object and thus a session though: https://symfony.com/doc/current/reference/events.html#kernel-events (given the session is set).

RequestStack::getSession() will not solve anything other than being it being a shortcut for getCurrentRequest()->getSession().

[nicolas-grekas]

We should then ensure subrequests have the session, or tell about getMasterRequest()->getSession() instead. Any blocker to deprecate the session service then? PR welcome if not.

[linaori]

Considering there are enough work-arounds, it seems that that it can be deprecated as service. Worst case scenario, provide an alternative solution to show how to register the Session as service using a factory in the DIC.

[lyrixx]

RequestStack::getSession() will not solve anything other than being it being a shortcut for getCurrentRequest()->getSession().

Yes, it's a shortcut, but a convenient one. That's why I'm in favor to add it to the request stack

[fabpot]

I like the RequestStack::getSession() shortcut as well.

[derrabus]

If we remove the service, can we make it so that controller actions that type-hint for SessionInterface still receive the session?

[linaori]

@derrabus What do you mean exactly? The SessionInterface will not be removed

[derrabus]

@linaori No, but the SessionInterface service will. I wasn't sure if controllers type-hinting for the interface depend on autowiring via controller.service_arguments to receive the session, but this is apparently not the case thanks to Symfony\Component\HttpKernel\Controller\ArgumentResolver\SessionValueResolver. So, those controllers will continue to work without the session service. 👍

[wouterj]

So, I started working on this as I thought "it should not be that hard". But discovered the \Symfony\Component\HttpKernel\EventListener\AbstractSessionListener class, which does exactly the opposite of what is discussed here: if a session service is initialized, it makes sure it's set on the master request:

symfony/src/Symfony/Component/HttpKernel/EventListener/AbstractSessionListener.php

Lines 48 to 66 in a07627e

	public function onKernelRequest(GetResponseEvent $event)
	{
	if (!$event->isMasterRequest()) {
	return;
	}
	$session = null;
	$request = $event->getRequest();
	if ($request->hasSession()) {
	// no-op
	} elseif (method_exists($request, 'setSessionFactory')) {
	$request->setSessionFactory(function () { return $this->getSession(); });
	} elseif ($session = $this->getSession()) {
	$request->setSession($session);
	}
	$session = $session ?? ($this->container && $this->container->has('initialized_session') ? $this->container->get('initialized_session') : null);
	$this->sessionUsageStack[] = $session instanceof Session ? $session->getUsageIndex() : 0;
	}

So that means we deprecate the session service in favor of masterRequest#getSession(), but masterRequest#getSession() depends heavily on the session service. I no longer understand what I should be doing and how we can keep BC with "initializing the session service".

[derrabus]

@wouterj Either way, the session needs to be initialized somehow. At the moment, the container does this and the objective (as I understand it) would be to move that initialization somewhere else. That listener could be the place we're looking for.

Since the session service needs to be kept in Symfony 4 in order not to break BC, that listener will probably be kept as-is for now. All other services provided by the framework that depend on the session should instead depend on the request stack.

In Symfony 5, the session service is removed and the listener will be responsible for constructing the session object before injecting it into the current request.

Does that make sense?

[Pierstoval]

First thing to me is to split the Session object and remove its dependency to the session storage, and refactor the storage so it can do something like $storage->save($session), decoupling both layers (data & persistence).

This would enforce the session to be created by either the storage, or a new factory that may need the storage itself, to create the session object, fetch data from the storage, populate the session with storage data, return the session as a data object.
The SessionListener may use such factory, for BC, and put the session in the Request object as usual.

BC and migration path would be achieved with new contracts, making the "old" session a SessionStorageInterface and injecting the storage with DI, making the session a decorator of the storage (just like it currently is, but by changing the contract). Or even farther: making the current session service a new class that would just make the calls to the factory, using request stack to populate the session in the current request, etc.. There are several ways to keep BC, IMO

WDYT?

[stof]

The first thing here would be to propose solutions about how this split would look like actually. As said previously, this is not easy, due to the fact that we wrap the PHP session handling, and so we are limited by what it allows.

[warslett]

Either way, the session needs to be initialized somehow. At the moment, the container does this and the objective (as I understand it) would be to move that initialization somewhere else. That listener could be the place we're looking for.

Since the session service needs to be kept in Symfony 4 in order not to break BC, that listener will probably be kept as-is for now. All other services provided by the framework that depend on the session should instead depend on the request stack.

RequestStack could be used as a factory and getSession the factory method to protect BC breaks while the session service is deprecated but not removed same as we did for the flash bag in #36063.

The Session could be instantiated in the SessionListener when the master request is received and onKernelRequest is triggered as @derrabus suggests. The problem I can see with this is if a service which injects the session is instantiated by the service container before that onKernelRequest event maybe in another event handler for example. In this situation getMasterRequest will be null so it won't be possible to get the Session from it.

Another approach might be to remove this behaviour from the SessionListener and instead make the RequestStack the owner of the session and set it's session object on each request that is pushed to it. That way we can rely on the Session being available to the service container at any point in the process including to all KernelEvents.

/**
 * @var Request[]
 */
private $requests = [];

/**
 * @var SessionFactoryInterface
 */
private $sessionFactory;

/**
 * @var SessionInterface|null
 */
private $session;

/**
 * @param SessionFactoryInterface $sessionFactory
 */
public function __construct(SessionFactoryInterface $sessionFactory)
{
    $this->sessionFactory = $sessionFactory;
}

/**
 * Pushes a Request on the stack.
 *
 * This method should generally not be called directly as the stack
 * management should be taken care of by the application itself.
 */
public function push(Request $request)
{
    $request->setSession($this->getSession());
    $this->requests[] = $request;
}

...

/**
 * @return SessionInterface
 */
public function getSession(): SessionInterface
{
    if (null === $this->session) {
        $this->session = $this->sessionFactory->createSession();
    }

    return $this->session;
}

We also have to consider applications that might have overwritten the definition of the session service with their own definition. We could maybe add a compiler pass that detects if the session service definition is overwritten (by checking whether it is using RequestStack as a factory) and if so replaces the definition for SessionFactoryInterface with one that uses an implementation that delegates to the service container.

That implementation could be deprecated from the start so that any applications that are overriding the default session service will get deprecation notices. Any application can override the default instantiation of the session, use their own class, register their own bags etc. by implementing their own SessionFactoryInterface.