[HTTP Cache][Session] Something is odd a behaviour was broken from 4.4.4 to 4.4.7

[Plopix]

Symfony version(s) affected: 4.4.x and 5.x

Description

I am not sure that's a bug but in my opinion, that should be addressed. I am then putting it here.
It's hell to manage HTTP Cache now some Symfony logics are kind of conflicting (that's why I am mainly creating the issue) each other.

Everything starts here https://symfony.com/doc/current/http_cache.html

Whenever the session is started during a request, Symfony turns the response into a private non-cacheable response. This is the best default behavior to not cache private user information (e.g. a shopping cart, a user profile details, etc.) and expose it to other visitors.

I agree 200% and I think that perfect for Symfony to behave this way.

But here comes the "hidden" parts:

How about adding in this documentation that we need lazy here:

    firewalls:
        main:
            anonymous: lazy

without the lazy, Session is open -> Response is private

Getting the user from Security Service

trait UserAware
{
    protected $user;

    public function setUser(Security $security): void
        $this->user = $security->getUser();
    }
  
    protected function isLoggedIn(): bool
    {
        return $this->user instanceof User;
    }
}

Pretty odd code for some people maybe. But if this trait is used somewhere, it opens the Session.
It passes code review and uninitiated developers won't measure the consequences.
The fix here is obvious, just inject the Security and call getUser when needed.

Profiler is opening the Session if you are logged in

Another tricky one.
Let's say I have a cacheable page, it does not use the session at all.
When I am not logged in Symfony behaves accordingly, Session is not open -> page is not enforced to be private, must-revalidate

But then if I log in, on the same page, which does not use the Session (lazy firewall correctly set), because I am on dev mode and I have the Profiler, the Profiler, to profile, => open the Session.

So in dev mode, logged in, I end up seeing private, must-revalidate. There is no way to really test the HTTP cache in dev. (that is working in prod mode as it should)

Kind of breaking change in 4.4.6

This is the origin of this issue, today I updated my dependencies to Symfony 4.4.7.
After that( and thanks to automated tests), I've seen the HTTP Cache was not working on our project.
We got the private, must-revalidate (when not even logged in)

I don't expect that on a minor version, but I do understand the fix... that's why it's hell to debug/identify when it happens

With some debugging, I figured it was because of #36063
Issue: #33084
=> The fix for that issue breaks it for our project.

We were injecting FlashBagService and therefore since the PR was merged, our controller was opening the Session even if the FlashBag were not used...

The fix is even trickier here because the solution is to inject the Session and get the flashbag from the Session Service. BUT, the SessionInterface (the Service) is not the implementation and therefore there is no getFlashBag in the interface so we need to do getBag('flashes')
=> that is explained nowhere and again for uninitiated developers I don't think this is clear...

Here I don't know the solution, maybe the Concrete class should be the service? maybe both should be Services?

Bonus: Improve the profiler to show when thee Session is opened.

The debug is also a bit tricky when you detect the page is flagged private, must-revalidate that would be awesome to have something that tells us.

That's it, I am not complaining here, I open the subject ;-)
I think there is a subject, at least it was something to share.

What do you think?

[warslett]

The flashbag service is actually being deprecated in 5.1 (see #36273) so you will get deprecation notices if you typehint FlashBagInterface. $session->getFlashBag() is just a convenience method for a specific use case of the session so it does make sense that it isn't part of the SessionInterface which defines a more abstract sort of contract.

You can call $session->getBag('flashes') but an implementation of SessionInterface might not even have a flash bag therefor there is no explicit contract in place for getting the flashbag from a SessionInterface you have to trust that you have an implementation which has a session bag called "flashes" which implements FlashBagInterface. Maybe this is an academic point I'm not sure.

maybe the Concrete class should be the service

This might be better with the current design.

I wonder whether a better way for client code to interact with flash messages is with a separate service which exposes the same sort of interface as FlashBagInterface but instead wraps the session and starts the session when it is first written to as opposed to the flash message functionality being an extension of the session bag that the messages are stored in.

This also relates to an ongoing discussion about how to expose the session via the service container.

[guillaumesmo]

I also experience a breaking change in 4.4.6 due to #36063

requests that were previously not writing sessions suddenly started doing so after upgrading Symfony

reverting the changes from #36063 prevented those additional writes in my case

I must admit we're using the flashbag here and there and I was surprised this was not writing sessions, but anyways, the behavior changed

[nicolas-grekas]

Solved by #36364