should we add it using @method? And cleanup in 5.0 ...

That was what I wanted to propose initially, but it seems it was decided against that. See #5568 (comment)

im still skeptical about method_exists, that's not a contract.

Yes, I also don't like the duck typing here. But it's the most robust way I see at the moment for a bugfix release.

To improve this, we could introduce a new interface that provides getFlashBag() with 4.4 and trigger a deprecation if the session implements getFlashBag() without the interface.

Then we could additionally try getBag('flashes') by convention for a fallback.

The assumption that the flash bag is stored as flashes could be wrong.

for a bugfix release

Right, i wasnt aware the PR targets 3.4 :) But i dont find it robust without any additional type check for FlashBagInterface. Hence the suggested helper to tackle the boilerplate.

The assumption that the flash bag is stored as flashes could be wrong.

Not if we type check as such :)

Im thinking

function getMeAFlashBag(SessionInterface $session): ?FlashBagInterface
{
    if instanceof Session; return getFlashBag()
    if getBag(flashes) instanceof FlashBagInterface; return getBag('flashes')
    return null
}

FlashBag::createFromSession

That wont work for 3.4 since it's not final. So the drawback is we need to add an internal new symbol in 3.4. But it's the robust way IMHO that remains viable for now without feature creeping it.

That'd break if I had a session like this one:

class MyOwnSession implements SessionInterface
{
     //…

    public function getFlashBag(): FlashBagInterface
    {
        return $this->getBag('my_own_flashes');
    }
}

Fair :) 3.4 needs a patch either way. For 4.4 we could let the framework configure the intended bag name (e.g. my_own_flashbag), or fix the type system.

im still skeptical about method_exists, that's not a contract.

My point here was that the use of getFlashBag is developer knowledge that is not documented anywhere and not in any contract. That's awkward, so an awkward solution makes them acutely aware of the problem 🙂

For 4.4 we could let the framework configure the intended bag name (e.g. my_own_flashbag), or fix the type system.

I feel like fixing the type system would be the more sustainable solution in the long run, i.e., the @method on the interface, issue a user deprecation when a class is encountered that implements SessionInterface but does not have a getFlashBag method, and then in 5.0 add the method to the interface.

the use of getFlashBag is developer knowledge that is not documented anywhere and not in any contract

IMHO the only truth is

so if anything, we expect a FlashBagInterface from calling getFlashBag(). But letting it crash at this point is fine i guess :)

I agree with #5568 (comment), we don't need it in a contract per se, having the bag name is sufficient.

I don't think the flash bag method will ever be added to the interface: #20258 (comment)

having the bag name is sufficient.

But what if people are implementing a SaaS and have multiple flash bags, one per tenant?

Not sure how this would be something per tenant, as multi tenancy is usually not recommended at application level

Fair enough. Just meant that the fact that there is one flash bag is also an assumption, and there are reasons there might be multiple, with different names.
I don't think the framework should make that impossible / require work-arounds to get it to work.

I don't think the flash bag method will ever be added to the interface: #20258 (comment)

No, but we can introduce a separate interface for that method and end the duck typing with 5.0.

Wouldn't a completely separate interface with a separate implementation for flash messages be nicer. as opposed to retro-fitting an interface to a flawed design?

If we add an interface for getFlashBag() to Session than all you get is a flash bag. I don't want a flash bag, I want to add / retrieve flash messages. That something somewhere is using a flash bag is fine by me, but not relevant to what I want to do (it's a leaky abstraction).

So how about something like

interface FlashMessageProvider
{
    public function add(string $type, $message): void;
    public function get(string $type, $default = []): array;
    public function getAll(): array;
    public function peek(string $type, $default = []): array;
    public function peekAll(): array;
    public function has(string $type): bool;
}

(I'm not stuck on the name, feel free to provide a better name!)

And then the default implementation could be injected a SessionInterface and then the service can add a FlashMessageBag of its own to that SessionInterface:

class SessionFlashMessageProvider
{
    private $bagName;
    public function __construct(SessionInterface $session)
    {
        $flashMessageBag = new FlashMessageBag();
        $this->bagName = $flashMessageBag->getName();
        $session->registerBag($flashMessageBag);
    }
}

The Session class itself would then no longer create and register a flash bag. so all notion of flash messages is gone from the Session class.

Later on the methods of the FlashMessageBagInterface can be simplified; it doesn't need to know about the difference between get and peek; that's a service consideration, not a storage consideration. Or maybe the FlashMessageBagInterface can be removed altogether, leaving just the default implementation for the SessionFlashMessageProvider.

So then it is using the session to store and read flash messages, but the session itself has no idea this is happening. Which, going by the current implementation, is what we wanted in the first place.

Also, if people want to store the messages elsewhere / have multiple FlashMessageBags / whatever, they are free to implement their own FlashMessageProvider and go nuts 🙂

Additionaly, as a migration path, Session::getFlashBag() would raise an E_USER_DEPRECATED stating that the new flash message service must be used instead, and internally the classes like AbstractController and AppVariable can already use the new service. So the only people who will have to change their code are the people who request call getFlashBag() from the session manually.

To return the flash bag from Session::getFlashBag() we may have to add a temporary BC layer to registerBag that checks if the bag implement FlashMessageBagInterface and keep a reference to it for when someone calls getFlashBag(). This BC layer can be removed once the getFlashBag() method is removed.

Do you think this would be viable? If so, I'll work it out some more and submit an RFC.

@rpkamp The interface that you’re describing is the FlashBagInterface. We have that already and I don’t see a reason why we should create a copy of it nor would I need another abstraction of it.

The interface is indeed the same, but it puts it top level, disconnected from the session (yes, they are connected by implementation, but not by concept).

The implementation right now is in the Session but pretends it's not, which is weird. If as it seems we don't want it in the session, we should not put it there, but somewhere else.

Also please note that if we introduce such a FlashMessageProvider, the FlashBagInterface can be trimmed to down, so they won't be the same anymore.

The user land code will be nicer though (IMO):

$this->flashMessageProvider->add('notice', 'Something happened!');

as apposed to

$this->session->getFlashBag()->add('notice', 'Something happened');

(which violates the law of Demeter BTW)

On the other hand, people will mostly use the AbstractController or AppVariable to interact with flash messages anyway, and for that use case it doesn't really matter how it is implemented, and adding an interface would make it already better than it is now, so maybe we should just do that.

There's work in progress to remove the session from the container, because it's an object bound to http context. If a separate object would be introduced for flashes, it should probably not be used as service.

The user land code will be nicer though (IMO):

The flash bag is already a standalone service. You don't have to take the detour via the session. Also, if you directly use the flash bag, your userland code does not need to care that the session is used to persist the flashes.

class MyService
{
    public function __construct(FlashBagInterface $flashBag)
    {
        $this->flashBag = $flashBag;
    }

    public function doSometing()
    {
        // …
        $this->flashBag->add('notice', 'Something happened');
        // …
    }
}

What about removing access to the flash bag from the Session altogether and properly inject the FlashBagInterface (explicitly)?

@fabpot that would introduce the same coupling as to the @session. Imo the flash bag (and session) are stateful objects that should be passed by context, similar to how the request is used. Injecting the flash bag (as well as the session), in an event listener to set a message for example, would fail if triggered from the commandline (as in, trying to set session data in the command).

Seeing the Request is usually the point of entry for events or otherwise available in the majority of code and passed along as request related state, perhaps this should be a responsibility of the Request?

At least it would loose it's dubious state of being implemented in the Session but not wanting to be implemented there, so in my opinion it is certainly better than the status quo.

Also, moving the session to the Request seems weird, since a Request is ephemeral, but session data is not, so once you throw away the Request the session data is still there.

Injecting the flash bag (as well as the session), in an event listener to set a message for example, would fail if triggered from the commandline (as in, trying to set session data in the command).

When the flash messages are moved to the Request so the listener would set a flash message on the Request this would still fail because there is no Request. Different check, same effect.

I think #10557 is the ticket for this discussion. :-)

Also, moving the session to the Request seems weird, since a Request is ephemeral, but session data is not, so once you throw away the Request the session data is still there.

@rpkamp The session is already present: Request::getSession() : SessionInterface

Closing: this staled, and doesn't solve a real-world use case (or at least it's not clear which one it solves.)
@fabpot's suggestion above might be worth it, on master.
PR welcome.

When using a form authenticator (AbstractFormLoginAuthenticator), the method "onAuthenticationSuccess" output a redirection. I would like to display a message of success for the user who successfully logged in. The use of the flashbag from the authenticator would have been nice for that use case. Do you have advise to solve this use case?
Thanks in advance.

Sounds like something unrelated from this ticket @Saryon61. I would recommend you open a new ticket with your question.