destroy session on logout instead of restarting it

[xini]

IMHO there is no reason to keep the session around after logout? When you visit a public page the session is not started either, so there is no reason to keep a session around after logout.

[maxime-rainville]

Looking at Session, it looks like restart will call destroy.

silverstripe-framework/src/Control/Session.php

Lines 246 to 250 in ae61be3

	public function restart(HTTPRequest $request)
	{
	$this->destroy();
	$this->start($request);
	}

And Session::init() looks like it wants to start a new session if one doesn't already exists.

silverstripe-framework/src/Control/Session.php

Lines 226 to 239 in ae61be3

	public function init(HTTPRequest $request)
	{
	if (!$this->isStarted() && $this->requestContainsSessionId($request)) {
	$this->start($request);
	}
	// Funny business detected!
	if (self::config()->get('strict_user_agent_check') && isset($this->data['HTTP_USER_AGENT'])) {
	if ($this->data['HTTP_USER_AGENT'] !== $this->userAgent($request)) {
	$this->clearAll();
	$this->restart($request);
	}
	}
	}

I suspect that in 99% of logouts, you'll be redirected to some page that will want to start a new session.

[xini]

Hi @maxime-rainville,

Thanks for your reply.

Looking at Session, it looks like restart will call destroy.

Yes, restart does call destroy, but it also restarts a new session. For a normal page without a form a session is not needed. When you visit a default Silverstripe page, a session is not started. Hence we should destroy the session after logout. No?

And Session::init() looks like it wants to start a new session if one doesn't already exists.

Correct, but we don't re-init the session after logout.

I suspect that in 99% of logouts, you'll be redirected to some page that will want to start a new session.

That might be the case, but that really is up to the developer building the site. If the target page needs a session, it can/will create it. But we don't need to force it. right?

[dhensby]

Whilst I agree with @maxime-rainville that most pages will probably start a session anyway, there's no point forcing the session to be started unless it's really needed. So I think this change is sensible.

I think it should go into 4 branch, though. Not 4.8.

[xini]

@dhensby I have rebased this. Thanks.

[dhensby]

Apparently there are some linting issues 😕