Handle DB session callback custom fields before session closed

[lubosdz]

Attempt to fix #9438, #13740, #15037.
This fixes error Unkown: Failed to write session data (user).
Explanation of cause here.

Please review - perhaps somebody might get better idea how to populate fields before closing the session without corrupting the data integrity .. (e.g. if a session variable stored into session field changed between being populated and closing the session). Personally I am not happy with this commit, but so far haven't found better solution :-|

[samdark]

Doesn't seem to pass tests: https://travis-ci.org/yiisoft/yii2/jobs/501995713

[lubosdz]

This is really hard to fix without BC break.
The implementation is faulty since the very beginning.

Method MultiFieldSession::composeFields() attempts to do two things at the same time - populate custom fields in custom session handler after session is closed (inactive). This results into permanent errors when somebody tries to read some variable from session into custom field, typically user ID.

I tried 3 attempts to move preparation of custom fields, but there is always some unit test failing. I could update unit tests but it might break backward compatibility if somebody used this feature to set custom session content.

Any idea how to fix ? - either introduce minor BC break or leave faulty implementation ? :-)

Also - it there guide how to run locally all framework tests? Before pushing the change I would like to run specific tests locally.

EDIT: Found guide here

[samdark]

The tests could be executed with ./vendor/bin/phpunit. As the argument you may pass the name of the test class.

[samdark]

if somebody used this feature to set custom session content

That is possible.

[lubosdz]

Fail on codeclimate writeSession accesses the super-global variable $_SESSION - it's used in many methods.

Final fix works, but IMO modifying session data directly via callback should not be allowed, just like it's not possible for FileSession. Users mainly use custom writeCallback fields for tracking who's online or IDs-related stuff etc. It's kinda undocumented dirty feature specific only to DB session :-)

[samdark]

We may consider disallowing it for 3.0 (sessions will be handled differently there anyway).

[samdark]

Looks good except minor things.

[samdark]

Is it possible to add unit tests for it?

[lubosdz]

Unit tests for sessions are already there:
https://github.com/yiisoft/yii2/blob/master/tests/framework/web/session/AbstractDbSessionTest.php#L136
There is no new feature to test.
Or what should be added?

[samdark]

A test that failed before these changes and isn't failing with these changes.

[lubosdz]

There was no failing test before this fix.
Travis was failing, because my fix was not compatible with original implementation.
Now the fix is backwards compatible so no failing tests now - just like before the fix.

[lubosdz]

OK, I think it would make sense to add test for new method close() with user ID pulled out from session - to ensure custom fields do not interfere with inactive session anymore.

[lubosdz]

Whooops .. I did not intent to merge anything,
I just clicked Updated branch button .. and it merged.
Please check if it's OK or squash commits, whatever needed ..
sorry, I am not really familiar with github administration, I use gitlab or bitbucket.

[samdark]

It's fine to merge master into pull request branch. No issues with it.

[sergeymakinen]

LGTM. Hacky but should work.

[samdark]

Merged. Thank you!