Allowing Session id to be set manually #818

Closed
wants to merge 3 commits into
from

Conversation

Projects
None yet
4 participants
@thechriswalker
Contributor

thechriswalker commented Feb 8, 2013

Updated the Session adaptable to pass through an optional
session_id parameter in the ::key method, to set the
session id, rather than only allowing getting of the id.
The PHP adapter supported this, but the functionality was
masked by the Session adaptable not passing the id on.

This uncovered a bug in the PHP adaptable in the ::isStarted
method which would false-positive if session_id($non_empty)
had been called beforehand. Also, the ::enabled() method
was checking whether a session had been started, and not whether
php session functionality was enabled. Both of these have been
fixed and the test updated.

NB session started detection is vastly superior in PHP5.4 due
to the presence of the session_status() function.

Allowing Session id to be set manually
Updated the Session adaptable to pass through an optional
session_id parameter in the `::key` method, to set the
session id, rather than only allowing getting of the id.
The PHP adapter supported this, but the functionality was
masked by the Session adaptable not passing the id on.

This uncovered a bug in the PHP adaptable in the `::isStarted`
method which would false-positive if `session_id($non_empty)`
had been called beforehand. Also, the `::enabled()` method
was checking whether a session had been started, and not whether
php session functionality was enabled. Both of these have been
fixed and the test updated.

NB session started detection is vastly superior in PHP5.4 due
to the presence of the `session_status()` function.
@gwoo

This comment has been minimized.

Show comment Hide comment
@gwoo

gwoo Feb 8, 2013

Contributor

Could you describe the use case for why this is necessary?

Contributor

gwoo commented Feb 8, 2013

Could you describe the use case for why this is necessary?

@nateabele

This comment has been minimized.

Show comment Hide comment
@nateabele

nateabele Feb 8, 2013

Member

Strictly speaking, the implementation for enabled() that we have now is not correct, which is why I asked him to submit the PR.

That said, this patch fails the coding standards on two counts: variables should be camelBacked, and I count two unnecessary else clauses. Finally, this causes some tests to fail on 5.3, so if you could fix the patch and squash accordingly, that'd be awesome. Thanks.

Member

nateabele commented Feb 8, 2013

Strictly speaking, the implementation for enabled() that we have now is not correct, which is why I asked him to submit the PR.

That said, this patch fails the coding standards on two counts: variables should be camelBacked, and I count two unnecessary else clauses. Finally, this causes some tests to fail on 5.3, so if you could fix the patch and squash accordingly, that'd be awesome. Thanks.

@thechriswalker

This comment has been minimized.

Show comment Hide comment
@thechriswalker

thechriswalker Feb 8, 2013

Contributor

I'll fix up the coding standards and rebase as requested.

As for the purpose, in an app I am writing, I do not want to use session cookies, but access tokens in an HTTP header. However in principle the two things are analogous and using the Session adapter provides me with the functionality I need.

The user specifies a token in a header and that is used to key the session. Before this change it was impossible to set the session id manually, because the Php adapter class would false-positive thinking the session was already started.

Contributor

thechriswalker commented Feb 8, 2013

I'll fix up the coding standards and rebase as requested.

As for the purpose, in an app I am writing, I do not want to use session cookies, but access tokens in an HTTP header. However in principle the two things are analogous and using the Session adapter provides me with the functionality I need.

The user specifies a token in a header and that is used to key the session. Before this change it was impossible to set the session id manually, because the Php adapter class would false-positive thinking the session was already started.

@thechriswalker

This comment has been minimized.

Show comment Hide comment
@thechriswalker

thechriswalker Feb 8, 2013

Contributor

Hope I've found all the violations! I call 'first contribution' 🍰

Contributor

thechriswalker commented Feb 8, 2013

Hope I've found all the violations! I call 'first contribution' 🍰

@jails

This comment has been minimized.

Show comment Hide comment
@jails

jails Feb 8, 2013

Contributor

Thanks ! Just squash the PR and it'll be awesome ;-)

Contributor

jails commented Feb 8, 2013

Thanks ! Just squash the PR and it'll be awesome ;-)

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