? Success

User tests: Successful: Unsuccessful:

avatar PhilETaylor
PhilETaylor
2 May 2017

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!)

avatar PhilETaylor PhilETaylor - open - 2 May 2017
avatar PhilETaylor PhilETaylor - change - 2 May 2017
Status New Pending
avatar joomla-cms-bot joomla-cms-bot - change - 2 May 2017
Category Libraries
avatar PhilETaylor
PhilETaylor - comment - 2 May 2017

this is just a edge case workaround and not something that should exist in production code

In that case someone needs to make a decision and then we can either change this PR or close the two outstanding issues with one foul sweep of the brush...

avatar PhilETaylor
PhilETaylor - comment - 2 May 2017

$this->isStarted()

This will only return true if the session was started by the Joomla API right, and not if "something else" started the session.

$this->getId()

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

avatar mbabker
mbabker - comment - 2 May 2017

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).

avatar csthomas
csthomas - comment - 2 May 2017
avatar mbabker
mbabker - comment - 2 May 2017

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.

avatar PhilETaylor PhilETaylor - change - 5 May 2017
Labels Added: ?
avatar PhilETaylor
PhilETaylor - comment - 5 May 2017

Implemented @mbabker request to use api.
Implemented @csthomas request to move settings.

avatar PhilETaylor
PhilETaylor - comment - 5 May 2017

Reverted @csthomas request to move settings on @mbabker suggestion

avatar csthomas
csthomas - comment - 5 May 2017

@PhilETaylor I have a new suggestion.

What about move it to start() method?

avatar PhilETaylor
PhilETaylor - comment - 5 May 2017

No idea - these kind of decisions are above my pay grade - paging @mbabker :)

avatar mbabker
mbabker - comment - 5 May 2017

That could be viable.

avatar PhilETaylor
PhilETaylor - comment - 5 May 2017

Implemented @csthomas request to move to start() :-)

avatar mbabker
mbabker - comment - 5 May 2017

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

avatar PhilETaylor
PhilETaylor - comment - 5 May 2017

Done.

avatar csthomas
csthomas - comment - 5 May 2017

Great, for me ok

avatar csthomas
csthomas - comment - 5 May 2017

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

avatar brianteeman brianteeman - change - 8 Jun 2017
Milestone Added:
avatar brianteeman brianteeman - change - 8 Jun 2017
Milestone Added:
avatar brianteeman brianteeman - change - 8 Jun 2017
Milestone Removed:
avatar brianteeman brianteeman - change - 8 Jun 2017
Milestone Removed:
avatar wetken
wetken - comment - 11 Sep 2017

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.

avatar franz-wohlkoenig
franz-wohlkoenig - comment - 11 Sep 2017

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

avatar csthomas
csthomas - comment - 11 Sep 2017

@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
avatar wetken
wetken - comment - 11 Sep 2017

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

avatar wetken
wetken - comment - 11 Sep 2017

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

avatar andrewpthompson
andrewpthompson - comment - 5 Mar 2018

@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).

avatar PhilETaylor
PhilETaylor - comment - 5 Mar 2018

@andrewpthompson If people test it and provide test results then it will have more of a chance of being accepted for merging.

avatar andrewpthompson
andrewpthompson - comment - 5 Mar 2018

@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.

avatar wetken
wetken - comment - 5 Mar 2018

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

avatar andrewpthompson
andrewpthompson - comment - 5 Mar 2018

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

avatar roland-d
roland-d - comment - 22 Jul 2018

@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.

avatar PhilETaylor
PhilETaylor - comment - 22 Jul 2018

The joy of having a PR open for over a year. 👎

It looks like the conflicts come from @mbabker 's commit here 5b92dc6

I will not be able to fix the conflicts this week. This is my busiest week of the year, the first week of the school holidays, and the week everyone wants (paid) work done before I take several weeks 'off' to go on holiday (Tokyo).

Please feel free to close and replace this PR in my absence.

avatar roland-d
roland-d - comment - 22 Jul 2018

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

avatar PhilETaylor
PhilETaylor - comment - 10 Feb 2019
avatar PhilETaylor PhilETaylor - close - 10 Feb 2019
avatar PhilETaylor PhilETaylor - change - 10 Feb 2019
Status Pending Closed
Closed_Date 0000-00-00 00:00:00 2019-02-10 20:52:39
Closed_By PhilETaylor

Add a Comment

Login with GitHub to post a comment