Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

#

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Jump to bottom

Fix for Warning: ini_set(): A session is active... #229

Merged
merged 1 commit into from
Jun 8, 2020
Merged

Fix for Warning: ini_set(): A session is active... #229

merged 1 commit into from
Jun 8, 2020

Conversation

spirit-q2
Copy link

@spirit-q2 spirit-q2 commented May 6, 2020 •
edited
Loading

Addresses the issue: #183

Changing session settings shouldn't be done on sfBasicSecurityUser class. Instead it should be done near (and before) the call to session_start() in sfSessionStorage

@spirit-q2
Copy link
Author

https://travis-ci.org/github/FriendsOfSymfony1/symfony1/jobs/683856543 doesn't look like a related error 🤔

  [Composer\Downloader\TransportException]  
  Peer fingerprint did not match

The command "composer install" failed and exited with 1 during .

@spirit-q2 spirit-q2 force-pushed the fix-session-warning-on-php72-and-above branch from 139290c to 0f6bdd8 Compare May 6, 2020 14:53
@spirit-q2
Copy link
Author

@alquerci hello, have some time to take a look? ;)

alquerci
alquerci reviewed May 6, 2020
View reviewed changes
Copy link
Contributor

@alquerci alquerci left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you @spirit-q2 to take care of that.

Need to check deeper but on a first approximation there can be a BC break regarding session timeout.

Expected to be configured on factories.user.param.timeout but not on factories.storage.param.timeout where both configuration needs to be in synchronization.

storage one > than user one

Edit: Also need to propagate this new factory parameter to all other session classes, if applicable.
Edit2: gc_maxlifetime is maybe a better name for this new parameter.

@spirit-q2 spirit-q2 force-pushed the fix-session-warning-on-php72-and-above branch 2 times, most recently from 28eb44c to 36a7cf5 Compare May 7, 2020 15:46
@spirit-q2
Copy link
Author

@alquerci please, check latest version ;)

alquerci
alquerci suggested changes May 7, 2020
View reviewed changes
Copy link
Contributor

@alquerci alquerci left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Going better without BC breaks but just left some refactoring things to do.

And for sure to be sure that current test already covers this.
Because of currently tests does not cover this warning.
see #227 (comment)

👌

lib/storage/sfSessionStorage.class.php Outdated
if (!array_key_exists('gc_maxlifetime', $this->options))
{
$this->options['gc_maxlifetime'] = 1800;
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This configuration check can be merged on default parameters values used on the first parameter of array_merge(), see on line 56.

lib/config/sfFactoryConfigHandler.class.php Outdated
@@ -109,6 +109,12 @@ public function execute($configFiles)
unset($parameters['database']);
}

$sessionGcMaxlifetime = 1800;
if (isset($config['user']['timeout'])) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The timeout parameter is located on $config['user']['param']['timeout'].

lib/config/sfFactoryConfigHandler.class.php Outdated
if (isset($config['user']['timeout'])) {
$sessionGcMaxlifetime = $config['user']['timeout'];
}
$defaultParameters[] = sprintf("'gc_maxlifetime' => %d,", $sessionGcMaxlifetime);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only append this new configuration if it is set on user factory configuration.

Let keep the default configuration value on sfSessionStorage class to avoid duplication and confusion.

Fix for `Warning: ini_set(): A session is active. You cannot change t…
4d512de 
…he session module's ini settings at this time`
@spirit-q2 spirit-q2 force-pushed the fix-session-warning-on-php72-and-above branch from 36a7cf5 to 4d512de Compare May 9, 2020 17:28
@spirit-q2
Copy link
Author

@alquerci updated all comments

@spirit-q2
Copy link
Author

@j0k3r @jeromemacias @e1himself @GromNaN Hello guys! Can anyone check/approve/merge?

mentalstring
mentalstring approved these changes May 11, 2020
View reviewed changes
Copy link
Contributor

@mentalstring mentalstring left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@ncharlot
Copy link

Interested by this fix too.

@spirit-q2
Copy link
Author

@j0k3r @jeromemacias @e1himself @GromNaN

Guys, any chances to get this merged?
and these PRs as well: #230, #228

It's the only blocker to move whole project to php7.4 for me (and I guess for many other people who are willing to move to new php)

@alquerci
Copy link
Contributor

alquerci commented Jun 5, 2020

cc @thePanz ☝️

@j0k3r
Copy link
Contributor

j0k3r commented Jun 6, 2020 •
edited
Loading

I’ll take care of them next week

@j0k3r j0k3r merged commit f8ee65d into FriendsOfSymfony1:master Jun 8, 2020
@j0k3r j0k3r linked an issue Jun 8, 2020 that may be closed by this pull request
Warning: ini_set(): A session is active. You cannot change the session module's ini settings at this time in symfony1\lib\user\sfBasicSecurityUser.class.php on line 257 #183
Closed
@thePanz thePanz self-requested a review June 8, 2020 07:07
@spirit-q2 spirit-q2 deleted the fix-session-warning-on-php72-and-above branch June 8, 2020 14:17
# for free to join this conversation on GitHub. Already have an account? # to comment
Reviewers

@mentalstring mentalstring mentalstring approved these changes

@j0k3r j0k3r j0k3r approved these changes

@alquerci alquerci alquerci approved these changes

@thePanz thePanz Awaiting requested review from thePanz

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
5 participants
@spirit-q2 @ncharlot @alquerci @j0k3r @mentalstring