UnionOfRAD / lithium

li₃ is the fast, flexible and most RAD development framework for PHP
http://li3.me
BSD 3-Clause "New" or "Revised" License
1.22k stars 238 forks source link

PHP Session Adapter failing when Cookie Adapter is also configured. #457

Open daschl opened 12 years ago

daschl commented 12 years ago

Hi all,

I've discovered a strange issue, maybe we can track it down together.

I have the following session config:

<?php
use lithium\storage\Session;

$name = basename(LITHIUM_APP_PATH);
Session::config(array(
    'default' => array(
        'adapter' => 'Php', 
        'session.name' => $name
    ),
    'cart' => array(
        'adapter' => 'Cookie', 
        'name' => $name
    )
));
?>

Now in my view, I check for login state:

<?php if($user = \lithium\security\Auth::check('default')): ?>
    <p class="navbar-text pull-right">Logged in as <?= $this->html->link($user['username'],  'Users::edit'); ?> | <?= $this->html->link('Logout', 'Sessions::delete'); ?></p>
<?php else: ?>
    <p class="navbar-text pull-right"><?= $this->html->link('Sign up', 'Users::add'); ?> or <?= $this->html->link('Login', 'Sessions::add'); ?></p>
<?php endif; ?>

And then I get the following error:

Notice: Array to string conversion in /home/michael/Websites/laptopshop/libraries/lithium/storage/session/adapter/Php.php on line 86

Interestingly, the line points to the session_start() call.

If I remove the Cookie config and just leave the Php adapter in there, it works like a charm.

I couldnt figure out where the conversion here is going on the first place, because settings like session.name are correctly formatted strings. Maybe the Auth adapter somewhere up the road is messing up with two configs?

Here's the full stack trace:

Call Stack:
    0.0000     330116   1. {main}() /home/michael/Websites/laptopshop/webroot/index.php:0
    0.0030    1528140   2. lithium\action\Dispatcher::run() /home/michael/Websites/laptopshop/webroot/index.php:41
    0.0030    1529692   3. lithium\core\StaticObject::_filter() /home/michael/Websites/laptopshop/libraries/lithium/action/Dispatcher.php:123
    0.0030    1531812   4. lithium\util\collection\Filters::run() /home/michael/Websites/laptopshop/libraries/lithium/core/StaticObject.php:126
    0.0031    1534944   5. lithium\core\{closure}() /home/michael/Websites/laptopshop/libraries/lithium/util/collection/Filters.php:183
    0.0031    1534944   6. lithium\util\collection\Filters->next() /home/michael/Websites/laptopshop/libraries/lithium/core/ErrorHandler.php:289
    0.0031    1534988   7. {closure}() /home/michael/Websites/laptopshop/libraries/lithium/util/collection/Filters.php:202
    0.0031    1534988   8. lithium\util\collection\Filters->next() /home/michael/Websites/laptopshop/config/bootstrap/cache.php:46
    0.0031    1535032   9. {closure}() /home/michael/Websites/laptopshop/libraries/lithium/util/collection/Filters.php:202
    0.0058    1595552  10. lithium\util\collection\Filters->next() /home/michael/Websites/laptopshop/config/bootstrap/action.php:52
    0.0058    1595596  11. lithium\action\{closure}() /home/michael/Websites/laptopshop/libraries/lithium/util/collection/Filters.php:202
    0.0075    1620972  12. lithium\core\StaticObject::invokeMethod() /home/michael/Websites/laptopshop/libraries/lithium/action/Dispatcher.php:122
    0.0075    1621016  13. lithium\action\Dispatcher::_call() /home/michael/Websites/laptopshop/libraries/lithium/core/StaticObject.php:75
    0.0075    1622404  14. lithium\core\StaticObject::_filter() /home/michael/Websites/laptopshop/libraries/lithium/action/Dispatcher.php:225
    0.0075    1622752  15. lithium\action\{closure}() /home/michael/Websites/laptopshop/libraries/lithium/core/StaticObject.php:119
    0.0075    1622752  16. lithium\action\Controller->__invoke() /home/michael/Websites/laptopshop/libraries/lithium/action/Dispatcher.php:222
    0.0075    1624792  17. lithium\core\Object->_filter() /home/michael/Websites/laptopshop/libraries/lithium/action/Controller.php:192
    0.0075    1625176  18. lithium\action\{closure}() /home/michael/Websites/laptopshop/libraries/lithium/core/Object.php:238
    0.0152    2029348  19. lithium\action\Controller->render() /home/michael/Websites/laptopshop/libraries/lithium/action/Controller.php:189
    0.0154    2032444  20. lithium\net\http\Media::render() /home/michael/Websites/laptopshop/libraries/lithium/action/Controller.php:260
    0.0154    2044912  21. lithium\core\StaticObject::_filter() /home/michael/Websites/laptopshop/libraries/lithium/net/http/Media.php:593
    0.0154    2045256  22. lithium\net\http\{closure}() /home/michael/Websites/laptopshop/libraries/lithium/core/StaticObject.php:119
    0.0156    2049152  23. lithium\core\StaticObject::invokeMethod() /home/michael/Websites/laptopshop/libraries/lithium/net/http/Media.php:590
    0.0156    2049584  24. lithium\net\http\Media::_handle() /home/michael/Websites/laptopshop/libraries/lithium/core/StaticObject.php:75
    0.0156    2052144  25. lithium\core\StaticObject::_filter() /home/michael/Websites/laptopshop/libraries/lithium/net/http/Media.php:750
    0.0156    2052488  26. lithium\net\http\{closure}() /home/michael/Websites/laptopshop/libraries/lithium/core/StaticObject.php:119
    0.0169    2082092  27. lithium\template\View->render() /home/michael/Websites/laptopshop/libraries/lithium/net/http/Media.php:746
    0.0260    2242012  28. lithium\template\View->_step() /home/michael/Websites/laptopshop/libraries/lithium/template/View.php:268
    0.0260    2246592  29. lithium\core\Object->_filter() /home/michael/Websites/laptopshop/libraries/lithium/template/View.php:310
    0.0260    2246968  30. lithium\template\{closure}() /home/michael/Websites/laptopshop/libraries/lithium/core/Object.php:238
    0.0262    2247208  31. lithium\template\view\adapter\File->render() /home/michael/Websites/laptopshop/libraries/lithium/template/View.php:308
    0.0263    2289288  32. include('/home/michael/Websites/laptopshop/resources/tmp/cache/templates/template_views_layouts_default.html_395720_1336978145_2335.php') /home/michael/Websites/laptopshop/libraries/lithium/template/view/adapter/File.php:111
    0.0324    2410528  33. lithium\security\Auth::check() /home/michael/Websites/laptopshop/resources/tmp/cache/templates/template_views_layouts_default.html_395720_1336978145_2335.php:38
    0.0324    2412836  34. lithium\core\StaticObject::_filter() /home/michael/Websites/laptopshop/libraries/lithium/security/Auth.php:131
    0.0324    2413180  35. lithium\security\{closure}() /home/michael/Websites/laptopshop/libraries/lithium/core/StaticObject.php:119
    0.0325    2415704  36. lithium\storage\Session::read() /home/michael/Websites/laptopshop/libraries/lithium/security/Auth.php:122
    0.0333    2445660  37. lithium\storage\session\adapter\Php::read() /home/michael/Websites/laptopshop/libraries/lithium/storage/Session.php:100
    0.0333    2445660  38. lithium\storage\session\adapter\Php::_start() /home/michael/Websites/laptopshop/libraries/lithium/storage/session/adapter/Php.php:137
    0.0333    2445820  39. session_start() /home/michael/Websites/laptopshop/libraries/lithium/storage/session/adapter/Php.php:86
daschl commented 12 years ago

Update:

okay, it looks like the identical names seem to be the root cause. Maybe we can raise an exception if this is not allowed?

nateabele commented 12 years ago

I've been meaning to change the API to eliminate the ambiguity. What do you think: Session::<method>("configName.keyName.subKeyName", ...) or Session::<method>("configName", "keyName.subKeyName", ...)?

daschl commented 12 years ago

I'd stick with the latter one, because the key is not directly associated with the config (often, you want to autogenerate keys in some way, but you don't want to mash the configName in there).

Another reason for this is that afaik the Cache API works the same way, and we should really keep it consistent.

On a side note, this doesn't solve the issue above - right? :)

nateabele commented 12 years ago

I believe it will, actually, which is why I brought it up. ;-)

Ciaro commented 12 years ago
ericcholis commented 12 years ago

@daschl Agreed, it would appear that their is a collision between configurations if you specify more than one with the same name.

For the time being, I'm using the Session and Cookie storage classes from @farhadi https://github.com/farhadi/ali3/tree/master/storage

These just force the default adapters for Session to "default" and Cookie to "cookie". Seems to work fine for now.

rmarscher commented 12 years ago

Joining the conversation here. I do think it's unintuitive the way the session adapter operates on all configurations by default. I think removing that makes sense.

I feel like you should be able to set 'default' => true in a configuration to get it to be the default. Then you could use 'true' for the configuration name to get the default (similar to the Environment and some other Adaptable classes). That would make sessions easier to work with across libraries/plugins.

Breaking the api is tough though. Session calls are littered all over the place. I could deal though.

I wonder if you could do it in a semi-polymorphous way to maintain backwards compatibility. i.e. Session::read("key") gets a key from the default configuration, Session::read("config", "key") gets from the named config, Session::read("key", array("name" => "cookie")) would be backwards compatibility to naming a config, and then Session::read("config", "key", array("strategies" => false)) would be the new preferred way.

To @daschl's original problem... php session.name is the name of the cookie it sets to save the session_id. So you wind up with a cookie named 'app' for the php session cookie. The cookie adapter will write cookies like 'app[someKey]' -- but php's $_COOKIE array automatically converts cookies named 'app[someKey]' into $_COOKIE['app']['someKey']. So when the php session handler tries to read that cookie to get the session_id, it chokes on it being an array.

So unless Nate is thinking that cookies will get named something like 'configuration.name' instead of just 'name', the conflict between naming php 'session.name' and cookie adapter 'name' isn't going to be fixed.

bayleedev commented 11 years ago

Has this been resolved?

SayB commented 11 years ago

Just ran into this issue ... so I don't think so @BlaineSch - investigating ...

SayB commented 11 years ago

investigation concluded ... the issue still persists when "cookie" configuration is added.

Also, heads up, anyone who faces the issue even after removing the cookie setting from Session configuration, you'd need to clear the cookies.

mariuswilms commented 9 years ago

Now targeting 1.1. We will also need to change the API (in a BC way) of the Session class, forcing to be explicit about the configuration which gets used. Additionally the actual name of the cookie used for both Php and Cookie adapters will need to be generated in a more safe way to avoid name clashes.

There are already many changes in 1.0. Basically this works if configured correctly.