Check if there is a valid session before messing with session ini values by PhilETaylor · Pull Request #15742 · joomla/joomla-cms

PhilETaylor · 2017-05-02T14:46:49Z

Closes #15680
Closes #12153
Check if there is a valid session before messing with session ini values
(NOT TESTED, coded on sight only - please test!)

Closes #15680 Check if there is a valid session before messing with session ini values (NOT TESTED, coded on sight only - please test!)

mbabker · 2017-05-02T14:54:21Z

libraries/joomla/session/handler/joomla.php

 	{
-		// Disable transparent sid support
-		ini_set('session.use_trans_sid', '0');
+		if (session_id() === "")


If we really have to do this we should be checking either $this->isStarted() or $this->getId(). Although I really feel like this is just a edge case workaround and not something that should exist in production code (I'm not aware of any other session API that has this level of checks).

mbabker · 2017-05-02T15:01:45Z

This is nothing more than a wrapper to session_id() anyway :)

Better to use the abstractions than hardcode stuff 😉

You have my opinion on the matter, whether others agree or not is to be determined. Honestly, session packages are one part of the PHP ecosystem that lack interoperability and part of it does boil down to the internal uses of ini_set() (at least in the cases of our own Session APIs, Symfony, and Aura; don't have the time at the moment to look for other packages and review those).

csthomas · 2017-05-02T20:30:02Z

~~IMO this place would be better for such ini_set() https://github.com/joomla/joomla-cms/blob/staging/libraries/joomla/session/session.php#L132~~

mbabker · 2017-05-02T20:34:27Z

Not really, the intent with the abstraction layers is to make it so that configuration changes that don't have to be made for an environment aren't. If you ran a custom app off the CMS app stack using JSession you could use a different JSessionHandlerInterface implementation that either doesn't need that ini_set() call or makes different ones altogether.

JSession (and in 4.0 the Framework's Joomla\Session\Session) are really just the interface for folks to read/write data from the session store. The abstraction layers underneath it (storage and handler) are the internal details where these configs start getting made. So JSession should rightfully be as ignorant to outside influences as practical.

csthomas · 2017-05-05T19:28:50Z

@PhilETaylor I have a new suggestion.

What about move it to start() method?

mbabker · 2017-05-05T19:32:13Z

That could be viable.

mbabker · 2017-05-05T19:38:57Z

JSessionHandlerJoomla::start() please. Unless there's some absolute critical reason it HAS to be in JSession::start().

…la::start()

csthomas · 2017-05-05T19:44:04Z

Great, for me ok

csthomas · 2017-05-05T19:46:35Z

@mbabker I thought about JSessionHandlerJoomla::start(), I forgot to mention class name.

wetken · 2017-09-11T08:24:51Z

I have this issue using Joomla 3.7.5. Is there anything I can do to resolve it until the next release?
(I'm using an unconventional setup, I accept that, but I'm also fighting a time constraint.)

Warning: ini_set(): A session is active. You cannot change the session module's ini settings at this time in /home/.../libraries/joomla/session/handler/joomla.php on line 46

Warning: Cannot modify header information - headers already sent by (output started at /home/.../libraries/joomla/session/handler/joomla.php:46) in /home/.../libraries/joomla/session/handler/joomla.php on line 104
Error displaying the error page: Application Instantiation Error: Failed to start the session because headers have already been sent by "/home/.../libraries/joomla/session/handler/joomla.php" at line 46.

ghost · 2017-09-11T08:39:40Z

@wetken please don't post same Comment double like in #15680

csthomas · 2017-09-11T08:42:01Z

@wetken It should be easy to copy this file and replace in 3.7.5. This PR was designed for 3.7.
At J3.8 this file was moved to other place.

If you use joomla git on linux and you have shell access you can add this changes by command:

curl https://patch-diff.githubusercontent.com/raw/joomla/joomla-cms/pull/15742.diff | git apply

wetken · 2017-09-11T09:05:33Z

I’m sorry, Franz, I realised after I had posted on the closed thread, so I re-posted on the other open one, hoping I had done no harm. If you have a pressing, intractable, issue and you’re not part of the community it can sometimes be difficult to get any attention at all. Thank you for taking the trouble. Ken From: Franz Wohlkönig [mailto:notifications@github.com] Sent: 11 September 2017 09:40 To: joomla/joomla-cms Cc: wetken; Mention Subject: Re: [joomla/joomla-cms] Check if there is a valid session before messing with session ini values (#15742) @wetken <https://github.com/wetken> please don't post same Comment double like in #15680 <#15680 (comment)> — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#15742 (comment)> , or mute the thread <https://github.com/notifications/unsubscribe-auth/AGAs3S6B5RlRlssb2ViNZJfGHRmyY5Exks5shPHVgaJpZM4NOPie> .Image removed by sender. No virus found in this message. Checked by AVG - www.avg.com <http://www.avg.com/email-signature> Version: 2016.0.8013 / Virus Database: 4782/14955 - Release Date: 09/11/17

wetken · 2017-09-11T09:31:11Z

Many thanks, Tomasz, that has helped tremendously. From: Tomasz Narloch [mailto:notifications@github.com] Sent: 11 September 2017 09:42 To: joomla/joomla-cms Cc: wetken; Mention Subject: Re: [joomla/joomla-cms] Check if there is a valid session before messing with session ini values (#15742) @wetken <https://github.com/wetken> It should be easy to copy this file and replace in 3.7.5. This PR was designed for 3.7. At J3.8 this file was moved to other place. If you use joomla git on linux and you have shell access you can add this changes by command: curl https://patch-diff.githubusercontent.com/raw/joomla/joomla-cms/pull/15742.diff | git apply — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#15742 (comment)> , or mute the thread <https://github.com/notifications/unsubscribe-auth/AGAs3ZaOUejxuUQYTNqzq0G0_Dr3TU6zks5shPJjgaJpZM4NOPie> .Image removed by sender. No virus found in this message. Checked by AVG - www.avg.com <http://www.avg.com/email-signature> Version: 2016.0.8013 / Virus Database: 4782/14955 - Release Date: 09/11/17

This is a test patch based on PR joomla#15742 updated for J3.8 to attempt to resolve CiviCRM's cron errors on J3.8. Warning: ini_set(): A session is active. You cannot change the session module's ini settings at this time in /var/www/html/membership/libraries/joomla/session/handler/joomla.php on line 48 Error: Failed to start application: Failed to start the session because headers have already been sent by "/var/www/html/membership/libraries/joomla/session/handler/joomla.php" at line 48.

This is a test patch based on PR joomla#15742 intended to fix a problem that the CiviCRM extension has been encountering since J3.8.0 with its CLI cron script not working on PHP 7.0: Warning: ini_set(): A session is active. You cannot change the session module's ini settings at this time in /var/www/html/membership/libraries/joomla/session/handler/joomla.php on line 48 Error: Failed to start application: Failed to start the session because headers have already been sent by "/var/www/html/membership/libraries/joomla/session/handler/joomla.php" at line 48.

andrewpthompson · 2018-03-05T04:35:15Z

@PhilETaylor and @mbabker I would like to ask you guys if there is still any plan to implement this PR? It would resolve a problem in the CiviCRM extension that has been present since Joomla 3.8.0. As CiviCRM has far more Drupal & Wordpress users and developers than it does Joomla, I've been trying to get this fixed out of necessity.

CiviCRM has a cron script that bootstraps the CMS and runs CiviCRM's scheduled jobs. In some environments (PHP 7) it fails to run due to:

Warning: ini_set(): A session is active. You cannot change the session module's ini settings at this time in /var/www/html/membership/libraries/joomla/session/handler/joomla.php on line 48
Error: Failed to start application: Failed to start the session because headers have already been sent by "/var/www/html/membership/libraries/joomla/session/handler/joomla.php" at line 48.

CiviCRM's cron.php is doing a session_start() and this occurs before it calls JPluginHelper::importPlugin('civicrm'); which leads to Joomla doing the ini_set() in libraries/joomla/session/handler/joomla.php when there is a session already active.

Almost the same thing happens with its REST API and again it does a session_start() before bootstapping Joomla.

I updated Phil's PR for Joomla 3.8 to test this and it does resolve these particular problems (although I don't really know if it's correct for CiviCRM to be doing the session_start() in the first place).

andrewpthompson · 2018-03-05T10:42:14Z

@PhilETaylor Would you be prepared to update your PR to resolve the conflicts so that it's more likely to get tested by other people? Thanks.

wetken · 2018-03-05T11:42:31Z

Sorry, came at me out of the blue, wasn’t sure what was happening for a moment. What exactly do you want me to do – what’s my PR? From: andrewpthompson [mailto:notifications@github.com] Sent: 05 March 2018 10:42 To: joomla/joomla-cms Cc: wetken; Mention Subject: Re: [joomla/joomla-cms] Check if there is a valid session before messing with session ini values (#15742) @PhilETaylor <https://github.com/philetaylor> Would you be prepared to update your PR to resolve the conflicts so that it's more likely to get tested by other people? Thanks. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#15742 (comment)> , or mute the thread <https://github.com/notifications/unsubscribe-auth/AGAs3fH1UbCQztU3xQtzXl_oemlHrMY3ks5tbRaVgaJpZM4NOPie> .Image removed by sender. No virus found in this message. Checked by AVG - www.avg.com <http://www.avg.com/email-signature> Version: 2016.0.8013 / Virus Database: 4793/15452 - Release Date: 03/04/18

andrewpthompson · 2018-03-05T21:21:05Z

@wetken My question was to PhilETaylor, the author of this pull request.

roland-d · 2018-07-22T21:03:55Z

@PhilETaylor Can you fix the conflicts so I can get it tested over the next two days?

_{This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/15742.}

roland-d · 2018-07-22T21:14:04Z

@PhilETaylor Thank you for replying, I will see what we can do.

Closes #12153

633c890

Closes #15680 Check if there is a valid session before messing with session ini values (NOT TESTED, coded on sight only - please test!)

joomla-cms-bot added the PR-staging label May 2, 2017

mbabker reviewed May 2, 2017

View reviewed changes