[Session] Changed session.flash_bag service to publicly available

[linaori]

Q	A
Bug fix?	no
New feature?	yes
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets	#11279
License	MIT
Doc PR	-

Main issue is described in my ticket. The problem is that in order to get the flash bag, you have to either ignore the SessionInterface on Request or type hint Session instead of SessionInterface during injection. Neither solutions work nicely with custom implementations of the session.

By defining session.flash_bag as public service, you can just inject this as it's created by session.getFlashBag. This makes sure that you always have the right bag, even if you decide to inject a custom flash bag in the session service.

For the default implementation, the old variant is used as injection. I have left this as it is so that people can still choose to omit it when constructing or putting in their own flash bag, with their own key if desired. This PR should be 100% BC.

[jakzal]

I'd think twice before doing this in the core, especially since we're considering to get rid of a session as a service (see #10557). You can always register such a service in your project.

Why do you say that type hinting with Session doesn't work with custom session implementations? Apart from situations that a custom implementation doesn't extend the Session (but that requirement is clearly communicated by a type hint).

[linaori]

@jakzal there's nothing concrete planned yet and this is an issue many programmers face. I have provided some cases below.

Each case has its own problems, however, the ones coming back are:

• You always need the session to get the flash bag because the session.flash_bag is private
• You always have to do a strict check to make sure the method is available
• You always have to inject either the request or session to get the flash bag, no other means available
• You can typehint Session, but why bother interfacing at all then
• You can define your custom FlashBag service by overriding the parameter, but you "can't" inject it because it's private

In my PR I have made it possible to inject the FlashBagInterface. This makes it a standalone from the session. The fact that it uses the session in the container is something that can change in 3.0, however, I still want my flash bag to be injected so I get do set() directly without always requiring the session or request (makes unit-testing easier too).

Case 1: plain action issue

public function someAction(Request $request)
{
    // just because SessionInterface doesn't have getFlashBag
    if (($session = $request->getSession()) instanceof Session) {
        $session->getFlashBag()->set('a', 'b');
    }
}

Case 2: Injection a session

class SomeService
{
    private $session;

    public function __construct(SessionInterface $session)
    {
        $this->session = $session;
    }

    public function someMethod()
    {
        // have to do the same check as in case 1 because SessionInterface doesn't have it
    }
}

Case 3: Injection the session that deviates from the interface, not generic anymore

class SomeService
{
    private $session;

    public function __construct(MyCustomSession $session)
    {
        $this->session = $session;
    }

    public function someMethod()
    {
        $this->session->getFlashBag()->setSomething();
    }
}

[stof]

Injecting a private service in your service is perfectly allowed and valid. What is forbidden for a service marked as private is retrieving it dynamically through $container->get() (because it could have been inlined and dropped)

[linaori]

@stof correct. However, using that service as injection in its current state is not 100% trustworthy. By default it's passed to the Session. However, it's not guaranteed to be that session as the FlashBag parameter is optional. If it's not passed along, it will be generated inside. Using the service I created catches this corner-case.

Worst case scenario: You put stuff in the FlashBag service, but Session is using a different one thus your values are not stored.

https://github.com/symfony/symfony/blob/master/src/Symfony/Component/HttpFoundation/Session/Session.php#L55

[stof]

@iltar in the case of FrameworkBundle, the service is already passed to the session, as it is there in the service definition

[linaori]

@stof again, correct. However, for custom session services, there is no way of properly defining a custom session other than overwriting the .class and usage of the service can't be guaranteed.

This PR solves the issue of SessionInterface not having getFlashBag and you no longer have a visible dependency on the Session nor Request any more. It also fixes the issue where the getFlashBag might not return the service but a different object (because session allows null as argument). Next to that, by making it public, people can do $container->get('session.flash_bag')->set(...). This is how it's presented in 90% of the documentation but currently not possible.

Because this allows people to use the session.flash_bag service, it's also made forward compatible with the idea of removing the session service. This service change won't break for anyone and makes it more robust, sounds like a win-win situation to me.

[linaori]

ping @stof @fabpot it's been quite a while since I've read anything regarding the redesign of sessions. Is there anything planned for this? I have this PR and #11279 open, I was wondering if I should close them or keep them open + rebase against 2.8.

[Tobion]

I guess it's a topic for the next team meeting.