Skip to content

Conversation

alexander-schranz
Copy link
Member

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

Fix parts of #46

The session still have the following problems::

  • Invalid session cookie maybe returned when session is not written when nothing was changed
  • Session cookie not deleted when not longer valid

if ($hasRequestSession) {
$session = $sfRequest->getSession();

$sessionId = \session_id();
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are we using PHP's built-in sessions? In particular, these won't work across multiple servers.

Copy link
Member Author

@alexander-schranz alexander-schranz May 17, 2021

Choose a reason for hiding this comment

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

Symfony uses this also when used Redis or other Session handlers all are calling that method to get the session_id thats why I call it here. I was afraid accessing again the session a new session start is trigger so I did directly access the session_id like here:

https://github.com/symfony/symfony/blob/bbb0a694fbbd5a527b2236636271a47122b5d47c/src/Symfony/Component/HttpFoundation/Session/Storage/Proxy/AbstractProxy.php#L78

Copy link
Member Author

Choose a reason for hiding this comment

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

But there are still open problems, so this is still WIP. Have a look at #46 for which things are currently not resolved.

Copy link
Member Author

Choose a reason for hiding this comment

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

As described in the issue alternative solution would be which I personally would prefer that maybe symfony add an option to write the cookie at: https://github.com/symfony/symfony/blob/bbb0a694fbbd5a527b2236636271a47122b5d47c/src/Symfony/Component/HttpKernel/EventListener/AbstractSessionListener.php#L110 to $request object this would make less problems for us.

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.

Feel free to create a new private method or two

@alexander-schranz alexander-schranz changed the title WIP: Add session handling to roadrunner symfony bridge Add session handling to roadrunner symfony bridge May 18, 2021
@alexander-schranz alexander-schranz force-pushed the feature/add-session-handling branch 2 times, most recently from 2a9c0fd to 334720c Compare May 18, 2021 08:01
@alexander-schranz alexander-schranz force-pushed the feature/add-session-handling branch from 334720c to 92a38a8 Compare May 18, 2021 08:03
@alexander-schranz alexander-schranz marked this pull request as ready for review May 18, 2021 08:08
@Nyholm Nyholm merged commit cb21762 into php-runtime:main May 18, 2021
@Nyholm
Copy link
Member

Nyholm commented May 18, 2021

Thank you.

Let's merge this and handle the other issues in a separate PR.

@alexander-schranz alexander-schranz deleted the feature/add-session-handling branch May 18, 2021 08:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants