Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[2.3] Handle PHP sessions started outside of Symfony #7571

Closed
wants to merge 5 commits into from
Closed

[2.3] Handle PHP sessions started outside of Symfony #7571

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
9bf391f
[HttpFoundation][Session] Better detection of existing sessions.
Apr 5, 2013
3fcceae
[HttpFoundation][Session] Don't wipe out non-Symfony session keys for…
Apr 5, 2013
7004fe5
Fix coding standards
Apr 5, 2013
f31fcc4
[HttpFoundation][Session] Clean up after tests.
Apr 7, 2013
190d87d
[HttpFoundation][FrameworkBundle] Added PhpBridgeSessionStorage as a
Apr 13, 2013
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
51 changes: 34 additions & 17 deletions src/Symfony/Component/HttpFoundation/Session/Storage/NativeSessionStorage.php
View file
Open in desktop
Expand Up @@ -12,6 +12,7 @@
namespace Symfony\Component\HttpFoundation\Session\Storage;

use Symfony\Component\HttpFoundation\Session\SessionBagInterface;
use Symfony\Component\HttpFoundation\Session\Storage\Handler\NativeSessionHandler;
use Symfony\Component\HttpFoundation\Session\Storage\MetadataBag;
use Symfony\Component\HttpFoundation\Session\Storage\Proxy\NativeProxy;
use Symfony\Component\HttpFoundation\Session\Storage\Proxy\AbstractProxy;
Expand Down Expand Up @@ -92,7 +93,8 @@ class NativeSessionStorage implements SessionStorageInterface
* url_rewriter.tags, "a=href,area=href,frame=src,form=,fieldset="
*
* @param array $options Session configuration options.
* @param object $handler SessionHandlerInterface.
* @param object|null $handler Must be instance of AbstractProxy or NativeSessionHandler;
* implement \SessionHandlerInterface; or be null.
* @param MetadataBag $metaBag MetadataBag.
*/
public function __construct(array $options = array(), $handler = null, MetadataBag $metaBag = null)
Expand Down Expand Up @@ -130,27 +132,26 @@ public function start()
return true;
}

// catch condition where session was started automatically by PHP
if (!$this->started && !$this->closed && $this->saveHandler->isActive()
&& $this->saveHandler->isSessionHandlerInterface()) {
$this->loadSession();

return true;
if (version_compare(phpversion(), '5.4.0', '>=') && \PHP_SESSION_ACTIVE === session_status()) {
throw new \RuntimeException('Failed to start the session: already started by PHP');
} elseif (version_compare(phpversion(), '5.4.0', '<') && isset($_SESSION) && session_id()) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can be if instead of elseif as the previous if block throws an exception

// not 100% fool-proof, but is the most reliable way to determine if a session is active in PHP 5.3
throw new \RuntimeException('Failed to start the session: already started by PHP ($_SESSION is set)');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All exception messages must end with a dot.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.

}

if (ini_get('session.use_cookies') && headers_sent()) {
throw new \RuntimeException('Failed to start the session because headers have already been sent.');
throw new \RuntimeException('Failed to start the session because headers have already been sent');
}

// start the session
// ok to try and start the session
if (!session_start()) {
throw new \RuntimeException('Failed to start the session');
}

$this->loadSession();

if (!$this->saveHandler->isWrapper() && !$this->saveHandler->isSessionHandlerInterface()) {
$this->saveHandler->setActive(false);
if (!$this->saveHandler->isWrapper() && !$this->getSaveHandler()->isSessionHandlerInterface()) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why changing it to use the getter the second time ?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Man you have good eyes :) fixed.

// This condition matches only PHP 5.3 with internal save handlers
$this->saveHandler->setActive(true);
}

return true;
Expand Down Expand Up @@ -216,6 +217,7 @@ public function save()
session_write_close();

if (!$this->saveHandler->isWrapper() && !$this->getSaveHandler()->isSessionHandlerInterface()) {
// This condition matches only PHP 5.3 with internal save handlers
$this->saveHandler->setActive(false);
}

Expand Down Expand Up @@ -329,29 +331,44 @@ public function setOptions(array $options)
}

/**
* Registers save handler as a PHP session handler.
* Registers session save handler as a PHP session handler.
*
* To use internal PHP session save handlers, override this method using ini_set with
* session.save_handler and session.save_path e.g.
*
* ini_set('session.save_handler', 'files');
* ini_set('session.save_path', /tmp');
*
* or pass in a NativeSessionHandler instance which configures session.save_handler in the
* constructor, for a template see NativeFileSessionHandler or use handlers in
* composer package drak/native-session
*
* @see http://php.net/session-set-save-handler
* @see http://php.net/sessionhandlerinterface
* @see http://php.net/sessionhandler
* @see http://github.com/drak/NativeSession
*
* @param object|null $saveHandler Must be instance of AbstractProxy or NativeSessionHandler;
* implement \SessionHandlerInterface; or be null.
*
* @param object $saveHandler Default null means NativeProxy.
* @throws \InvalidArgumentException
*/
public function setSaveHandler($saveHandler = null)
{
// Wrap $saveHandler in proxy
if (!$saveHandler instanceof AbstractProxy &&
!$saveHandler instanceof NativeSessionHandler &&
!$saveHandler instanceof \SessionHandlerInterface &&
$saveHandler !== null) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

symfony standard is usually the reverse of this?
null !== $saveHandler

throw new \InvalidArgumentException('Must be instance of AbstractProxy or NativeSessionHandler; implement \SessionHandlerInterface; or be null');
}

// Wrap $saveHandler in proxy and prevent double wrapping of proxy
if (!$saveHandler instanceof AbstractProxy && $saveHandler instanceof \SessionHandlerInterface) {
$saveHandler = new SessionHandlerProxy($saveHandler);
} elseif (!$saveHandler instanceof AbstractProxy) {
$saveHandler = new NativeProxy();
$saveHandler = version_compare(phpversion(), '5.4.0', '>=') ?
new SessionHandlerProxy(new \SessionHandler()) : new NativeProxy();
}

$this->saveHandler = $saveHandler;

if ($this->saveHandler instanceof \SessionHandlerInterface) {
Expand Down
68 changes: 68 additions & 0 deletions src/Symfony/Component/HttpFoundation/Session/Storage/PhpSessionStorage.php
View file
Open in desktop
@@ -0,0 +1,68 @@
<?php

/*
* This file is part of the Symfony package.
*
* (c) Fabien Potencier <fabien@symfony.com>
*
* For the full copyright and license information, please view the LICENSE
* file that was distributed with this source code.
*/

namespace Symfony\Component\HttpFoundation\Session\Storage;

use Symfony\Component\HttpFoundation\Session\Storage\MetadataBag;

/**
* Allows session to be started by PHP and managed by Symfony2
*
* @author Drak <drak@zikula.org>
*/
class PhpSessionStorage extends NativeSessionStorage
{
/**
* Constructor.
*
* @param object|null $handler Must be instance of AbstractProxy or NativeSessionHandler;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Phpdoc allows those type information also here @param AbstractProxy|NativeSessionHandler|SessionHandlerInterface|null

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks really unweildly like that:

    /**
     * Constructor.
     *
     * @param AbstractProxy|NativeSessionHandler|\SessionHandlerInterface|null $handler
     * @param MetadataBag                                                      $metaBag MetadataBag.
     */
    public function __construct($handler = null, MetadataBag $metaBag = null)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, done.

* implement \SessionHandlerInterface; or be null.
* @param MetadataBag $metaBag MetadataBag.
*/
public function __construct($handler = null, MetadataBag $metaBag = null)
{
$this->setMetadataBag($metaBag);
$this->setSaveHandler($handler);
}

/**
* {@inheritdoc}
*/
public function start()
{
if ($this->started && !$this->closed) {
return true;
}

$this->loadSession();
if (!$this->saveHandler->isWrapper() && !$this->getSaveHandler()->isSessionHandlerInterface()) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here also getter at 2nd condition

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.

// This condition matches only PHP 5.3 + internal save handlers
$this->saveHandler->setActive(true);
}

return true;
}

/**
* {@inheritdoc}
*/
public function clear()
{
// clear out the bags and nothing else that may be set
// since the purpose of this driver is to share a handler
foreach ($this->bags as $bag) {
$bag->clear();
}

// reconnect the bags to the session
$this->loadSession();
}
}
15 changes: 15 additions & 0 deletions src/Symfony/Component/HttpFoundation/Session/Storage/Proxy/AbstractProxy.php
View file
Open in desktop
Expand Up @@ -72,16 +72,31 @@ public function isWrapper()
*/
public function isActive()
{
if (version_compare(phpversion(), '5.4.0', '>=')) {
return $this->active = \PHP_SESSION_ACTIVE === session_status();
}

return $this->active;
}

/**
* Sets the active flag.
*
* Has no effect under PHP 5.4+ as status is detected
* automatically in isActive()
*
* @internal
*
* @param Boolean $flag
*
* @throws \LogicException
*/
public function setActive($flag)
{
if (version_compare(phpversion(), '5.4.0', '>=')) {
throw new \LogicException('This method is disabled in PHP 5.4.0+');
}

$this->active = (bool) $flag;
}

Expand Down
120 changes: 112 additions & 8 deletions src/Symfony/Component/HttpFoundation/Tests/Session/Storage/NativeSessionStorageTest.php
View file
Open in desktop
Expand Up @@ -11,11 +11,15 @@

namespace Symfony\Component\HttpFoundation\Tests\Session\Storage;

use Symfony\Component\HttpFoundation\Session\Storage\Handler\NativeSessionHandler;
use Symfony\Component\HttpFoundation\Session\Storage\NativeSessionStorage;
use Symfony\Component\HttpFoundation\Session\Storage\Handler\NativeFileSessionHandler;
use Symfony\Component\HttpFoundation\Session\Storage\Handler\NullSessionHandler;
use Symfony\Component\HttpFoundation\Session\Flash\FlashBag;
use Symfony\Component\HttpFoundation\Session\Attribute\AttributeBag;
use Symfony\Component\HttpFoundation\Session\Storage\Proxy\AbstractProxy;
use Symfony\Component\HttpFoundation\Session\Storage\Proxy\NativeProxy;
use Symfony\Component\HttpFoundation\Session\Storage\Proxy\SessionHandlerProxy;

/**
* Test class for NativeSessionStorage.
Expand Down Expand Up @@ -126,32 +130,132 @@ public function testCookieOptions()
$this->assertEquals($options, $gco);
}

public function testSetSaveHandler()
/**
* @expectedException \InvalidArgumentException
*/
public function testSetSaveHandlerException()
{
$storage = $this->getStorage();
$storage->setSaveHandler(new \StdClass());
$this->assertInstanceOf('Symfony\Component\HttpFoundation\Session\Storage\Proxy\NativeProxy', $storage->getSaveHandler());
$storage->setSaveHandler(new \StdClass);
}

public function testSetSaveHandlerPHP53()
public function testSetSaveHandler53()
{
if (version_compare(phpversion(), '5.4.0', '>=')) {
$this->markTestSkipped('Test skipped, for PHP 5.3 only.');
}

ini_set('session.save_handler', 'files');
$storage = $this->getStorage();
$storage->setSaveHandler(new NativeFileSessionHandler());
$storage->setSaveHandler();
$this->assertInstanceOf('Symfony\Component\HttpFoundation\Session\Storage\Proxy\NativeProxy', $storage->getSaveHandler());
$storage->setSaveHandler(null);
$this->assertInstanceOf('Symfony\Component\HttpFoundation\Session\Storage\Proxy\NativeProxy', $storage->getSaveHandler());
$storage->setSaveHandler(new NativeSessionHandler());
$this->assertInstanceOf('Symfony\Component\HttpFoundation\Session\Storage\Proxy\NativeProxy', $storage->getSaveHandler());
$storage->setSaveHandler(new SessionHandlerProxy(new SessionHandler()));
$this->assertInstanceOf('Symfony\Component\HttpFoundation\Session\Storage\Proxy\SessionHandlerProxy', $storage->getSaveHandler());
$storage->setSaveHandler(new SessionHandler());
$this->assertInstanceOf('Symfony\Component\HttpFoundation\Session\Storage\Proxy\SessionHandlerProxy', $storage->getSaveHandler());
$storage->setSaveHandler(new NativeProxy());
$this->assertInstanceOf('Symfony\Component\HttpFoundation\Session\Storage\Proxy\NativeProxy', $storage->getSaveHandler());
}

public function testSetSaveHandlerPHP54()
public function testSetSaveHandler54()
{
if (version_compare(phpversion(), '5.4.0', '<')) {
$this->markTestSkipped('Test skipped, for PHP 5.4+ only.');
$this->markTestSkipped('Test skipped, for PHP 5.4 only.');
}

ini_set('session.save_handler', 'files');
$storage = $this->getStorage();
$storage->setSaveHandler(new NullSessionHandler());
$storage->setSaveHandler();
$this->assertInstanceOf('Symfony\Component\HttpFoundation\Session\Storage\Proxy\SessionHandlerProxy', $storage->getSaveHandler());
$storage->setSaveHandler(null);
$this->assertInstanceOf('Symfony\Component\HttpFoundation\Session\Storage\Proxy\SessionHandlerProxy', $storage->getSaveHandler());
$storage->setSaveHandler(new SessionHandlerProxy(new NativeSessionHandler()));
$this->assertInstanceOf('Symfony\Component\HttpFoundation\Session\Storage\Proxy\SessionHandlerProxy', $storage->getSaveHandler());
$storage->setSaveHandler(new NativeSessionHandler());
$this->assertInstanceOf('Symfony\Component\HttpFoundation\Session\Storage\Proxy\SessionHandlerProxy', $storage->getSaveHandler());
$storage->setSaveHandler(new SessionHandlerProxy(new SessionHandler()));
$this->assertInstanceOf('Symfony\Component\HttpFoundation\Session\Storage\Proxy\SessionHandlerProxy', $storage->getSaveHandler());
$storage->setSaveHandler(new SessionHandler());
$this->assertInstanceOf('Symfony\Component\HttpFoundation\Session\Storage\Proxy\SessionHandlerProxy', $storage->getSaveHandler());
}

/**
* @expectedException \RuntimeException
*/
public function testStartedOutside53()
{
if (version_compare(phpversion(), '5.4.0', '>=')) {
$this->markTestSkipped('Test skipped, for PHP 5.3 only.');
}

$storage = $this->getStorage();

$this->assertFalse(isset($_SESSION));

session_start();
$this->assertTrue(isset($_SESSION));
// PHP session might have started, but the storage driver has not, so false is correct here
$this->assertFalse($storage->isStarted());

$key = $storage->getMetadataBag()->getStorageKey();
$this->assertFalse(isset($_SESSION[$key]));
$storage->start();
}

/**
* @expectedException \RuntimeException
*/
public function testCanStartOutside54()
{
if (version_compare(phpversion(), '5.4.0', '<')) {
$this->markTestSkipped('Test skipped, for PHP 5.4 only.');
}

$storage = $this->getStorage();

$this->assertFalse(isset($_SESSION));
$this->assertFalse($storage->getSaveHandler()->isActive());
$this->assertFalse($storage->isStarted());

session_start();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should the session be destroyed so no sideffect are left behind by this test

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. I had meant to do that but forgot. Adding now.

$this->assertTrue(isset($_SESSION));
$this->assertTrue($storage->getSaveHandler()->isActive());
// PHP session might have started, but the storage driver has not, so false is correct here
$this->assertFalse($storage->isStarted());

$key = $storage->getMetadataBag()->getStorageKey();
$this->assertFalse(isset($_SESSION[$key]));
$storage->start();
}
}

class SessionHandler implements \SessionHandlerInterface
{
public function open($savePath, $sessionName)
{
}

public function close()
{
}

public function read($id)
{
}

public function write($id, $data)
{
}

public function destroy($id)
{
}

public function gc($maxlifetime)
{
}
}