Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

[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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Conversation

@alexander-schranz
Copy link
Contributor

@alexander-schranzalexander-schranz commentedMay 23, 2021
edited by Nyholm
Loading

QA
Branch?5.4
Bug fix?no
New feature?no
Deprecations?no
TicketsFix#42890
LicenseMIT
Doc PRsymfony/symfony-docs#...

Currently the session cookie is never set on therequest 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 likeRoadrunner thesession cookie is never set on theRequest object, as this kind of appserver never really outputs something and is running in thecli context.

Actually there is no way from outside to know if$session->save() was called or not because when the code inRoadrunner 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 theResponse 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 theAbstractSessionListener service viaResetInterface.

Nyholm, t-richard, chirimoya, and ging-dev reacted with thumbs up emojiad3n, chirimoya, Nyholm, and ging-dev reacted with heart emoji
@carsonbotcarsonbot added this to the4.4 milestoneMay 23, 2021
@alexander-schranzalexander-schranzforce-pushed thebugfix/session-cli-availibility branch 2 times, most recently from5d51c41 tobe9fc75CompareMay 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-schranzalexander-schranz changed the titleFix session cookie handling in cli context[HttpKernel] Fix session cookie handling in cli contextMay 27, 2021
@alexander-schranzalexander-schranzforce-pushed thebugfix/session-cli-availibility branch 6 times, most recently fromf5a49e5 to4cc89f7CompareJuly 7, 2021 22:53
@alexander-schranzalexander-schranzforce-pushed thebugfix/session-cli-availibility branch 2 times, most recently fromaac0ccb toab83e8aCompareJuly 7, 2021 23:27
@alexander-schranzalexander-schranz changed the title[HttpKernel] Fix session cookie handling in cli contextWIP: [HttpKernel] Fix session cookie handling in cli contextJul 7, 2021
@alexander-schranzalexander-schranz marked this pull request as draftJuly 8, 2021 00:24
@alexander-schranzalexander-schranzforce-pushed thebugfix/session-cli-availibility branch fromab83e8a tofbfc9cfCompareJuly 8, 2021 02:10
@alexander-schranzalexander-schranzforce-pushed thebugfix/session-cli-availibility branch fromfbfc9cf to647a4e0CompareJuly 8, 2021 02:26
Copy link
Member

@NyholmNyholm 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
ContributorAuthor

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

@NyholmNyholm marked this pull request as ready for reviewJuly 8, 2021 17:13
@carsonbotcarsonbot changed the titleWIP: [HttpKernel] Fix session cookie handling in cli context[HttpKernel] WIP: Fix session cookie handling in cli contextJul 8, 2021
Copy link
Member

@NyholmNyholm 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 pusheda small commit to fix the tests.

t-richard, alexander-schranz, chirimoya, and ging-dev reacted with heart emoji
$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 fromTestSessionListener toSessionListener

$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.

@fabpotfabpotforce-pushed thebugfix/session-cli-availibility branch from8bf60c5 tob3e4f66CompareSeptember 6, 2021 08:28
@fabpot
Copy link
Member

Thank you@alexander-schranz.

t-richard, ging-dev, chirimoya, and alexander-schranz reacted with hooray emoji

@Nyholm
Copy link
Member

Awesome.
Thank you all.

chirimoya, alexander-schranz, and ging-dev reacted with heart emoji

@alexander-schranzalexander-schranz deleted the bugfix/session-cli-availibility branchSeptember 6, 2021 08:52
@alexander-schranz
Copy link
ContributorAuthor

Nice to see this merged. Thank you all!

chirimoya and ging-dev reacted with heart emoji

@GrahamCampbell
Copy link
Contributor

🎉

This was referencedNov 5, 2021
Baldinof added a commit to Baldinof/roadrunner-bundle that referenced this pull requestFeb 8, 2022
Baldinof added a commit to Baldinof/roadrunner-bundle that referenced this pull requestFeb 8, 2022
Baldinof added a commit to Baldinof/roadrunner-bundle that referenced this pull requestFeb 28, 2022
nicolas-grekas added a commit that referenced this pull requestMar 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#41390Commits-------3e209c3 [HttpKernel] Fix advertizing deprecations for *TestSessionListener
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@nicolas-grekasnicolas-grekasnicolas-grekas left review comments

@NyholmNyholmNyholm left review comments

@fabpotfabpotfabpot approved these changes

@jderussejderussejderusse approved these changes

Assignees

No one assigned

Projects

None yet

Milestone

5.4

Development

Successfully merging this pull request may close these issues.

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

8 participants

@alexander-schranz@carsonbot@fabpot@Nyholm@GrahamCampbell@nicolas-grekas@jderusse@OskarStark

[8]ページ先頭

©2009-2025 Movatter.jp