-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
[Session] Changed session.flash_bag service to publicly available #11957
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
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 |
@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:
In my PR I have made it possible to inject the 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();
}
} |
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 |
@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 Worst case scenario: You put stuff in the FlashBag service, but Session is using a different one thus your values are not stored. |
@iltar in the case of FrameworkBundle, the service is already passed to the session, as it is there in the service definition |
@stof again, correct. However, for custom session services, there is no way of properly defining a custom session other than overwriting the This PR solves the issue of Because this allows people to use the |
I guess it's a topic for the next team meeting. |
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
onRequest
or type hintSession
instead ofSessionInterface
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 bysession.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.