inhibit creation of new session files for cookie-less requests

[pine3ree]

No description provided.

[pine3ree]

Hello @weierophinney, @batumibiz

I provided a fix (co the code for testing from here) for #33

This stills works differently from php session_start() but it may be even more efficient

DETAILS

• initializeSessionFromRequest now only starts a new php-session if a session cookie was provided. The default php behaviour is to start a session anyway if session_start() is called. But we do not need to open an empty file to read data from it (no data in there). We open it (or create a new one if not found) only if instructed by the request.
• persistSession:
  1. If the session was instructed to be regenerated we close the previous session file and open a new file with a fresh id.
  2. otherwise, if any data changed we need to persist it to a file so if no session file was started during initialization we generate an id and start a session with it to create the file.
• I removed a test case that worked under the assumption that a php-session is alway started and added new test cases

TODO

• we need to stop sending a set-cookie if the request cookie value matches the last session_id() value
• make sure that if session_start() is called at least once the cache headers are added even if no set-cookie is sent

[pine3ree]

Removed the new PHP_SESSION_ACTIVE test that decreased coverage: the php-ext-session C-code does that itself in php_session_flush (php 7.1, ref1, ref2) and again in PHP_FUNCTION(session_write_close) (php 7.2, ref) even before calling php_session_flush()

[batumibiz]

Dear @pine3ree
You have an interesting idea, but I can't agree with it and explain why...
(this is my vision, although I can be wrong)

Call SessionPersistence implies that we in any case You must start the session. If we do not, there might be compatibility issues with other scripts, which I probably will use in my application (scripts expect that the session has). If no COOKIE for this session, they need to create.

In the original package the problem is in this line
There is now the following code:

return new Session($_SESSION, $sessionId);

And it is necessary that was so:

return new Session($_SESSION, $id);

and the problem will be simply solved :)
The mistake is that instead of $id, which we checked and generated above, using $sessionId, which is taken directly from the COOKIE and for which a new session may not exist.

[batumibiz]

I corrected that issue here

[pine3ree]

Dear @batumibiz ,

yes that was my first solution (see one of the first comments in #33 (comment)). Having session_id() and $session->id synched is also less confusing (this can also be achieved in my implementation memoizing the request session cookie value in a property) Also php starts a new session as soon as session_start() is called.

But then I found out that we could just avoid creating a new session file when there is no access to any session data and no session file (id) targeted by the request (no request cookie).

If a request cookie is present, a new session will be started anyway (thus opening an old session file or creating a new one) because we need to retrieve persisted session data (if any) and use it to initialize the Session instance.

kind regards

[batumibiz]

yes that was my first solution (see one of the first comments in #33 (comment)).

I think so right

[pine3ree]

Hello @batumibiz,

there is also another thing to consider in https://github.com/zendframework/zend-expressive-session-ext/blob/master/src/PhpSessionPersistence.php#L106:

the test '' === $id //- the id is empty, but the data has changed (new session) is used to verify that we started with a request that does not carry any session id, but we changed the session data (=> so we should start a session, which is done by the regenerateSession() call). So the (proxied) Session instance should be initialized with the request session id, even if empty.
kind regards

[batumibiz]

Yes I agree.
If you do, as I suggest, and as you suggested, then there can be no session without an ID.
Therefore, this piece of code (which checks ID) will never be used and can be deleted.

[samsonasik]

should can use $this->assertCount():

Suggested change

	$this->assertSame(1, count($files));
	$this->assertCount(1, $files);

[samsonasik]

should can use $this->assertCount():

Suggested change

	$this->assertSame(0, count($files));
	$this->assertCount(0, $files);

[pine3ree]

Thanks @samsonasik! Will add after #38 is pulled. Otherwise phpunit failures could randomly happen. regards!

[weierophinney]

@pine3ree #38 is in both master and develop now.

[pine3ree]

@samsonasik using assertCount here. kind regards.

[batumibiz]

Hello again everyone!
I've done some tests on a prototype of a real application, thought it over and am currently leaning towards the solution that @pine3ree suggested in this PR. Now will explain its opinion:

Compatibility with ordinary PHP sessions is not needed and they are used only as a data storage mechanism. I don't think anyone using Expressive will use simple native sessions from PHP at the same time. Therefore, it is logical to do as @pine3ree suggested, that is, for new sessions that do not have COOKIES and no data, do not run session_start () at all.

If no one has any objections, then please cancel my PR

[weierophinney]

Thanks, @pine3ree! Cherry-picked to master, as I consider this a bugfix, and releasing immediately as version 1.5.1.