$options['_expire'] in JSession expects minutes not seconds #1715

Closed
wants to merge 1 commit into
from

Projects

None yet

4 participants

@elinw
Contributor
elinw commented Nov 25, 2012

There is a mismatch between the factory (which is converting session expiration time to seconds when it creates a session object) and JSession where the property is described as measured in minutes in several places.

https://github.com/joomla/joomla-platform/blob/staging/libraries/joomla/session/session.php#L37

https://github.com/joomla/joomla-platform/blob/staging/libraries/joomla/session/session.php#L196

This proposes to use minutes consistently.

@elinw elinw $options['expire'] expects minute not seconds
There is a mismatch between the factory (which is converting session expiration time to seconds and 
JSession where the property is described as measured in minutes in several places.

https://github.com/joomla/joomla-platform/blob/staging/libraries/joomla/session/session.php#L37

https://github.com/joomla/joomla-platform/blob/staging/libraries/joomla/session/session.php#L196

This proposes to use minutes consistently.
91bb161
@elinw
Contributor
elinw commented Nov 25, 2012

I should say this issue came up in writing some tests and getting 900 as the result of getExpire while the existing tests expected 20.

@eddieajau
Contributor

I have no problem with the change, but will this affect downstream behaviour?

@elinw
Contributor
elinw commented Dec 5, 2012

The downstream behavior in the CMS is just that, the default session is currently being set to 15 hours and the documentation in the form of the tooltip says it is being set to 15 minutes.

@realityking
Member

The issue is even worse than that. We're using this value to set session.gc_maxlifetime (https://github.com/joomla/joomla-platform/blob/staging/libraries/joomla/session/session.php#L934) which is measured in seconds (http://php.net/manual/en/session.configuration.php#ini.session.gc-maxlifetime).

@eddieajau
Contributor

I'm not sure how to call this fix? Is the CMS going to be ok if we merge it?

@dongilbert
Contributor

@elinw @mbabker Should we merge or close this? What is the CMS's view on it?

@elinw
Contributor
elinw commented Mar 18, 2013

I really am not sure what to do. On the one hand we clearly have a situation where the logic is failing and the unit tests prove it (hence the other issue that Andrew closed was not resolvable). On the other hand, it certainly is an unstable change of behavior.

I think I am going to call it close and fix in Framework when session is reconstructed and JCache is merged. It's just too risky/too much in the way of potential unknown consequences right now.

@elinw elinw closed this Mar 18, 2013
@dongilbert
Contributor

OK - thanks for the update.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment