User tests: Successful: Unsuccessful:
Pull Request for #15742
Original issue closes:
Moved ini_set
out of constructor into JSessionHandlerJoomla::start
and added a check to test, if there is an active session, so ini_set
doesn't get called. This prevents an error from being thrown, if a PHP session was setup before Joomla! is started.
Additionally added a similar check in JSessionHandlerJoomla::setCookieParams
, because the last call inside this function session_set_cookie_params
also fails, if there is already a PHP session.
You can test this code by putting a session_start()
statement somewhere, before the Joomla! application starts. For example in the beginning of your index.php or inside a custom php-application using the Joomla! framework.
No warnings or errors.
A warning is displayed, telling you, that you can't change cookie parameters when a session is active:
Warning: session_set_cookie_params(): Cannot change session cookie parameters when session is active in xxx/libraries/joomla/session/handler/joomla.php on line 151
Or a warning is displayed, telling you, that you can't change the ini when a session is active:
Warning: ini_set(): A session is active. You cannot change the session module's ini settings at this time in xxx/libraries/joomla/session/handler/joomla.php on line 48
Manipulating the PHP session outside of Joomla! leads to Joomla! being unable to setup the session by itself. This prevents the cookie params from being set (with and without the fix).
Perhaps there should be a warning, that you should not mess with the session unless you're knowing what you are doing.
Status | New | ⇒ | Pending |
Category | ⇒ | Libraries |
@franz-wohlkoenig i guess it's because of this #15742 (comment)
Phil is not able to fix conflicts these days
@jeckodevelopment thanks, so all fine.
I looked into Joomla! 4.0, where this issue also exists. But after adding the two checks from this PR to /libraries/src/Session/Storage/JoomlaStorage.php
the application still fails and I'm greeted with following error site:
The reason for this lies within /libraries/vendor/joomla/session/src/Storage/NativeStorage.php
:
The method setName on line 403 checks for an active session and terminates
public function setName(string $name)
{
if ($this->isActive())
{
throw new \LogicException('Cannot change the name of an active session');
}
session_name($name);
return $this;
}
So I have two questions now:
Labels |
Added:
?
|
I'm happy for 3.8 to ignore I think - especially as in 3.x we don't have good session management in the CLI scripts - and just to tighten the screw in 4.x
Okay, that's fine. So we just need some testers before we can finally merge this PR
So we just need some testers before we can finally merge this PR
@niklas-deworetzki-thm correct. Every Pull Request need 2 successfully Tests (not by PR-Dev.) before Maintainer decide if PR gets merged.
@niklas-deworetzki-thm I have tested this patch on Joomla 3.9.1 and it works as expected.
My interest in it is that it would resolve a longstanding problem (since J3.8) with CiviCRM's cron jobs failing because of Error: Failed to start application: Failed to start the session because headers have already been sent by "/var/www/html/libraries/joomla/session/handler/joomla.php" at line 48.
. I used this in part to test the patch: works fine with it applied and broken without.
I am wondering about the other instances of headers_sent()
in the Joomla code (or maybe what I'm about to say would be scope creep...). This commit is a good place to see the most relevant instances, i.e. where there is an ini_set()
after if (!headers_sent())
, and as is the case inJSessionHandlerNative
as it currently stands (before this PR), these don't have && session_id() == ''
so is there potential for the same problem? I don't know if there is a real problem though, because with this PR's patch applied I can't produce any problem with Joomla's session handler set to database or redis (I didn't test others).
@niklas-deworetzki-thm can you please comment @andrewpthompson above?
@andrewpthompson I don't know about any other instances of ini_set()
in the Joomla code. But having an "unprotected" ini_set()
doesn't necessarily cause problems or a crash, since it's a timing issue. As long as no data has been sent, there is nothing to fear from setting ini-variables.
Since those variables should only be changed during Joomlas startup procedure, it is relatively safe to assume no other errors will occur at runtime.
Thanks @niklas-deworetzki-thm. That makes sense. I hadn't seen any problem arise from other instances of ini_set()
anyway. This patch worked well for me.
@andrewpthompson please mark your test as successfully > https://docs.joomla.org/Testing_Joomla!_patches#Recording_test_results
I have tested this item
First, I reproduced the problem using the CiviCRM extension. CiviCRM has had a longstanding problem (since J3.8) with its cron jobs failing to run. I used this command to test:
wget -O - -q -t 1 'https://localhost/testsite/administrator/components/com_civicrm/civicrm/bin/cron.php?name=civicrm_cron&pass=password&key=mysitekey'
Prior to applying the patch I received this error:
Error: Failed to start application: Failed to start the session because headers have already been sent by "/var/www/html/testsite/libraries/joomla/session/handler/joomla.php" at line 48.
After applying the patch, no error.
No adverse effects seen from the patch. I tested logging in and out of J Admin console and frontend, editing article.
I have tested this item
First, I reproduced the problem using the CiviCRM extension. CiviCRM has had a longstanding problem (since J3.8) with its cron jobs failing to run. I used this command to test:
wget -O - -q -t 1 'https://localhost/testsite/administrator/components/com_civicrm/civicrm/bin/cron.php?name=civicrm_cron&pass=password&key=mysitekey'
Prior to applying the patch I received this error:
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.
After applying the patch, no error.
No adverse effects seen from the patch. I tested logging in and out of J Admin console and frontend, editing article.
@PhilETaylor can you please test as Opener of #15742?
@franz-wohlkoenig No. Sorry but I no longer contribute to the Joomla project as is not a safe place for contributors.
as is not a safe place for contributors.
didn't get what this mean.
@PhilETaylor got it and sorry to read. I try to make this better.
It can't be applied:
The file marked for modification does not exist: libraries/joomla/session/handler/joomla.php
@anibalsanchez did you by chance test this on 4.0? This PR is for 3.9.x.
We are testing on J4.
Status | Pending | ⇒ | Closed |
Closed_Date | 0000-00-00 00:00:00 | ⇒ | 2022-03-28 07:25:18 |
Closed_By | ⇒ | laoneo | |
Labels |
Added:
?
Removed: ? |
As Joomla is an application in itself where a session management is completely handled internally, a session should not be started before invoking an index.php. If you want to start the session before the app is loaded, then you can do in Joomla 4 your own session service provider and start the app with that one, so you have full control over the session. I also really suggest to use the web service API to interact nowadays with Joomla from an external application. So I'm closing as this is expected behavior.
@niklas-deworetzki-thm #15742 is an Pull Request, so you wan't to close this?