Added redis cache driver #948

Merged
merged 10 commits into from Jun 9, 2012

7 participants

@mptre

A basic test suite is available in application/controllers/test_redis.php which hopefully will make merging easy.

@philsturgeon philsturgeon commented on an outdated diff Jan 21, 2012
system/libraries/Cache/drivers/Cache_redis.php
+ * @subpackage Libraries
+ * @category Core
+ * @author Anton Lindqvist <anton@qvister.se>
+ * @link
+ */
+class CI_Cache_redis extends CI_Driver
+{
+
+ /**
+ * Default config
+ *
+ * @access private
+ * @static
+ * @var array
+ */
+ private static $_default_config = array(
@philsturgeon
philsturgeon added a line comment Jan 21, 2012

We try and avoid setting anything as private as if somebody wants to extend this class they should be able to modify these attributes. Please use protected.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@philsturgeon philsturgeon commented on an outdated diff Jan 21, 2012
system/libraries/Cache/drivers/Cache_redis.php
+ *
+ * @access public
+ * @return void
+ */
+ public function __destruct()
+ {
+ if ($this->_redis)
+ {
+ $this->_redis->close();
+ }
+ }
+
+ /**
+ * Get cache
+ *
+ * @access public
@philsturgeon
philsturgeon added a line comment Jan 21, 2012

Can you remove these access lines? They are no longer needed as documentation systems since PHP 5 look at the actual scope below, not this comment.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@philsturgeon philsturgeon commented on the diff Jan 21, 2012
system/libraries/Cache/drivers/Cache_redis.php
+ * Check if Redis driver is supported
+ *
+ * @access public
+ * @return boolean
+ */
+ public function is_supported()
+ {
+ if (extension_loaded('redis'))
+ {
+ $this->_setup_redis();
+
+ return TRUE;
+ }
+ else
+ {
+ log_message(
@philsturgeon
philsturgeon added a line comment Jan 21, 2012

This probably doesn't need to be on multiple lines, it's pretty short.

@mptre
mptre added a line comment Jan 21, 2012

According to the PEAR standard which I tend to obey any line exceeding 80 characters should be split up. But you're the boss :)

@philsturgeon
philsturgeon added a line comment Jan 21, 2012

I'm not the boss, notice I said "probably" :)

I was always told to aim for around 120. This is 79 characters anyway ignoring tabs, so we're probably safe.

@mptre
mptre added a line comment Jan 21, 2012

Agree!

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

What do you guys think? @gaker @pkriete @derekjones Looks good from a standards point of view, but im no Redis buff.

@derekjones

I've not used it myself. Would this be better off as a Spark, at least until there is some community discernment and use? My only other comment would be the package author and copyright owner is the EllisLab Dev Team, not the "ExpressionEngine Dev Team", and the files are missing license attribution notice. Is this code clear of any existing licenses for inclusion?

@philsturgeon

@derekjones this will be down to this being a very old pull request that was on a fork of a fork of a fork of my old Reactor mirror. I asked @mptre to make this new pull request, but the doc blocks at the top will indeed need to be updated to match the new ones.

Also, can you delete the test controller? It's useful to have but shouldn't go in the merge.

Anton Lindqvist added some commits Jan 23, 2012
@myanimal

I think this will be a good addition to CI, but it doesn't appear to support authentication with the Redis server.

This could be achieved by adding a password field to the config, and if it is not empty perform an AUTH call in _setup_redis()?

Other than that, it looks like a good implementation to this Redis beginner.

@markhuot

This is amazing. I just tested and confirmed this works as expected on my system. Hope it gets merged sometime soon.

@philsturgeon

It will need to have conflicts resolved before I can merge it.

@markhuot

I'd be happy to help resolve them but I don't think I can see what is conflicting from my account. Anton, let me know if there's anything I can do to help.

@mptre

Will a simple git pull origin develop inside my fork do the trick?

@narfbg

@mptre Yes, that should do it.

Anton Lindqvist Merge branch 'develop' of https://github.com/EllisLab/CodeIgniter int…
…o develop

Conflicts:
	system/libraries/Cache/Cache.php
92f10e8
@philsturgeon philsturgeon commented on an outdated diff Apr 25, 2012
system/libraries/Cache/Cache.php
@@ -37,15 +37,16 @@
class CI_Cache extends CI_Driver_Library {
protected $valid_drivers = array(
- 'cache_apc',
@philsturgeon
philsturgeon added a line comment Apr 25, 2012

Can you make this array use logical indenting? You seem to be pushing it even further to the right.

protected $valid_drivers    = array(
    'cache_apc',
    'cache_file',
    'cache_memcached',
    'cache_dummy',
    'cache_wincache',
    'cache_redis',
);
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Anton Lindqvist added some commits Apr 25, 2012
@jjaffeux

Any news on this ?

@philsturgeon

Let's get this going. Can you merge upstream changes one more time as there are conflicts.

@philsturgeon philsturgeon reopened this Jun 7, 2012
Anton Lindqvist Merge branch 'develop' of https://github.com/EllisLab/CodeIgniter int…
…o develop

Conflicts:
	system/libraries/Cache/Cache.php
94c6b1f
@mptre

Hopefully the last merge :bowtie:

@narfbg narfbg commented on an outdated diff Jun 8, 2012
system/libraries/Cache/drivers/Cache_redis.php
@@ -0,0 +1,220 @@
+<?php if ( ! defined('BASEPATH')) exit('No direct script access allowed');
+/**
+ * CodeIgniter
+ *
+ * An open source application development framework for PHP 5.1.6 or newer
@narfbg
narfbg added a line comment Jun 8, 2012

The PHP version requirement was changed to 5.2.4, so is this docblock changed in each file.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@narfbg narfbg commented on an outdated diff Jun 8, 2012
system/libraries/Cache/drivers/Cache_redis.php
+ * @category Core
+ * @author Anton Lindqvist <anton@qvister.se>
+ * @link
+ */
+class CI_Cache_redis extends CI_Driver
+{
+
+ /**
+ * Default config
+ *
+ * @static
+ * @var array
+ */
+ protected static $_default_config = array(
+ 'host' => '127.0.0.1',
+ 'password' => null,
@narfbg
narfbg added a line comment Jun 8, 2012

null => NULL

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@narfbg narfbg commented on an outdated diff Jun 8, 2012
system/libraries/Cache/drivers/Cache_redis.php
+
+ return FALSE;
+ }
+
+ }
+
+ /**
+ * Setup Redis config and connection
+ *
+ * Loads Redis config file if present. Will halt execution if a Redis connection
+ * can't be established.
+ *
+ * @return void
+ * @see Redis::connect()
+ */
+ private function _setup_redis()
@narfbg
narfbg added a line comment Jun 8, 2012

This should be protected.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@narfbg narfbg commented on an outdated diff Jun 8, 2012
system/libraries/Cache/drivers/Cache_redis.php
+ }
+
+ $config = array_merge(self::$_default_config, $config);
+
+ $this->_redis = new Redis();
+
+ try
+ {
+ $this->_redis->connect($config['host'], $config['port'], $config['timeout']);
+ }
+ catch (RedisException $e)
+ {
+ show_error('Redis connection refused. ' . $e->getMessage());
+ }
+
+ if (isset($config['password'])) {
@narfbg
narfbg added a line comment Jun 8, 2012

Put { on the next line.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@narfbg narfbg commented on an outdated diff Jun 8, 2012
system/libraries/Cache/drivers/Cache_redis.php
+ /**
+ * Setup Redis config and connection
+ *
+ * Loads Redis config file if present. Will halt execution if a Redis connection
+ * can't be established.
+ *
+ * @return void
+ * @see Redis::connect()
+ */
+ private function _setup_redis()
+ {
+ $config = array();
+ $CI =& get_instance();
+
+ if ($CI->config->load('redis', TRUE, TRUE))
+ {
@narfbg
narfbg added a line comment Jun 8, 2012

This looks weird ... if you're using spaces - switch to tabs. :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@narfbg narfbg merged commit ba9350f into bcit-ci:develop Jun 9, 2012
@narfbg

@mptre Could you give a link to the Redis PHP extension?

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