Skip to content

[HttpKernel] Add session cookie handling in cli context #41390

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

Merged

Conversation

alexander-schranz
Copy link
Contributor

@alexander-schranz alexander-schranz commented May 23, 2021

Q A
Branch? 5.4
Bug fix? no
New feature? no
Deprecations? no
Tickets Fix #42890
License MIT
Doc PR symfony/symfony-docs#...

Currently the session cookie is never set on the request object. In a normal webserver setup this is not needed as a session is in php-fpm or apache fastcgi written by php itself when the response is outputted in:

When use a runner like Roadrunner the session cookie is never set on the Request object, as this kind of appserver never really outputs something and is running in the cli context.

Actually there is no way from outside to know if $session->save() was called or not because when the code in Roadrunner executes the session is actually written and so closed here:

.

The best way so to fix this issue is that in CLI context symfony writes the session cookie on the Response object when the session is really saved.

This fixes issues which we have in the current roadrunner implementation of php runtime:

Beside Roadrunner there is also Swoole implementation which would require the same here: php-runtime/runtime#60

With this changes the Runners can be simplified a lot: https://github.com/php-runtime/runtime/pull/62/files#

TODO

Reset session variables after successfully response. The following code currently lives also in the runtime implementation but is requires also by all runners and so we should find a place where we put it:

                if (PHP_SESSION_ACTIVE === session_status()) {
                    session_abort();
                }
                // reset all session variables to initialize state
                $_SESSION = [];
session_id(''); // in this case session_start() will generate us a new session_id()

EDIT: Added this now to the AbstractSessionListener service via ResetInterface.

@carsonbot carsonbot added this to the 4.4 milestone May 23, 2021
@alexander-schranz alexander-schranz force-pushed the bugfix/session-cli-availibility branch 2 times, most recently from 5d51c41 to be9fc75 Compare May 23, 2021 19:24
@carsonbot
Copy link

Hey!

I think @Simperfit has recently worked with this code. Maybe they can help review this?

Cheers!

Carsonbot

@alexander-schranz alexander-schranz changed the title Fix session cookie handling in cli context [HttpKernel] Fix session cookie handling in cli context May 27, 2021
@alexander-schranz alexander-schranz force-pushed the bugfix/session-cli-availibility branch 6 times, most recently from f5a49e5 to 4cc89f7 Compare July 7, 2021 22:53
@alexander-schranz alexander-schranz force-pushed the bugfix/session-cli-availibility branch 2 times, most recently from aac0ccb to ab83e8a Compare July 7, 2021 23:27
@alexander-schranz alexander-schranz changed the title [HttpKernel] Fix session cookie handling in cli context WIP: [HttpKernel] Fix session cookie handling in cli context Jul 7, 2021
@alexander-schranz alexander-schranz marked this pull request as draft July 8, 2021 00:24
@alexander-schranz alexander-schranz force-pushed the bugfix/session-cli-availibility branch from ab83e8a to fbfc9cf Compare July 8, 2021 02:10
@alexander-schranz alexander-schranz force-pushed the bugfix/session-cli-availibility branch from fbfc9cf to 647a4e0 Compare July 8, 2021 02:26
Copy link
Member

@Nyholm Nyholm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you.

I like this. Is it ready for a review (ie, not draft)?
If so, I would be happy if @jderusse gave his opinion on this solution.

@alexander-schranz
Copy link
Contributor Author

@Nyholm Yes the PR itself is ready for review, I want still provide some tests for the Listener logic.

@Nyholm Nyholm marked this pull request as ready for review July 8, 2021 17:13
@carsonbot carsonbot changed the title WIP: [HttpKernel] Fix session cookie handling in cli context [HttpKernel] WIP: Fix session cookie handling in cli context Jul 8, 2021
Copy link
Member

@Nyholm Nyholm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I hope you don't mind that I pushed a small commit to fix the tests.

@@ -325,7 +325,7 @@ public function load(array $configs, ContainerBuilder $container)
$this->sessionConfigEnabled = true;
$this->registerSessionConfiguration($config['session'], $container, $loader);
if (!empty($config['test'])) {
$container->getDefinition('test.session.listener')->setArgument(1, '%session.storage.options%');
$container->getDefinition('test.session.listener')->setArgument(2, '%session.storage.options%');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is because we switched from TestSessionListener to SessionListener

@@ -142,7 +142,7 @@ public function onKernelResponse(ResponseEvent $event)
$sessionId = $session->getId();
$sessionCookiePath = $this->sessionOptions['cookie_path'] ?? '/';
$sessionCookieDomain = $this->sessionOptions['cookie_domain'] ?? null;
$sessionCookieSecure = $this->sessionOptions['cookie_secure'] ?? null;
$sessionCookieSecure = $this->sessionOptions['cookie_secure'] ?? false;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@fabpot fabpot force-pushed the bugfix/session-cli-availibility branch from 8bf60c5 to b3e4f66 Compare September 6, 2021 08:28
@fabpot
Copy link
Member

fabpot commented Sep 6, 2021

Thank you @alexander-schranz.

@Nyholm
Copy link
Member

Nyholm commented Sep 6, 2021

Awesome.
Thank you all.

@alexander-schranz alexander-schranz deleted the bugfix/session-cli-availibility branch September 6, 2021 08:52
@alexander-schranz
Copy link
Contributor Author

Nice to see this merged. Thank you all!

@GrahamCampbell
Copy link
Contributor

🎉

This was referenced Nov 5, 2021
Baldinof added a commit to Baldinof/roadrunner-bundle that referenced this pull request Feb 8, 2022
Baldinof added a commit to Baldinof/roadrunner-bundle that referenced this pull request Feb 8, 2022
Baldinof added a commit to Baldinof/roadrunner-bundle that referenced this pull request Feb 28, 2022
nicolas-grekas added a commit that referenced this pull request Mar 7, 2022
…onListener (nicolas-grekas)

This PR was merged into the 5.4 branch.

Discussion
----------

[HttpKernel] Fix advertizing deprecations for *TestSessionListener

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

Looks like we missed that during the review of #41390

Commits
-------

3e209c3 [HttpKernel] Fix advertizing deprecations for *TestSessionListener
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Sessions are not saved when using runtime + bref/swoole/roadrunner
8 participants