Skip to content

Fix session in long-running apps#61

Open
viktorprogger wants to merge 3 commits into
masterfrom
long-run-fix
Open

Fix session in long-running apps#61
viktorprogger wants to merge 3 commits into
masterfrom
long-run-fix

Conversation

@viktorprogger
Copy link
Copy Markdown
Contributor

@viktorprogger viktorprogger commented May 20, 2024

Q A
Is bugfix? ✔️
New feature?
Breaks BC?
Fixed issues

Fixes a bug for long-running applications: session is not resetted between requests. Session ids are UTF-safe, as this may be critical for some session handlers.

@viktorprogger viktorprogger added the status:code review The pull request needs review. label May 20, 2024
Comment thread src/Session.php Outdated
Comment thread src/Session.php
if ($this->sessionId !== null) {
session_id($this->sessionId);
} else {
session_id(session_create_id());
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

What about collision check? session_create_id don't check collision if called before start session.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Any idea?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I don't know. In symfony it's not check. May be to make custom session ID creating optional? Use boolean flag or callback or create SessionIdGeneratorInterface?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I think this would be very non-intuitive for users

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think this would be very non-intuitive for users

Then we must find solutions without collision risk. Otherwise we fix bug for longliving apps, but add new problem with collisions.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

What about this?

session_start($this->options);
$this->sessionId = session_create_id();
session_write_close();
session_id($this->sessionId);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

What about this?

session_start($this->options);
$this->sessionId = session_create_id();
session_write_close();
session_id($this->sessionId);

This will entail additional costs on read/write session data.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Extra opening a session doesn't sound good to me.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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

Labels

status:code review The pull request needs review.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants