[FrameworkBundle] start session on flashbag injection

[warslett]

Q	A
Branch?	3.4
Bug fix?	yes
New feature?	no
Deprecations?	no
Tickets	Fix #33084
License	MIT

This PR addresses an issue whereby if the FlashBag is injected into the application using the default service configuration, we cannot rely that the session has been started. This behaviour is in contradiction to the docs:

Sessions are automatically started whenever you read, write or even check for the existence of data in the session.

This is because symfony ensures the session has been started on calls to getFlashBag() which is normally how the flashbag will be accessed but this is not called if you inject the FlashBag directly into the container.

I have addressed this issue by changing the way the Flashbag service is built so that it uses Session as a factory service and getFlashBag as a factory method. This means that anywhere in symfony where FlashBag is injected can now rely on the fact the session is started.

I have also added a new functional test to verify this behaviour.

[warslett]

@nicolas-grekas should we also configure session.attribute_bag in the same way for consistency?

[nicolas-grekas]

yes that'd make sense

[nicolas-grekas]

Then on master, we could deprecate these two services.

[nicolas-grekas]

@warslett up to send a PR to deprecate these?

[warslett]

@nicolas-grekas here: #36273 I get build failures but doesn't seem related. Are the builds on master wonky right now?

[fabpot]

Thank you @warslett.

[fabpot]

@warslett Can you prepare a PR on master to deprecate the two services as suggested by Nicolas (I've merged 3.4 up to master)?

[warslett]

@fabpot sure I can do that this evening. I'll use the message: "The "%service_id%" service is deprecated since Symfony 5.1"

[ekosogin]

After this this bug fix new bug was created if session with namespaces was configured

session:
    class: Symfony\Component\HttpFoundation\Session\Session
    arguments:
        - '@session.storage'
        - '@Symfony\Component\HttpFoundation\Session\Attribute\NamespacedAttributeBag'
        - '@session.flash_bag'
Symfony\Component\HttpFoundation\Session\Attribute\NamespacedAttributeBag: ~

Circular reference detected for service "session", path: "session -> session.flash_bag -> session".

[warslett]

@ekosogin you don't actually need the @session.flash_bag parameter on there. You should be able to do this:

session:
    class: Symfony\Component\HttpFoundation\Session\Session
    arguments:
        - '@session.storage'
        - '@Symfony\Component\HttpFoundation\Session\Attribute\NamespacedAttributeBag'
Symfony\Component\HttpFoundation\Session\Attribute\NamespacedAttributeBag: ~

[warslett]

@fabpot I have a branch with the deprecation on master but haven't created a PR yet (sorry virus got in the way). Do we need to consider @ekosogin's problem first? It is simple enough to remove the session.flash_bag argument from his session definition but if lots of people using symfony have overridden the default session service definition and injected either of these two soon-to-be deprecated services then they might be experiencing the same problem.

[ekosogin]

The problem with circular reference still present with this configuration

session:
    class: Symfony\Component\HttpFoundation\Session\Session
    arguments:
        - '@session.storage'
        - '@Symfony\Component\HttpFoundation\Session\Attribute\NamespacedAttributeBag'
Symfony\Component\HttpFoundation\Session\Attribute\NamespacedAttributeBag: ~

[nicolas-grekas]

FYI, discussions on closed PRs are like oral talks: we'll all forget about them.

[jmalinens]

we have the same issue.

This is the 3rd time in a year where in minor symfony version upgrade breaks something for us in sessions :/

this won't work for us anymore in 4.4.6:

    # Symfony services
    session.flash_bag:
        class: OurCustom\FlashBag

    session.attribute_bag:
        class: Symfony\Component\HttpFoundation\Session\Attribute\NamespacedAttributeBag
        calls:
            - [setName, ["login"]]
        arguments: ['login', '.']

[jmalinens]

@nicolas-grekas looks like the ticket is about session flash bag and that' is ok to remove it from Session service definition but

https://github.com/symfony/symfony/pull/36063/files#diff-1777be3d93e71a9295d7fed8ed262c3eL16

but session.attribute_bag is also removed in this PR.

We rely on NamespacedAttributeBag so we store symfony session in non-root namespace.

With this change on: https://github.com/symfony/symfony/blob/v4.4.6/src/Symfony/Component/Security/Http/Firewall/ContextListener.php#L119

symfony ignores our namesapce (login) and because of that can not find symfony session data

[nicolas-grekas]

Fixed in #36261
Please abstain from commenting on closed PRs next time, except to tell about the new issue you created to report any regression.