Skip to content

[Session, 5.4.0-RC] Can't use session before an HTTP request is made in functional tests #44253

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

Closed
Pierstoval opened this issue Nov 24, 2021 · 6 comments

Comments

@Pierstoval
Copy link
Contributor

Pierstoval commented Nov 24, 2021

Symfony version(s) affected

5.4.0-RC1

Description

It seems that there were some changes in the Session that change the initialization process in the KernelBrowser or other related services. Not sure if this is a BC break, a bug, or anything else, so reporting it anyway just in case.

How to reproduce

  • Create a Controller that makes some checks on the Session, one $session->get('some_key') should suffice.
  • Create a functional test implementing WebTestCase
  • Get the HTTP client using $client = self::createClient()
  • Modify the session with something like $client->getContainer()->get('session')->set('some_key');
  • Save the session with $client->getContainer()->get('session')->save()
  • Make an HTTP request with $client->request(...) on the route that's defined by the aforementioned Controller
  • In your Controller, using a debugger or dump() statements, you'll notice that the value of some_key is not set in the Session

Additionnally, if you want to reproduce it with the project that made me discover the "issue" (not sure if it's a BC break or not), you can clone this project: https://github.com/Pierstoval/CorahnRin/blob/5.4-reproducer/tests/CorahnRin/Step/AbstractStepTest.php#L33-L35

To reproduce using the project, here are the steps:

  • You need Docker and Make to install it, and do it via make install.
  • Comment the three lines 33, 34 and 35 in the Tests\CorahnRin\Step\AbstractStepTest class
  • Run only the Tests\CorahnRin\Step\Step07SetbacksTest::testNoSetback test

The test will fail because the GeneratorController (coming from the PierstovalCharacterManagerBundle) checks the session to get the values from $session->get('character.corahn_rin'), which is supposed to be already set in the Tests\CorahnRin\Step\AbstractStepTest::submitAction() method beforehand.

Possible Solution

The current workaround I'm using is to make an HTTP request to any URL of the project that sets the session, but does not need to get elements from it. Basically, a GET request on most pages that check if user is logged in should do the trick.

Additional Context

The same happens if you make an HTTP request and execute $client->restart(), it acts just like the Client was recreated, maybe because of the lack of Cookie HTTP header.

@fancyweb
Copy link
Contributor

fancyweb commented Nov 25, 2021

Hello, thank you for your report and all the details on how to reproduce the issue.

I found out that https://github.com/symfony/symfony/pull/41390/files#diff-2e73c153ff851124cb2a93c1dde4e21be01feeb234ad37a4fdfe4a537726f6dbR72 broke your case and indeed causes a BC break. The session you use in the test is not considered as started because of

. Your test will pass if you call $session->start() after $session->save().

At the moment, I don't know what the correct fix is however.

@Pierstoval
Copy link
Contributor Author

After having thought about it, it should technically be fine to have such behavior, especially in Symfony 6 (because BC are fine in majors).

After all, Session is supposed to be a Request-based object, and IIRC there's ongoing work to detach the Session from the container (just like the Request was removed from the container in Symfony 2.4).

Updating the Session before making an HTTP request shouldn't technically happen, because it doesn't make sense.

If at some point this break is fixed in 5.4, I'd suggest to keep it for 6.0, for consistency with how Session should be handled (maybe this is another issue then)

@alexander-schranz
Copy link
Contributor

alexander-schranz commented Dec 16, 2021

Could be related to: #44553

@Pierstoval can you give a try:

The reproducer seems to require a heavy load of docker build. I could not get it to work that it could connect to the database.

There are currently also some other fixes about sessions which could fixes the problem:

but think #44634 would from my point of view make the most sense that is effect.

@gbirke
Copy link

gbirke commented Jan 11, 2022

I have run into the same problem, wanting to run a functional test for controller behavior that's dependent on session state. As a workaround, I have created a trait for functional tests that allows to inject a session with the right values into the request:

trait PrepareSessionValuesTrait
{
	public function prepareSessionValues( array $values ): void {
		static::getContainer()->get( 'event_dispatcher' )->addListener( 
                     KernelEvents::REQUEST, 
                     function ( RequestEvent $event ) use ( $values ) {
			/** @var Session $session */
			$session = static::getContainer()->get( 'session.factory' )->createSession();
			foreach( $values as $k => $v ) {
				$session->set( $k, $v );
			}
			$event->getRequest()->setSession( $session );
		} );
	}
}

A test would look like this:

$client = self::createClient();
$this->prepareSessionValues( ['my_value' => 'switcheroo' ] );
$client->request( /* request here */ );

For me, this workaround is perfect and I could contribute a PR that adds the trait (and some documentation) if you like. I'd be also interested in criticism and improvements to my approach.

gbirke added a commit to wmde/fundraising-application that referenced this issue Jan 11, 2022
Remove all instances where we access the session through the container,
replace with access through the request object.
For E2E tests, use the PrepareSessionValuesTrait to get around
symfony/symfony#44253

Remove session key for donor address change - that was needed for the
Twig-based skins that rendered HTML, now the data has moved to the
client side.

Adapt our custom AccessDeniedException to have default empty string
message.
gbirke added a commit to wmde/fundraising-application that referenced this issue Jan 11, 2022
Remove all instances where we access the session through the container,
replace with access through the request object.
For E2E tests, use the PrepareSessionValuesTrait to get around
symfony/symfony#44253

Remove session key for donor address change - that was needed for the
Twig-based skins that rendered HTML, now the data has moved to the
client side.

Adapt our custom AccessDeniedException to have default empty string
message.
gbirke added a commit to wmde/fundraising-application that referenced this issue Jan 13, 2022
Remove all instances where we access the session through the container,
replace with access through the request object.
For E2E tests, use the PrepareSessionValuesTrait to get around
symfony/symfony#44253

Remove session key for donor address change - that was needed for the
Twig-based skins that rendered HTML, now the data has moved to the
client side.

Adapt our custom AccessDeniedException to have default empty string
message.
gbirke added a commit to wmde/fundraising-application that referenced this issue Jan 14, 2022
Remove all instances where we access the session through the container,
replace with access through the request object.
For E2E tests, use the PrepareSessionValuesTrait to get around
symfony/symfony#44253

Remove session key for donor address change - that was needed for the
Twig-based skins that rendered HTML, now the data has moved to the
client side.

Adapt our custom AccessDeniedException to have default empty string
message.
gbirke added a commit to wmde/fundraising-application that referenced this issue Jan 17, 2022
Remove all instances where we access the session through the container,
replace with access through the request object.
For E2E tests, use the PrepareSessionValuesTrait to get around
symfony/symfony#44253

Remove session key for donor address change - that was needed for the
Twig-based skins that rendered HTML, now the data has moved to the
client side.

Adapt our custom AccessDeniedException to have default empty string
message.
@alexander-schranz
Copy link
Contributor

Does this issue still appear or can it close?

@carsonbot
Copy link

Hey, thanks for your report!
There has not been a lot of activity here for a while. Is this bug still relevant? Have you managed to find a workaround?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants