[Session] Add getFlashBag() to SessionInterface

[znerol]

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

[yguedidi]

This is a BC break

[linaori]

It's most likely not going to happen, it has been suggested before. I've already made a PR I find better solution: #11957

You can already use the session.flash_bag service and inject it. Session as a service will most likely disappear as it is right now in symfony 3.0, check #10557

[znerol]

Session as a service will most likely disappear as it is right now in symfony 3.0

This is one more reason to have the method on the interface, because then the only way to cleanly get at it is through Request::setSession() and Request::getSession()

[znerol]

@yguedidi updated the issue summary. Still I'm wondering on the impact of this BC break. Existing third-party session implementations are forced to add a new method, is that the problem?

[yguedidi]

@znerol Yes it is. Maybe they aren't open sources implementations, but maybe they are in closed sources projects.

[linaori]

This is one more reason to have the method on the interface, because then the only way to cleanly get at it is through Request::setSession() and Request::getSession()

There are no concrete plans, but I doubt this will be added as it is.

[znerol]

I did my share in grepping and pickaxing through the git history. Here is what I found:

The large session refactoring in #2853 introduced the SessionInterface. For some time that interface had a method getFlashes() which was removed by commit dad60ef of the pull request. The removal likely happened accidentally because it is clearly visible from the same diff that SessionHelper used this method to retrieve flash messages. Note that SessionHelper still happily uses its successor getFlashBag() in master (without the instanceof checks shown in the example code of #11957). Other usages are in FrameworkBundle/Controller/Controller.php, WebProfilerBundle/Controller/ProfilerController.php, WebProfilerBundle/EventListener/WebDebugToolbarListener.php, HttpKernel/DataCollector/RequestDataCollector.php.

How likely is it that a third party SessionInterface implementation would even work with Symfony if it would strictly adhere to the interface and leave out getFlashBag()?

[linaori]

@znerol this was back in 2.1 if I checked correctly. As of 2.3 semver is being followed and BC breaks should be avoided, this is one of those cases.

[znerol]

@iltar I do not argue that a BC break is okay here because it was fine then. I just want to point out that this was broken from the beginning and that it is still in master.

[jakzal]

Duplicate of #11279 #10036 #5568.

It's not broken, it's like this by design (I'm talking about getFlashBag not being on SessionInterface, not about client code using SessionInterface as if it was a Session). See @Drak's reasoning for rejecting this in the past here: #5568 (comment)

getFlashBag() is just a shortcut method provided by Symfony's implementation of SessionInterface. We don't want to force all the implementations to support flash bags. You can use it Session as a typehint, if you really want to rely on this specific implementation. Otherwise, use getBag(), which belongs to the interface.

The SessionInterface is technically not tagged with an @api (its methods are), but we try to limit BC breaks as much as possilble.

[znerol]

How is FrameworkBundle/Controller/Controller.php not broken when it operates on SessionInterface and still wants to use getFlashBag()?

[linaori]

@znerol that's why you can inject the session.flash_bag service. I know it's broken and I'd like to see it fixed. But at this moment, the SessionInterface doesn't and shouldn't know about framework specific implementation.

[Tobion]

@znerol FrameworkBundle/Controller/Controller.php does not operate on SessionInterface but on the session service which is configured to be https://github.com/symfony/symfony/blob/master/src/Symfony/Bundle/FrameworkBundle/Resources/config/session.xml#L8

[znerol]

@Tobion I mentioned other examples as well, e.g. SessionHelper (which retrieves the session from the request and the request is clearly type hinted against SessionInterface). Also as an exercise for the interested reader: Try to operate any Symfony application with a Session implementation lacking getFlashBag(). An easy way to do this is simply to remove the method. I guess that in 99% of all cases your application will explode instantly.

[Tobion]

@znerol I agree with you but your example was not ideal. There are also several other design problems I already reported: #9233

[fabpot]

Closing for the reasons explained by @Tobion. #9233 has all the information we need to move forward. It's now up to someone to think about how to tackle this issue for the next Symfony version.