diogo/symfony
Archived
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

merged branch vicb/session_nnhandlers (PR #4227)

Browse Source 
Commits
-------

12e22c0 [Session] Memcache/d cleanup, test improvements
788adfb [Session] Pdo Handler cleanup
0216e05 [HttpFoundation][Session] Assume that memcache(d) instances are already configured
72d21c6 [HttpFoundation][Session] change possible replace() & set() for set only()

Discussion
----------

[Session] Non-native Session handlers

A few item to discuss. Needs @drak inputs.

* 72d21c66 is trivial,
* 0216e056 is about memcache(d) handlers
    * I don't think the handlers should configure the memcache(d) instances. Those instances are injected into the storage so they should already be confidured (this will be done in the CacheBundle when available)
    * A SW prefix has been added to the memcached handlers so that the same instance of memcached can be shared - you can still set the `Memcached::OPT_PREFIX_KEY` before injecting the memcached instance.
    * It was not possible to use an expiration > 30days before, see [php.net](http://www.php.net/manual/en/memcached.expiration.php)
* 788adfb6 is trivial (cleanup in the PDO handler)

---------------------------------------------------------------------------

by travisbot at 2012-05-08T09:49:03Z

This pull request [passes](http://travis-ci.org/symfony/symfony/builds/1274808) (merged 788adfb6 into e54f4e46).

---------------------------------------------------------------------------

by drak at 2012-05-09T15:20:38Z

Overall this PR looks good to me. Since Memcache/d objects are passed by DI anyway, there is no need to provide a way to configure the objects here.

However, I am not sure it's consistent to provide internal handling of the prefix/expire if we are saying the objects should be configured and injected - if we hand over all configuration to the injected objects, that's exactly what we should do. In the case of the `Memcache` handler there is no handling for prefix by the Memcache object that is why it's handled internally.

Unless there are some other technical consideration I've missed, I would also not expect the same Memcache/d object to be used in all use cases (e.g. session storage and database caching layer).   I realise we are trying to unify things in one cache component, but I am not entirely convinced session storage would necessarily have to be part of that nor that "one object fits all" is practical or wise.

As far as I am aware, apart from default settings, memcache/memcached instances retain their own settings once configured so it's quite feasible to expect there might be a couple of differently configured instances in a complex system.

In summary, I would remove the `$memcachedOptions` config entirely from the `MemcachedHander` along with the associated prefix and time and let it all be configured by the injected `Memcached` instance.

---------------------------------------------------------------------------

by travisbot at 2012-05-10T07:32:53Z

This pull request [passes](http://travis-ci.org/symfony/symfony/builds/1293064) (merged 12e22c0d into e54f4e46).

---------------------------------------------------------------------------

by vicb at 2012-05-10T07:34:31Z

@drak thanks for your feeback.

About the prefix: it might be necessary to avoid collisions when you re-use the same instance of `memcache/d`. This is why the prefix is handled internally and not by `memcached` (it would be global and not serve the purpose then).

About the ttl:

* `memcache/d` can not handle ttl > 30 days (they would consider the time as an absolute timestamp then) and this is why the PR always convert the ttl to an absolute ts (`time() + $ttl`)
* Moreover I think that the ttl should be initialized by the `Session`: there is no reason why the ttl should be different from the `gc_maxlifetime`. I think this is out of the scope of this PR.

About sharing `memcache/d ` instances: it will be possible but it does not mean that you have to, you still can use different instances if this suit your needs.

The tests have been improved.

If you are ok with the latest changes, this PR should be ready to be merged

---------------------------------------------------------------------------

by drak at 2012-05-10T09:29:18Z

@vicb - I think it's ok to merge now. You are right about the TTL since PHP will pass a maxlifetime not a timestamp, and since memcached varies how it treats $expire, it does need to be normalised in the handler. I'm not necessarily 100% convinced about the prefix, but I don't object.  Nice work.

/cc @fabpot
This commit is contained in:
Fabien Potencier 2012-05-10 11:47:04 +02:00
commit 28d045ed48
5 changed files with 169 additions and 188 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

86
src/Symfony/Component/HttpFoundation/Session/Storage/Handler/MemcacheSessionHandler.php
View File

@ -19,85 +19,55 @@ namespace Symfony\Component\HttpFoundation\Session\Storage\Handler;
class MemcacheSessionHandler implements \SessionHandlerInterface
{
/**
* Memcache driver.
*
* @var \Memcache
* @var \Memcache Memcache driver.
*/
private $memcache;
/**
* Configuration options.
*
* @var array
* @var integer Time to live in seconds
*/
private $memcacheOptions;
private $ttl;
/**
* Key prefix for shared environments.
*
* @var string
* @var string Key prefix for shared environments.
*/
private $prefix;
/**
* Constructor.
*
* @param \Memcache $memcache A \Memcache instance
* @param array $memcacheOptions An associative array of Memcache options
* @param array $options Session configuration options.
* List of available options:
* * prefix: The prefix to use for the memcache keys in order to avoid collision
* * expiretime: The time to live in seconds
*
* @param \Memcache $memcache A \Memcache instance
* @param array $options An associative array of Memcache options
*
* @throws \InvalidArgumentException When unsupported options are passed
*/
public function __construct(\Memcache $memcache, array $memcacheOptions = array(), array $options = array())
public function __construct(\Memcache $memcache, array $options = array())
{
$this->memcache = $memcache;
// defaults
if (!isset($memcacheOptions['serverpool'])) {
$memcacheOptions['serverpool'] = array(array(
'host' => '127.0.0.1',
'port' => 11211,
'timeout' => 1,
'persistent' => false,
'weight' => 1,
'retry_interval' => 15,
if ($diff = array_diff(array_keys($options), array('prefix', 'expiretime'))) {
throw new \InvalidArgumentException(sprintf(
'The following options are not supported "%s"', implode(', ', $diff)
));
}
$memcacheOptions['expiretime'] = isset($memcacheOptions['expiretime']) ? (int)$memcacheOptions['expiretime'] : 86400;
$this->prefix = isset($memcacheOptions['prefix']) ? $memcacheOptions['prefix'] : 'sf2s';
$this->memcacheOptions = $memcacheOptions;
}
protected function addServer(array $server)
{
if (!array_key_exists('host', $server)) {
throw new \InvalidArgumentException('host key must be set');
}
$server['port'] = isset($server['port']) ? (int)$server['port'] : 11211;
$server['timeout'] = isset($server['timeout']) ? (int)$server['timeout'] : 1;
$server['persistent'] = isset($server['persistent']) ? (bool)$server['persistent'] : false;
$server['weight'] = isset($server['weight']) ? (int)$server['weight'] : 1;
$server['retry_interval'] = isset($server['retry_interval']) ? (int)$server['retry_interval'] : 15;
$this->memcache->addserver($server['host'], $server['port'], $server['persistent'],$server['weight'],$server['timeout'],$server['retry_interval']);
$this->memcache = $memcache;
$this->ttl = isset($options['expiretime']) ? (int) $options['expiretime'] : 86400;
$this->prefix = isset($options['prefix']) ? $options['prefix'] : 'sf2s';
}
/**
* {@inheritdoc}
* {@inheritDoc}
*/
public function open($savePath, $sessionName)
{
foreach ($this->memcacheOptions['serverpool'] as $server) {
$this->addServer($server);
}
return true;
}
/**
* {@inheritdoc}
* {@inheritDoc}
*/
public function close()
{
@ -105,7 +75,7 @@ class MemcacheSessionHandler implements \SessionHandlerInterface
}
/**
* {@inheritdoc}
* {@inheritDoc}
*/
public function read($sessionId)
{
@ -113,19 +83,15 @@ class MemcacheSessionHandler implements \SessionHandlerInterface
}
/**
* {@inheritdoc}
* {@inheritDoc}
*/
public function write($sessionId, $data)
{
if (!$this->memcache->replace($this->prefix.$sessionId, $data, 0, $this->memcacheOptions['expiretime'])) {
return $this->memcache->set($this->prefix.$sessionId, $data, 0, $this->memcacheOptions['expiretime']);
}
return true;
return $this->memcache->set($this->prefix.$sessionId, $data, 0, time() + $this->ttl);
}
/**
* {@inheritdoc}
* {@inheritDoc}
*/
public function destroy($sessionId)
{
@ -133,7 +99,7 @@ class MemcacheSessionHandler implements \SessionHandlerInterface
}
/**
* {@inheritdoc}
* {@inheritDoc}
*/
public function gc($lifetime)
{

82
src/Symfony/Component/HttpFoundation/Session/Storage/Handler/MemcachedSessionHandler.php
View File

@ -24,55 +24,57 @@ namespace Symfony\Component\HttpFoundation\Session\Storage\Handler;
class MemcachedSessionHandler implements \SessionHandlerInterface
{
/**
* Memcached driver.
*
* @var \Memcached
* @var \Memcached Memcached driver.
*/
private $memcached;
/**
* Configuration options.
*
* @var array
* @var integer Time to live in seconds
*/
private $memcachedOptions;
private $ttl;
/**
* @var string Key prefix for shared environments.
*/
private $prefix;
/**
* Constructor.
*
* @param \Memcached $memcached A \Memcached instance
* @param array $memcachedOptions An associative array of Memcached options
* @param array $options Session configuration options.
* List of available options:
* * prefix: The prefix to use for the memcached keys in order to avoid collision
* * expiretime: The time to live in seconds
*
* @param \Memcached $memcached A \Memcached instance
* @param array $options An associative array of Memcached options
*
* @throws \InvalidArgumentException When unsupported options are passed
*/
public function __construct(\Memcached $memcached, array $memcachedOptions = array(), array $options = array())
public function __construct(\Memcached $memcached, array $options = array())
{
$this->memcached = $memcached;
// defaults
if (!isset($memcachedOptions['serverpool'])) {
$memcachedOptions['serverpool'][] = array(
'host' => '127.0.0.1',
'port' => 11211,
'weight' => 1);
if ($diff = array_diff(array_keys($options), array('prefix', 'expiretime'))) {
throw new \InvalidArgumentException(sprintf(
'The following options are not supported "%s"', implode(', ', $diff)
));
}
$memcachedOptions['expiretime'] = isset($memcachedOptions['expiretime']) ? (int)$memcachedOptions['expiretime'] : 86400;
$this->memcached->setOption(\Memcached::OPT_PREFIX_KEY, isset($memcachedOptions['prefix']) ? $memcachedOptions['prefix'] : 'sf2s');
$this->memcachedOptions = $memcachedOptions;
$this->ttl = isset($options['expiretime']) ? (int) $options['expiretime'] : 86400;
$this->prefix = isset($options['prefix']) ? $options['prefix'] : 'sf2s';
}
/**
* {@inheritdoc}
* {@inheritDoc}
*/
public function open($savePath, $sessionName)
{
return $this->memcached->addServers($this->memcachedOptions['serverpool']);
return true;
}
/**
* {@inheritdoc}
* {@inheritDoc}
*/
public function close()
{
@ -80,51 +82,35 @@ class MemcachedSessionHandler implements \SessionHandlerInterface
}
/**
* {@inheritdoc}
* {@inheritDoc}
*/
public function read($sessionId)
{
return $this->memcached->get($sessionId) ?: '';
return $this->memcached->get($this->prefix.$sessionId) ?: '';
}
/**
* {@inheritdoc}
* {@inheritDoc}
*/
public function write($sessionId, $data)
{
return $this->memcached->set($sessionId, $data, $this->memcachedOptions['expiretime']);
return $this->memcached->set($this->prefix.$sessionId, $data, time() + $this->ttl);
}
/**
* {@inheritdoc}
* {@inheritDoc}
*/
public function destroy($sessionId)
{
return $this->memcached->delete($sessionId);
return $this->memcached->delete($this->prefix.$sessionId);
}
/**
* {@inheritdoc}
* {@inheritDoc}
*/
public function gc($lifetime)
{
// not required here because memcached will auto expire the records anyhow.
return true;
}
/**
* Adds a server to the memcached handler.
*
* @param array $server
*/
protected function addServer(array $server)
{
if (array_key_exists('host', $server)) {
throw new \InvalidArgumentException('host key must be set');
}
$server['port'] = isset($server['port']) ? (int)$server['port'] : 11211;
$server['timeout'] = isset($server['timeout']) ? (int)$server['timeout'] : 1;
$server['presistent'] = isset($server['presistent']) ? (bool)$server['presistent'] : false;
$server['weight'] = isset($server['weight']) ? (bool)$server['weight'] : 1;
}
}

27
src/Symfony/Component/HttpFoundation/Session/Storage/Handler/PdoSessionHandler.php
View File

@ -39,11 +39,10 @@ class PdoSessionHandler implements \SessionHandlerInterface
*
* @param \PDO $pdo A \PDO instance
* @param array $dbOptions An associative array of DB options
* @param array $options Session configuration options
*
* @throws \InvalidArgumentException When "db_table" option is not provided
*/
public function __construct(\PDO $pdo, array $dbOptions = array(), array $options = array())
public function __construct(\PDO $pdo, array $dbOptions = array())
{
if (!array_key_exists('db_table', $dbOptions)) {
throw new \InvalidArgumentException('You must provide the "db_table" option for a PdoSessionStorage.');
@ -58,7 +57,7 @@ class PdoSessionHandler implements \SessionHandlerInterface
}
/**
* {@inheritdoc}
* {@inheritDoc}
*/
public function open($path, $name)
{
@ -66,7 +65,7 @@ class PdoSessionHandler implements \SessionHandlerInterface
}
/**
* {@inheritdoc}
* {@inheritDoc}
*/
public function close()
{
@ -74,12 +73,12 @@ class PdoSessionHandler implements \SessionHandlerInterface
}
/**
* {@inheritdoc}
* {@inheritDoc}
*/
public function destroy($id)
{
// get table/column
$dbTable = $this->dbOptions['db_table'];
$dbTable = $this->dbOptions['db_table'];
$dbIdCol = $this->dbOptions['db_id_col'];
// delete the record associated with this id
@ -97,20 +96,20 @@ class PdoSessionHandler implements \SessionHandlerInterface
}
/**
* {@inheritdoc}
* {@inheritDoc}
*/
public function gc($lifetime)
{
// get table/column
$dbTable = $this->dbOptions['db_table'];
$dbTable = $this->dbOptions['db_table'];
$dbTimeCol = $this->dbOptions['db_time_col'];
// delete the session records that have expired
$sql = "DELETE FROM $dbTable WHERE $dbTimeCol < (:time - $lifetime)";
$sql = "DELETE FROM $dbTable WHERE $dbTimeCol < :time";
try {
$stmt = $this->pdo->prepare($sql);
$stmt->bindValue(':time', time(), \PDO::PARAM_INT);
$stmt->bindValue(':time', time() - $lifetime, \PDO::PARAM_INT);
$stmt->execute();
} catch (\PDOException $e) {
throw new \RuntimeException(sprintf('PDOException was thrown when trying to manipulate session data: %s', $e->getMessage()), 0, $e);
@ -120,12 +119,12 @@ class PdoSessionHandler implements \SessionHandlerInterface
}
/**
* {@inheritdoc}
* {@inheritDoc}
*/
public function read($id)
{
// get table/columns
$dbTable = $this->dbOptions['db_table'];
$dbTable = $this->dbOptions['db_table'];
$dbDataCol = $this->dbOptions['db_data_col'];
$dbIdCol = $this->dbOptions['db_id_col'];
@ -154,7 +153,7 @@ class PdoSessionHandler implements \SessionHandlerInterface
}
/**
* {@inheritdoc}
* {@inheritDoc}
*/
public function write($id, $data)
{
@ -201,7 +200,7 @@ class PdoSessionHandler implements \SessionHandlerInterface
private function createNewSession($id, $data = '')
{
// get table/column
$dbTable = $this->dbOptions['db_table'];
$dbTable = $this->dbOptions['db_table'];
$dbDataCol = $this->dbOptions['db_data_col'];
$dbIdCol = $this->dbOptions['db_id_col'];
$dbTimeCol = $this->dbOptions['db_time_col'];

102
src/Symfony/Component/HttpFoundation/Tests/Session/Storage/Handler/MemcacheSessionHandlerTest.php
View File

@ -15,6 +15,8 @@ use Symfony\Component\HttpFoundation\Session\Storage\Handler\MemcacheSessionHand
class MemcacheSessionHandlerTest extends \PHPUnit_Framework_TestCase
{
const PREFIX = 'prefix_';
const TTL = 1000;
/**
* @var MemcacheSessionHandler
*/
@ -29,7 +31,10 @@ class MemcacheSessionHandlerTest extends \PHPUnit_Framework_TestCase
}
$this->memcache = $this->getMock('Memcache');
$this->storage = new MemcacheSessionHandler($this->memcache);
$this->storage = new MemcacheSessionHandler(
$this->memcache,
array('prefix' => self::PREFIX, 'expiretime' => self::TTL)
);
}
protected function tearDown()
@ -40,82 +45,53 @@ class MemcacheSessionHandlerTest extends \PHPUnit_Framework_TestCase
public function testOpenSession()
{
$this->memcache->expects($this->atLeastOnce())
->method('addServer')
->with('127.0.0.1', 11211, false, 1, 1, 15);
$this->assertTrue($this->storage->open('', ''));
}
public function testConstructingWithServerPool()
{
$mock = $this->getMock('Memcache');
$storage = new MemcacheSessionHandler($mock, array(
'serverpool' => array(
array('host' => '127.0.0.2'),
array('host' => '127.0.0.3',
'port' => 11212,
'timeout' => 10,
'persistent' => true,
'weight' => 5,
'retry_interval' => 39,
),
array('host' => '127.0.0.4',
'port' => 11211,
'weight' => 2
),
),
));
$matcher = $mock
->expects($this->at(0))
->method('addServer')
->with('127.0.0.2', 11211, false, 1, 1, 15);
$matcher = $mock
->expects($this->at(1))
->method('addServer')
->with('127.0.0.3', 11212, true, 5, 10, 39);
$matcher = $mock
->expects($this->at(2))
->method('addServer')
->with('127.0.0.4', 11211, false, 2, 1, 15);
$this->assertTrue($storage->open('', ''));
}
public function testCloseSession()
{
$this->memcache->expects($this->once())
$this->memcache
->expects($this->once())
->method('close')
->will($this->returnValue(true));
->will($this->returnValue(true))
;
$this->assertTrue($this->storage->close());
}
public function testReadSession()
{
$this->memcache->expects($this->once())
->method('get');
$this->memcache
->expects($this->once())
->method('get')
->with(self::PREFIX.'id')
;
$this->assertEquals('', $this->storage->read(''));
$this->assertEquals('', $this->storage->read('id'));
}
public function testWriteSession()
{
$this->memcache->expects($this->once())
$this->memcache
->expects($this->once())
->method('set')
->will($this->returnValue(true));
->with(self::PREFIX.'id', 'data', 0, $this->equalTo(time() + self::TTL, 2))
->will($this->returnValue(true))
;
$this->assertTrue($this->storage->write('', ''));
$this->assertTrue($this->storage->write('id', 'data'));
}
public function testDestroySession()
{
$this->memcache->expects($this->once())
$this->memcache
->expects($this->once())
->method('delete')
->will($this->returnValue(true));
->with(self::PREFIX.'id')
->will($this->returnValue(true))
;
$this->assertTrue($this->storage->destroy(''));
$this->assertTrue($this->storage->destroy('id'));
}
public function testGcSession()
@ -123,4 +99,26 @@ class MemcacheSessionHandlerTest extends \PHPUnit_Framework_TestCase
$this->assertTrue($this->storage->gc(123));
}
/**
* @dataProvider getOptionFixtures
*/
public function testSupportedOptions($options, $supported)
{
try {
new MemcacheSessionHandler($this->memcache, $options);
$this->assertTrue($supported);
} catch (\InvalidArgumentException $e) {
$this->assertFalse($supported);
}
}
public function getOptionFixtures()
{
return array(
array(array('prefix' => 'session'), true),
array(array('expiretime' => 100), true),
array(array('prefix' => 'session', 'expiretime' => 200), true),
array(array('expiretime' => 100, 'foo' => 'bar'), false),
);
}
}

60
src/Symfony/Component/HttpFoundation/Tests/Session/Storage/Handler/MemcachedSessionHandlerTest.php
View File

@ -15,6 +15,9 @@ use Symfony\Component\HttpFoundation\Session\Storage\Handler\MemcachedSessionHan
class MemcacheddSessionHandlerTest extends \PHPUnit_Framework_TestCase
{
const PREFIX = 'prefix_';
const TTL = 1000;
/**
* @var MemcachedSessionHandler
*/
@ -29,7 +32,10 @@ class MemcacheddSessionHandlerTest extends \PHPUnit_Framework_TestCase
}
$this->memcached = $this->getMock('Memcached');
$this->storage = new MemcachedSessionHandler($this->memcached);
$this->storage = new MemcachedSessionHandler(
$this->memcached,
array('prefix' => self::PREFIX, 'expiretime' => self::TTL)
);
}
protected function tearDown()
@ -40,10 +46,6 @@ class MemcacheddSessionHandlerTest extends \PHPUnit_Framework_TestCase
public function testOpenSession()
{
$this->memcached->expects($this->atLeastOnce())
->method('addServers')
->will($this->returnValue(true));
$this->assertTrue($this->storage->open('', ''));
}
@ -54,28 +56,37 @@ class MemcacheddSessionHandlerTest extends \PHPUnit_Framework_TestCase
public function testReadSession()
{
$this->memcached->expects($this->once())
->method('get');
$this->memcached
->expects($this->once())
->method('get')
->with(self::PREFIX.'id')
;
$this->assertEquals('', $this->storage->read(''));
$this->assertEquals('', $this->storage->read('id'));
}
public function testWriteSession()
{
$this->memcached->expects($this->once())
$this->memcached
->expects($this->once())
->method('set')
->will($this->returnValue(true));
->with(self::PREFIX.'id', 'data', $this->equalTo(time() + self::TTL, 2))
->will($this->returnValue(true))
;
$this->assertTrue($this->storage->write('', ''));
$this->assertTrue($this->storage->write('id', 'data'));
}
public function testDestroySession()
{
$this->memcached->expects($this->once())
$this->memcached
->expects($this->once())
->method('delete')
->will($this->returnValue(true));
->with(self::PREFIX.'id')
->will($this->returnValue(true))
;
$this->assertTrue($this->storage->destroy(''));
$this->assertTrue($this->storage->destroy('id'));
}
public function testGcSession()
@ -83,5 +94,26 @@ class MemcacheddSessionHandlerTest extends \PHPUnit_Framework_TestCase
$this->assertTrue($this->storage->gc(123));
}
/**
* @dataProvider getOptionFixtures
*/
public function testSupportedOptions($options, $supported)
{
try {
new MemcachedSessionHandler($this->memcached, $options);
$this->assertTrue($supported);
} catch (\InvalidArgumentException $e) {
$this->assertFalse($supported);
}
}
public function getOptionFixtures()
{
return array(
array(array('prefix' => 'session'), true),
array(array('expiretime' => 100), true),
array(array('prefix' => 'session', 'expiretime' => 200), true),
array(array('expiretime' => 100, 'foo' => 'bar'), false),
);
}
}