php session.name could be incorrect with default setup. #218

Closed
andraskende opened this Issue Dec 22, 2011 · 7 comments

Comments

Projects
None yet
4 participants
@andraskende

lithium/storage/session/adapter/Php.php
line 57:
$config['session.name'] = basename(LITHIUM_APP_PATH);

/Users/andras/Sites/s3.kende.dev/app/webroot :
this works :
basename(LITHIUM_APP_PATH) = 'app'

/Users/andras/Sites/s3.kende.dev/webroot :
this doesn't works :
basename(LITHIUM_APP_PATH) = 's3.kende.dev' probably the periods are causing issues with session.name not saving etc...

This was confusing as it was working while having app folder :)

I figured to set the session.name as below which works great:

Session::config(array(
// 'cookie' => array('adapter' => 'Cookie'),
'default' => array('adapter' => 'Php', 'session.name' => 'shop')
));

Wondering if the $config['session.name'] should be changed to alphanumeric..

Thank you!!

@mehlah

This comment has been minimized.

Show comment
Hide comment
@mehlah

mehlah Dec 22, 2011

Contributor

Yep, I confirm that as I had the same issue #99 finally solved by setting manually a session name

Contributor

mehlah commented Dec 22, 2011

Yep, I confirm that as I had the same issue #99 finally solved by setting manually a session name

@nateabele

This comment has been minimized.

Show comment
Hide comment
@nateabele

nateabele Dec 22, 2011

Member

Interesting. Might be good to use Inflector::slug() or something on it in that case. @gwoo, @daschl Thoughts?

Member

nateabele commented Dec 22, 2011

Interesting. Might be good to use Inflector::slug() or something on it in that case. @gwoo, @daschl Thoughts?

@daschl

This comment has been minimized.

Show comment
Hide comment
@daschl

daschl Dec 22, 2011

Contributor

Looking at the documentation here `http://php.net/manual/en/function.session-name.php, it seems that only alphanumeric characters are allowed:

The session name references the name of the session, which is used in cookies and URLs (e.g. PHPSESSID). It should contain only alphanumeric characters; it should be short and descriptive (i.e. for users with enabled cookie warnings). If name is specified, the name of the current session is changed to its value.

Warning: The session name can't consist of digits only, at least one letter must be present. Otherwise a new session id is generated every time.

Slugs also allow and don't incorporate the additional warning so maybe we should write a string method for this?

Contributor

daschl commented Dec 22, 2011

Looking at the documentation here `http://php.net/manual/en/function.session-name.php, it seems that only alphanumeric characters are allowed:

The session name references the name of the session, which is used in cookies and URLs (e.g. PHPSESSID). It should contain only alphanumeric characters; it should be short and descriptive (i.e. for users with enabled cookie warnings). If name is specified, the name of the current session is changed to its value.

Warning: The session name can't consist of digits only, at least one letter must be present. Otherwise a new session id is generated every time.

Slugs also allow and don't incorporate the additional warning so maybe we should write a string method for this?

@mehlah

This comment has been minimized.

Show comment
Hide comment
@mehlah

mehlah Dec 22, 2011

Contributor

What about making a default string like Lithium and add it to bootstrap/session.php config array with a short comment about accepted values, to invite users to set it manually.

If an additional method is added to util\String, it needs anyway a default string in case basename(LITHIUM_APP_PATH) is numeric (a timestamp in case of an automatic deployment).

Contributor

mehlah commented Dec 22, 2011

What about making a default string like Lithium and add it to bootstrap/session.php config array with a short comment about accepted values, to invite users to set it manually.

If an additional method is added to util\String, it needs anyway a default string in case basename(LITHIUM_APP_PATH) is numeric (a timestamp in case of an automatic deployment).

@daschl

This comment has been minimized.

Show comment
Hide comment
@daschl

daschl Dec 22, 2011

Contributor

Jeah, I think a default string with proper override documentation is a good idea.

@nateabele ?

Contributor

daschl commented Dec 22, 2011

Jeah, I think a default string with proper override documentation is a good idea.

@nateabele ?

@nateabele

This comment has been minimized.

Show comment
Hide comment
@nateabele

nateabele Dec 29, 2011

Member

Well, I like the name of the cookie to automatically follow the name of the app. In either case, the override should be documented, but I lean towards using Inflector::slug() on the base name.

Member

nateabele commented Dec 29, 2011

Well, I like the name of the cookie to automatically follow the name of the app. In either case, the override should be documented, but I lean towards using Inflector::slug() on the base name.

@andraskende

This comment has been minimized.

Show comment
Hide comment
@andraskende

andraskende Jan 5, 2012

this was fixed at #225 closing issue

this was fixed at #225 closing issue

@andraskende andraskende closed this Jan 5, 2012

gwoo added a commit that referenced this issue Jan 5, 2012

Updating Php and Cookie Session adapters to use the basename of the L…
…ITHIUM_APP_PATH by defalt.

In the future, this default may be removed so we can also completely remove the LITHIUM_APP_PATH constant.
Please refer to commit to UnionOfRAD/framework@72d6673 for proper usage.
refs #225 and #218.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment