Skip to content
This repository was archived by the owner on Jan 29, 2020. It is now read-only.

Fixed a situation where for an empty (no data) session, COOKIE was no… #1

Merged
merged 7 commits into from
Feb 28, 2018

Conversation

batumibiz
Copy link
Contributor

@batumibiz batumibiz commented Oct 23, 2017

The error is visible only on the empty session data, it simply does not install the required COOKIE, if they did not exist. If the session has some data, there is no error.

@weierophinney
Copy link
Member

We need a unit test demonstrating the scenario, and validating the fix, before we can merge this.

As it is, this patch causes an existing test to fail; please see https://travis-ci.org/zendframework/zend-expressive-session-ext/jobs/292188417#L615 for details.

@batumibiz
Copy link
Contributor Author

batumibiz commented Dec 14, 2017

I removed the testPersistSessionReturnsOriginalResposneIfSessionIsEmpty() method from the test.
Cause: changing the behavior of the PhpSessionPersistence::persistSession()

Reason is as follows:
If a visitor for the first time comes to your site, it is still no Session COOKIE.
And if there is no data in the session (and this can be the first time you visit), then COOKIE will not be installed, each time the page is updated in the browser, or if you switch to another page, a new session will be created. It turns out a lot of garbage (it's easy to see if you look at the session_save_path folder)

If I called session_start (), then the session should be keep regardless of whether there is any data in $_SESSION, or not.

@weierophinney
Copy link
Member

@batumibiz We also need a test demonstrating the scenario you describe, and validating the changes you are proposing enable that scenario.

@batumibiz
Copy link
Contributor Author

OK, we have a night now, tomorrow I'll do it.

@batumibiz
Copy link
Contributor Author

Done. Added tests with descriptions in PhpDoc

I deleted the testPersistSessionReturnsResponseWithSetCookieHeaderIfSessionHasContents(), because the behavior of the class does not change, regardless of whether there is data in the session, or not. Instead, he added a small test testPersistSessionIfSessionHasContents() that checks the data.

@geerteltink
Copy link
Member

The error is visible only on the empty session data, it simply does not install the required COOKIE, if they did not exist. If the session has some data, there is no error.

How is this an error? If there is no session data, should the cookie be set? AFAIK for zend-expressive-session it shouldn't matter if the cookie is sent or not. To be honest, I prefer to not have cookies sent if there is no session data.

@batumibiz
Copy link
Contributor Author

batumibiz commented Feb 24, 2018

OK, for demonstration, I have made a small example based on the latest versions of components zend-expressive-skeleton with zend-expressive-session and zend-expressive-session-ext.
That the problem was visible, I set session_save_path to a folder /data/session

How to see the problem?

  1. Clone or unpack into a web directory and through Composer install dependencies.
  2. Open browser at this address, make sure the default expressive page is visible
  3. Delete all cookies for this site from the browser
  4. Again, go to this address, refresh the page several times and watch the folder /data/session

You will see that after each page refresh, a new session file is created.

@RalfEggert
Copy link

RalfEggert commented Feb 28, 2018

@weierophinney @xtreamwayz

We just ran into this issue with a Zend\Expressive 3 RC1 application. This PR fixes the issue we have. It took us some hours to track this down. Please accept this PR!

Thanks, Ralf

@weierophinney weierophinney merged commit 3261d34 into zendframework:master Feb 28, 2018
weierophinney added a commit that referenced this pull request Feb 28, 2018
Fixed a situation where for an empty (no data) session, COOKIE was no…
weierophinney added a commit that referenced this pull request Feb 28, 2018
weierophinney added a commit that referenced this pull request Feb 28, 2018
@weierophinney
Copy link
Member

@batumibiz Thanks for the detailed example and fix!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants