Fix #104: php 7.2 handle ini_set() when session already started

[samsonasik]

Provide a narrative description of what you are trying to accomplish:

• Are you fixing a bug?
  • Detail how the bug is invoked currently.
    We can't call ini_set() when session already started
  • Detail the original, incorrect behavior.
    When it called, it got error : "ini_set(): A session is active. You cannot change the session module's ini settings at this time"
  • Detail the new, expected behavior.
    It call session_write_close() first, then start the session again
  • Base your feature on the master branch, and submit against that branch.
  • Add a regression test that demonstrates the bug, and proves the fix.
  • Add a CHANGELOG.md entry for the fix.

[weierophinney]

I'd rename this to $sessionRequiresRestart, as it better conveys what needs to happen. This is particularly important when you come to line 144, as calling session_start() because $sessionAlreadyStarted seems like a mistake when just looking at the verbiage.

[samsonasik]

I've updated variable to sessionRequiresRestart

[weierophinney]

Honestly, this seems really problematic, considering your original use case as demonstrated in #104. It means that you are starting, stopping, and restarting your session on every request, resulting in I/O even if no data is ever written or accessed. This may not seem like a big deal, but it can lead to longer response times (due to I/O of writing the session), and potential concurrency issues.

Wouldn't it be more performant and less prone to error to do as I suggested in that issue and inject the fully configured session manager into your container during instantiation? Have you tried the solution I posted on that issue?

[samsonasik]

Yes, I tried your solution, but that still got issue, for example, on call sessionmanager ->rememberMe() after login success that requires changes cookie lifetime, while session already active.

[samsonasik]

also when calling sessionmanager->forgetMe() as it set cookie_lifetime to 0, while session already started

[samsonasik]

@weierophinney I've updated code to check session_status() compare with PHP_SESSION_ACTIVE once only before ini_set() call as we already have sessionRequiresRestart variable set to true when it already active, so it reduce call.

[weierophinney]

@samsonasik One other thing that could make this better, particularly for the use case you presented in the original issue: check to see that the storage option provided differs from the one already set. If it isn't different, the method can bail early, without attempting the ini_set() operation at all. All that takes is an ini_get() call and a comparison.

[samsonasik]

@weierophinney do you mean that we need to add : ini_get() check before ini_set() at SessionConfig:: setStorageOption() ?

[weierophinney]

do you mean that we need to add : ini_get() check before ini_set() at SessionConfig:: setStorageOption() ?

Yes; it would bail early, before any attempts to determine if the session needs to be restarted, or setting the INI value, if the values are the same.

[samsonasik]

@weierophinney I've updated to have ini_get() check value first before ini_set()

[weierophinney]

Thanks, @samsonasik!