session_set_cookie_params(): Cannot change session cookie parameters when session is active
babeuloula opened this issue · 23 comments
Hello,
I have this error with PHP7.2 session_set_cookie_params(): Cannot change session cookie parameters when session is active in this file /slim-session/src/Slim/Middleware/Session.php:95
Someone can help me ?
Thanks
It's only a warning, which was always present in older PHP versions but kept silent.
Mind working on a PR for this? Only thing needed should be to check if a session is running before calling session_set_cookie_params and session_name.
Maybe it's a warning but with Tracy I have an error.
To solve this I put session_write_close(); before session_set_cookie_params();. It's not a good fix but it's works
Please push this to master branch. This fix is very useful.
Edit: Just read your comment on the commit, would be helpful if someone could push a fix for this. If not I'll most likely push one because I can't stand these errors!
I'm willing to accept it if you send it as PR, and it doesn't break anything.
Wrote a patch for this, please look over and accept if all good!
Thank You
Rewrote patch, can you approve?
Hello! Can you aprove the patch?
Thank´s!
Experiencing this issue plus a few other warnings. Specifically:
Warning: ini_set(): A session is active. You cannot change the session module's ini settings at this time
Warning: session_set_cookie_params(): Cannot change session cookie parameters when session is active
Warning: session_name(): Cannot change session name when session is active
Warning: session_cache_limiter(): Cannot change cache limiter when session is active.
About to submit a patch that should resolve these. I'll do some basic testing tomorrow alongside my current work, and comment on it if I have any issues.
Did you read the above conversation? I already submitted a working patch for it. Your patch won't pass because the creator said " Only thing needed should be to check if a session is running before calling session_set_cookie_params and session_name."
My Patch: 0b4154b
This is all that is needed.
I did read the above conversation. What confused me was that the proposed patch may still throw warnings for ini_set and session_cache_limiter, since those two functions would still be called on an active session.
My proposed patch essentially just prevents the session from even trying to start if there is already an active session. It also prevents any call to ini_set if there is already an active session. Ultimately, nothing happens if there is already an active session. I think this change makes sense, but I don't know what I don't know. If I am mistaken, feel free to reject my pull request.
Sorry for the late reply, I had no laptop available to work on.
In a few days I'll be testing all of your patches and warnings, also reading the official PHP docs on session management.
For now, you both can work on code cleanup and fixing. I left a comment in each PR regarding which things can be improved for now until I test the changes and make sure it doesn't break anything.
Regards.
No worries c:
Changes have been made, and I probably should read through the PHP docs on session management as well. Sessions have been working just fine on my server, which leads me to wonder whether session.auto_start is set. Something to figure out tomorrow.
It would be helpful if someone can share his environment information from current PHP, because even using Tracy as @babeuloula pointed out I don't get an error nor a warning.
I just tested the page given in the README, using PHP 7.2.7 (with PHP development server), on Manjaro, also using autorefresh and all that stuff.
Hello @bryanjhv, there is my env :
// index.php
require_once '../vendor/autoload.php';
// Activation de la debugbar Tracy
Debugger::enable(Debugger::DEVELOPMENT);
Debugger::$strictMode = true;
Debugger::$maxDepth = 15;
Debugger::$maxLength = 250;
RunTracy\Helpers\Profiler\Profiler::enable();
\RunTracy\Helpers\Profiler\Profiler::start('initApp');
App::app();
include_once '../app/Router.php';
App::app()->run();
RunTracy\Helpers\Profiler\Profiler::finish('initApp');// tracy settings
$config = ['settings' => array(
'determineRouteBeforeAppMiddleware' => true,
'displayErrorDetails' => !$prod,
// Params de Tracy
'tracy' => array(
'showPhpInfoPanel' => 0,
'showSlimRouterPanel' => 0,
'showSlimEnvironmentPanel' => 0,
'showSlimRequestPanel' => 0,
'showSlimResponsePanel' => 0,
'showSlimContainer' => 0,
'showEloquentORMPanel' => 0,
'showTwigPanel' => 0,
'showIdiormPanel' => 1,// > 0 mean you enable logging
// but show or not panel you decide in browser in panel selector
'showDoctrineDBALPanel' => 1,
'showProfilerPanel' => 0,
'showVendorVersionsPanel' => 0,
'showXDebugHelper' => 1,
'showIncludedFiles' => 0,
'showConsolePanel' => 0,
'configs' => array(
// XDebugger IDE key
'XDebugHelperIDEKey' => 'PHPSTORM',
// Disable login (don't ask for credentials, be careful) values( 1 || 0 )
'ConsoleNoLogin' => 0,
// Multi-user credentials values( ['user1' => 'password1', 'user2' => 'password2'] )
'ConsoleAccounts' => array(
'dev' => '34c6fceca75e456f25e7e99531e2425c6c1de443'// = sha1('dev')
),
// Password hash algorithm (password must be hashed) values('md5', 'sha256' ...)
'ConsoleHashAlgorithm' => 'sha1',
// Home directory (multi-user mode supported) values ( var || array )
// '' || '/tmp' || ['user1' => '/home/user1', 'user2' => '/home/user2']
//'ConsoleHomeDirectory' => DIR,
'ConsoleHomeDirectory' => '',
// terminal.js full URI
'ConsoleTerminalJs' => '/assets/js/jquery.terminal.min.js',
// terminal.css full URI
'ConsoleTerminalCss' => '/assets/css/jquery.terminal.min.css',
'ProfilerPanel' => array(
// Memory usage 'primaryValue' set as Profiler::enable() or Profiler::enable(1)
// 'primaryValue' => 'effective', // or 'absolute'
'show' => array(
'memoryUsageChart' => 1, // or false
'shortProfiles' => true, // or false
'timeLines' => true // or false
)
)
)
)
)];// composer.json
"runcmf/runtracy": "^0.2.7",
"yep/tracy-twig-extensions": "^1.1",My server :
- Ubuntu server 16.04
- PHP 7.2.7
- MySQL
- Apache
Do you need more information ?
I can safely say that I feel pretty silly right now.
When I set up my project I used the slimphp/slim-skeleton. Let me show you what its index.php looks like really quick:
<?php
if (PHP_SAPI == 'cli-server') {
// To help the built-in PHP dev server, check if the request was actually for
// something which should probably be served as a static file
$url = parse_url($_SERVER['REQUEST_URI']);
$file = __DIR__ . $url['path'];
if (is_file($file)) {
return false;
}
}
require __DIR__ . '/../vendor/autoload.php';
session_start();
// Instantiate the app
$settings = require __DIR__ . '/../src/settings.php';
$app = new \Slim\App($settings);
// Set up dependencies
require __DIR__ . '/../src/dependencies.php';
// Register middleware
require __DIR__ . '/../src/middleware.php';
// Register routes
require __DIR__ . '/../src/routes/index.php';
// Run app
$app->run();See the issue?
It calls session_start() before loading the middleware, including your slim-session package. Thus, since the session has already started, all the initialization and configuration in your package throws the warnings I've been seeing.
Sorry for wasting your time xP
I will definitely be putting more time into debugging before submitting an issue in the future. Now that I've discovered Tracy, that should be easier for me.
Nice catch! It was not a waste of time, I think. I'd like to know if other participants also had session_start added somewhere, and ask them to remove that line and see if the problem persists.
PS: For me it didn't generate any warning, neither with Tracy or PHP built-in server, and I enabled display_errors=On and error_reporting=E_ALL so if I don't get those warnings it might be a session misconfiguration in Debian/Ubuntu (which happen to be the most problematic systems). I'd like to see phpinfo's session configuration to setup my system like yours and check the PRs accordingly.
PS 2: If calling session_start twice was the problem, then a message can be added to README telling you don't need to start the session manually or do any session_* call in their code, just use the middleware and helper.
Thanks for all of your support and feedback.
Here are the specs of the server:
- Windows Server 2012 R2
- IIS 8.5
- PHP 7.2.8 (I had updated from 7.2.3 during debugging but the errors still showed)
Here's the session config that I got from phpinfo:
session
Session Support: enabled
Registered save handlers: files user
Registered serializer handlers: php_serialize php php_binary wddx
session.auto_start Off
session.cache_expire 180
session.cache_limiter nocache
session.cookie_domain no value
session.cookie_httponly no value no value
session.cookie_lifetime 0
session.cookie_path /
session.cookie_secure 0
session.gc_divisor 1000
session.gc_maxlifetime 1440
session.gc_probability 1
session.lazy_write On
session.name PHPSESSID
session.referer_check no value
session.save_handler files
session.save_path C:/Windows/temp
session.serialize_handler php
session.sid_bits_per_character 5
session.sid_length 26
session.upload_progress.cleanup On
session.upload_progress.enabled On
session.upload_progress.freq 1%
session.upload_progress.min_freq 1
session.upload_progress.name PHP_SESSION_UPLOAD_PROGRESS
session.upload_progress.prefix upload_progress_
session.use_cookies 1
session.use_only_cookies 1
session.use_strict_mode 1
session.use_trans_sid 0
If more config info is desired I can post my php.ini when I'm back at work on Monday.
AFAIK those settings are OK, and shouldn't interfere with this package in any way. Anyway I was thinking in looking carefully at the PHP docs and if they explicitly state that a session must be not active before calling the needed methods, then I should add the checks and see what happens (I mean, if it breaks other users). If that is not stated, then the affected users should fix their PHP configuration or system accordingly (anyway, who runs Slim on a shared hosting uh?).
For all facing this issue, you can try version 3.6.0 now, it should be what you needed. All the calls needed to be done before calling session_start so it is wrapped now.
Reading through the docs myself, from my understanding session_start has to be called on every single request.
ini_set changes the value of the configuration option only "during the script's execution, and will be restored [to its original setting] at the script's ending". This implies that for a given value to persist across requests, it must be called on every request (not explicitly stated though).
session_set_cookie_params, session_name, session_cache_limiter all explicitly state that they must be called for every request and before session_start is called. As justification for this, session_set_cookie_params says that the change only lasts for the duration of the script, and the other two state that it is reset to the default value at request startup time. These two justifications sound, essentially, the same. I wouldn't be surprised if their implementations used ini_set, under the hood.
If I had to guess, the reason why @babeuloula 's fix (back in February, to add session_write_close before session_set_cookie_params) works is because the session is started for some reason (some other code starts it before this package is loaded), and he ends up closing that so that your middleware restarts it.
It feels like a session's lifetime only lasts for the duration of the request, and automatically ends with the request, just to be started again with the next request. Ending just leaves a file/session data behind, though. session_destroy destroys the file.
Which makes me wonder, what exactly are the $inactive checks for? I mean, they check to make sure that the session hasn't already been started, but that shouldn't happen with just the code from this middleware. If some other module calls session_start before this middleware is loaded then what's the point of even using it? (The handler is the point I guess. It's a nice handler.)
The strangest thing was that there was an $inactive check before starting the session in the first place! That didn't make any sense to me. It kinda defeats the purpose of all the configuration! So I went through the git history and found #8 . The issue that person had was the message "A session had already been started - ignoring session_start()". If the session had already been started, though, doesn't that defeat the purpose of set_cookie_params and all the other calls?
Oh man this ended up being a bit of a ramble. The point I wanted to make was that the $inactive check prevents any of the configuration from happening. Amounting to only really using this middleware for the Session Handler. Thus, for those who actually use this package as intended (without any extra session_start()) then everything works smoothly. For those who don't, things will fail silently now. Which might just be all we need.
OK, I think you just got it right. PHP needs a call to session_start on each request to be able to work with session data (reading and writing, cleaning, etc), and Slim just provides middlewares for that purpose: tasks to be run before or after the request is processed or 'answered', that's why __invoke first starts the session and then processes the request. PHP keeps session data under two circumstances: the GC didn't got a cycle yet (which is why initially this package was full of ini_set calls making GC more tolerant but newer PHP comes preconfigured to have a poor probability to run it and also disk space shouldn't be a problem because session data is saved in serialized form), or if the session cookie is still alive (which is why we need to setcookie a lot on each request to a newer expire time so autorefresh works, otherwise PHP will just clear out the session and start another without advice and no matter if the server got a new request during the lifetime). I rejected the session_write_close call because, if you look at the code, it's meant to destroy the entire session and forcefully write data to the handler (by calling $handler->write), which is what you don't want to do on each request (believe me, or call it each time and you'll see lots of session files).
If you're afraid of the configuration being ignored, just install my package, and change in vendor the Session class by adding a var_dump before each call, you'll see that all of the functions after the check run, unless, as you said, other package didn't call session_start before in your middleware stack. Here you have a quick and clean solution available: call add first for this package and later for the other ones that want to mess the session, it's not that hard to fix that if you think about it. Maybe a good choice here would be to document it in the README stating clearly that this package should go before any other that manages sessions (I thought it was clear before because I didn't put // Other middleware comment before the call to add.
The other thing is, as you might have figured out, that this package allows you to use your custom session handler, it doesn't lock you to session files as other packages do. You can write your own handler or integrate another one (illuminate/session for example includes its handlers standalone) without having lots of package starting sessions randomly so breaking your app. You want a database, a Redis store, a memory store (PS: it's PHP nature haha), a do what you want store? Write a handlerfor it and this package will use it as you tell it.
If we get rationale, PHP emits those warnings for something: for you to be careful of only calling those functions once in your entire app (i.e. request). If you try to do it twice, it will cry with their stupid warnings. And yes, all of we know sessions are a sensitive piece of shit we should take seriously and carefully, so we can't just go making salad with user's data (some people stores its entire profile data in a session). Maybe it sounds I'm trying to impose my package over others, but it's not like that, first because it says "simple" in the description, and second because there are too few alternatives to make it right (illuminate/session, I'm looking at you again).
And: nope, there's no ini_set under the hood:
https://github.com/php/php-src/blob/php-7.2.7/ext/session/session.c#L1662-L1733