? Success

User tests: Successful: Unsuccessful:

avatar tampe125
tampe125
29 Dec 2015

If the session expires, it's no longer "valid" and an uncaught exception is thrown, resulting in the following:

Fatal error: Uncaught exception 'RuntimeException' with message 'The session is not started.' in /Users/tampe125/git/joomla-cms/libraries/joomla/session/handler/native.php on line 221
RuntimeException: The session is not started. in /Users/tampe125/git/joomla-cms/libraries/joomla/session/handler/native.php on line 221

Since this is the last thing performed, there no harm is done.
I think we should always save data to session, no matter what, however I'd like @mbabker to take a look at this. Since the code was already there, maybe there is an hidden reason for that?

avatar tampe125 tampe125 - open - 29 Dec 2015
avatar tampe125 tampe125 - change - 29 Dec 2015
Status New Pending
avatar joomla-cms-bot joomla-cms-bot - change - 29 Dec 2015
Labels Added: ?
avatar mbabker
mbabker - comment - 29 Dec 2015

IIRC it's supposed to prevent deep logic errors. At some point the session has to be started to be able to save it, and it shouldn't be trying to save the data multiple times in a single request under normal operation. In the context of the CMS it's probably fine to drop it though.

avatar anibalsanchez
anibalsanchez - comment - 30 Dec 2015

Yes, it is the same issue, described at the administrator login, that requires several retries until the session is properly initialized.

avatar infograf768
infograf768 - comment - 30 Dec 2015

I confirm the Error each time I log out.

avatar infograf768 infograf768 - test_item - 30 Dec 2015 - Tested successfully
avatar infograf768
infograf768 - comment - 30 Dec 2015

I have tested this item :white_check_mark: successfully on 8f9eecf


This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/8808.

avatar mbabker
mbabker - comment - 30 Dec 2015

If this patch "fixes" the issue, IMO someone needs to dig into the underlying issue this is exposing which is basically that sessions are being saved/closed multiple times in a request cycle.

avatar anibalsanchez anibalsanchez - test_item - 31 Dec 2015 - Tested successfully
avatar anibalsanchez
anibalsanchez - comment - 31 Dec 2015

I have tested this item :white_check_mark: successfully on 8f9eecf

Solved!


This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/8808.

avatar infograf768
infograf768 - comment - 31 Dec 2015

If this patch "fixes" the issue, IMO someone needs to dig into the underlying issue this is exposing which is basically that sessions are being saved/closed multiple times in a request cycle.

No idea, as you may guess... :smiley:
Concentrating on New Year dinner.

avatar brianteeman brianteeman - change - 12 Jan 2016
Status Pending Ready to Commit
avatar brianteeman
brianteeman - comment - 12 Jan 2016

RTC - thanks


This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/8808.

avatar joomla-cms-bot joomla-cms-bot - change - 12 Jan 2016
Labels Added: ?
avatar infograf768
infograf768 - comment - 12 Jan 2016

Milestone should be 3.5.0 for this one. It is really bothering...

avatar roland-d
roland-d - comment - 12 Jan 2016

@tampe125 Can you look into the comment made by @mbabker that there is probably an underlying issue?

avatar anibalsanchez
anibalsanchez - comment - 27 Jan 2016

Agree with @infograf768 , it must be included in J3.5 Beta 2. Otherwise, it will reported multiple time.

Please, @wilsonge take note of this Fatal error


This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/8808.

avatar mbabker
mbabker - comment - 27 Jan 2016

I'll echo what I said above about needing to dig deeper into the real issue. Merging this should be fine short term, maybe even just comment the code out instead of delete it, but please don't let it become another task for a rainy day.

avatar brianteeman
brianteeman - comment - 27 Jan 2016

@mbabker pull requests welcome ;)

On 27 January 2016 at 19:04, Michael Babker notifications@github.com
wrote:

I'll echo what I said above about needing to dig deeper into the real
issue. Merging this should be fine short term, maybe even just comment the
code out instead of delete it, but please don't let it become another
task for
https://github.com/joomla/joomla-cms/blob/a0b215250bb140092ec13d529328ceb6812c275d/plugins/user/joomla/joomla.php#L93 a
rainy day
fc9ec7a#diff-8b42e55d5e424b7f172d227f1a012e1dR72
.


Reply to this email directly or view it on GitHub
#8808 (comment).

Brian Teeman
Co-founder Joomla! and OpenSourceMatters Inc.
http://brian.teeman.net/

avatar mbabker
mbabker - comment - 27 Jan 2016

Accept my other ones first and maybe we'll talk :smiling_imp:

avatar alikon
alikon - comment - 27 Jan 2016

damned gloabl warming :sweat_smile:

avatar wilsonge
wilsonge - comment - 27 Jan 2016

I already made a decision I'm not merging this for beta 2 for exactly the reasons michael mentioned. I will mention this and the utf8mb4 as "known issues" however

avatar dgt41
dgt41 - comment - 27 Jan 2016

Shall we close it then?

avatar roland-d
roland-d - comment - 27 Jan 2016

@dgt41 No, because the issue needs to be resolved.

avatar roland-d roland-d - change - 27 Jan 2016
Status Ready to Commit Closed
Closed_Date 0000-00-00 00:00:00 2016-01-27 19:38:54
Closed_By roland-d
avatar roland-d roland-d - close - 27 Jan 2016
avatar roland-d roland-d - close - 27 Jan 2016
avatar roland-d roland-d - change - 27 Jan 2016
Status Closed New
Closed_Date 2016-01-27 19:38:54
Closed_By roland-d
Labels Removed: ?
avatar roland-d roland-d - change - 27 Jan 2016
Status New Pending
avatar roland-d roland-d - reopen - 27 Jan 2016
avatar roland-d roland-d - reopen - 27 Jan 2016
avatar joomla-cms-bot joomla-cms-bot - change - 27 Jan 2016
Status Pending Closed
avatar mbabker
mbabker - comment - 27 Jan 2016

OK, I'll give you this for free. Some ability to read C code is required.

Using PHP 5.3.10 (Joomla's minimum), PHP's session_write_close() function is a wrapper around an internal C function in the session extension, php_session_flush(). The latter function will only write if the session is active (that status is only available to userland code in PHP 5.4 or later). In lieu of being able to check PHP_SESSION_ACTIVE === session_status() (PHP 5.4 code), the two options are to either remove the check in full or instead of throwing an Exception if the session is not started (according to the Joomla API, not something internal to PHP) to just early return and not trigger the session_write_close() function. FWIW these two C functions are mostly unchanged in PHP 7.0.2 also, so there's no change in behavior across PHP branches to account for.

avatar roland-d
roland-d - comment - 28 Jan 2016

Thank you @mbabker for the feedback.

@tampe125 Can you update the PR to wrap the session_write_close in the started check? I believe it should be like this:

if (!$this->isStarted())
        {
            session_write_close();
        }

That worked for me. The docblock should be updated as well since we no longer throw an exception.

Let me know if you have time to do it, otherwise I will create a new PR. Thanks.

avatar tampe125
tampe125 - comment - 28 Jan 2016

@roland-d Can you please handle this? At the moment I'm running out of time...

avatar roland-d
roland-d - comment - 28 Jan 2016

@tampe125 No problem, will handle it. Thanks.

avatar mbabker
mbabker - comment - 28 Jan 2016

Don't do it just around session_write_close(). Either wrap the full method (because there are other side effects caused by calling it) or don't do a status check at all because the API to get the active status is not exposed to userland PHP code in PHP 5.3 (when 5.4 is the minimum do the check I noted).

avatar mbabker
mbabker - comment - 28 Jan 2016

Or, if you really want to do a check if the session is active (as PHP defines it, because that's what the underlying code is reliant on, not Joomla's state tracking for sessions), do a check similar to what's in the start method (checks status with PHP 5.4 API if able and a fallback mechanism for 5.3).

avatar roland-d
roland-d - comment - 28 Jan 2016

I have created a new PR #9011 Hope all is good like this @mbabker

avatar roland-d roland-d - change - 28 Jan 2016
Status New Closed
Closed_Date 0000-00-00 00:00:00 2016-01-28 16:02:26
Closed_By roland-d
avatar roland-d roland-d - close - 28 Jan 2016

Add a Comment

Login with GitHub to post a comment