8000 5.4.0: `SessionListener::onKernelResponse` listener is registered twice in test env · Issue #44345 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

5.4.0: SessionListener::onKernelResponse listener is registered twice in test env #44345

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.

Sign up for GitHub

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

Jump to bottom
Closed
mhujer opened this issue Nov 29, 2021 · 5 comments
Closed

5.4.0: SessionListener::onKernelResponse listener is registered twice in test env #44345

mhujer opened this issue Nov 29, 2021 · 5 comments
Labels
Bug Status: Needs Review

Comments

@mhujer
Copy link
Contributor
mhujer commented Nov 29, 2021

Symfony version(s) affected

5.4.0

Description

Symfony\Component\HttpKernel\EventListener\SessionListener::onKernelResponse() listener is registered twice in test environment.

Our tests for controllers where we set NO_AUTO_CACHE_CONTROL_HEADER flag started failing, because the flag is reset on the first call of onKernelResponse and the subsequent call overwrites the previously set headers.

How to reproduce

It works fine on 5.3:

$ symfony new symfonysessionlistenertest53 --version="5.3.x@dev" --full
$ cd symfonysessionlistenertest53 
$ symfony console debug:event --env=test

"kernel.response" event
-----------------------
....
  #11     Symfony\Component\HttpKernel\EventListener\SessionListener::onKernelResponse()            -1000
...

But is registered twice on 5.4:

$ symfony new symfonysessionlistenertest54 --version="5.4.x@dev" --full
$ cd symfonysessionlistenertest54 
$ symfony console debug:event --env=test

"kernel.response" event
-----------------------
...
  #10     Symfony\Component\HttpKernel\EventListener\SessionListener::onKernelResponse()            -1000
  #11     Symfony\Component\HttpKernel\EventListener\SessionListener::onKernelResponse()            -1000
...

Possible Solution

No response

Additional Context

Note: The listener is set only once in dev and prod envs.

I guess that the issue may be related to #41390.
@mhujer mhujer added the Bug label Nov 29, 2021
@OskarStark
Copy link
Contributor

Friendly ping @alexander-schranz

@alexander-schranz
Copy link
Contributor
alexander-schranz commented Nov 30, 2021 •
edited
Loading

We did replace the TestSessionListener with the new SessionListener this actually causing now that the event seems to be registered multiple times.

https://github.com/symfony/symfony/pull/41390/files#diff-2b07ed6c629fe7c7a6ed6a82c36f039b0af6ad1be83d5656787082dd281e6e48R38

A workaround maybe would here that we replace the SessionListener with the TestSessionListener instead adding a new service, but would be interested if all works correctly in your case if there is only 1 SessionListener. Or just alias the session listener service.

@mhujer can you check if you comment the following lines out and so avoid the additional SessionListener. If then work all correctly?: https://github.com/alexander-schranz/symfony/blob/b3e4f6657c0c47e1b5ba85e4554a3c273ee61066/src/Symfony/Bundle/FrameworkBundle/Resources/config/test.php#L38-L46

@mhujer
Copy link
Contributor Author
mhujer commented Nov 30, 2021

@alexander-schranz yes, commenting out the suggested lines fixes the issue (the listener is registered only once).

If anyone else is facing the similar issue, I added this as a workaround to my Kernel class and the tests are now passing:

...
use Symfony\Component\DependencyInjection\Compiler\CompilerPassInterface;
use Symfony\Component\DependencyInjection\ContainerBuilder;
...

class Kernel extends BaseKernel implements CompilerPassInterface
{
    ...

    public function process(ContainerBuilder $container): void
    {
        if ($this->environment !== 'test') {
            return;
        }

        // fix https://github.com/symfony/symfony/issues/44345
        $container->removeDefinition('test.session.listener');
    }

@alexander-schranz
Copy link
Contributor

@mhujer Thank you for testing this out. So we should make sure that only one session listener is registered.

@jderusse What do you think that we alias session_listener to test.session.listener then there should only be one listener? Maybe doing it after here:

symfony/src/Symfony/Bundle/FrameworkBundle/DependencyInjection/FrameworkExtension.php

Line 341 in 60ce5a3

$this->registerSessionConfiguration($config['session'], $container, $loader); 

            if (!empty($config['test'])) {
                // test listener will replace the exist session
8000
 listener
                // we are aliasing to avoid duplicated registered events
                $container->setAlias('session_listener', 'test.session.listener');
            }

@alexander-schranz alexander-schranz mentioned this issue Nov 30, 2021
Merged
@alexander-schranz
Copy link
Contributor

@mhujer Can you give #44359 a try?

nicolas-grekas added a commit that referenced this issue Dec 1, 2021
@nicolas-grekas
bug #44359 Avoid duplicated session listener registration in tests (a…
2ace406 
…lexander-schranz)

This PR was squashed before being merged into the 5.4 branch.

Discussion
----------

Avoid duplicated session listener registration in tests

| Q             | A
| ------------- | ---
| Branch?       | 5.4
| Bug fix?      | yes
| New feature?  | no
| Deprecations? | no
| Tickets       | Fix #44345
| License       | MIT
| Doc PR        | -

Avoid duplicated session listener registration in tests.

Commits
-------

366cb1a Avoid duplicated session listener registration in tests
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Bug Status: Needs Review
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests

3378
5 participants
@nicolas-grekas @mhujer @OskarStark @alexander-schranz @carsonbot
0