Add phpredis FeatureRequester

[nicofff]

Requirements

• I have added test coverage for new or changed functionality
• I have followed the repository's pull request submission guidelines
• I have validated my changes against all supported platform versions

Describe the solution you've provided

This PR adds PHPRedis as a Feature Requester option.
The goal behind this is to allow the library to use persistent connections, avoiding the overhead of reconnecting to the Redis server on each request.

[nicofff]

The phpredis extension doesn't support php 5.x. Would you be interested in only supporting this feature for php 7+? If so, should I find a way to skip this test on php < 7?

[eli-darkly]

To make this fail a little more gracefully if the extension is not installed, could you please add something like this:

        if (!extension_loaded('phpredis')) {
            throw new \RuntimeException("phpredis extension is required to use Integrations::PHPRedis");
        }

[nicofff]

Fixed

[eli-darkly]

Similar to previous comment: even though I know it's possible to set an environment variable to skip all of the database tests, I don't think it's a good idea to have a unit test class that can't possibly work unless a specific extension is installed (since installing things with pecl is not something that is always possible in any given environment). So I think I would prefer to have some logic here that skips these tests if phpredis is not installed. This should work:

    public static function setUpBeforeClass()
    {
        if (!static::isSkipDatabaseTests() && extension_loaded('phpredis')) {
            $redis = new \Redis();
            $redis->connect('127.0.0.1', 6379);
            self::$redisClient = $redis;
        }
    }

    public function setUp()
    {
        if (!static::isSkipDatabaseTests() && !extension_loaded('phpredis')) {
            $this->markTestSkipped('phpredis is not installed');
        } else {
            parent::$setup();
        }
    }

[nicofff]

I did use @require to try to limit the test to only PHP 7+, but now that you mention it, checking if the extension is installed if probably a better choice.
I just pushed the change on the @require, since that would avoid running the test altogether, but I'm open to using your suggestion if you think it's better.

[eli-darkly]

I wasn't sure if @require was supported in this old version of phpunit, but it looks like it is so that's fine.

[eli-darkly]

I just noticed that the CI step you added to install the extension is being run in the 5.6 build as well as the 7.x ones, even though it can't work in 5.6. But I was planning to do some refactoring of the CI script anyway so I can fix that after merging this; it's not actually breaking the build (though I'm not sure why it's not).

[eli-darkly]

This is included in the 3.7.0 release.

[nicofff]

Thanks! Do you know when will it show up in packagist?

[eli-darkly]

@nicofff That's odd - normally packagist picks up our releases automatically. I just forced an update so it should be available now.

[guillaumewuip]

@eli-darkly @nicofff, fyi we can do persistent connections with predis already.

What I'm doing with a previous version of launchdarkly/php-server-sdk is

$predisClient = new Predis\Client([
            'scheme'             => 'tcp',
            'host'               => 'redis_host',
            'port'               => 'redis_port',
            'timeout'            => 2.0,
            'read_write_timeout' => 2.0,
            'persistent'         => true // this is the option to control persistent connections
        ]);

        $launchDarklyClient = new LDClient(
            'sdk-key',
            [
                'feature_requester' => Redis::featureRequester([
                    'predis_client' => $predisClient,
                    'redis_prefix'  => 'redis_prefix'
                ]),
                'event_publisher' => Guzzle::eventPublisher([
                    'events_uri' => 'events_uri'
                ])
            ]
        );

So I think the 3.7 changelog entry is not totally correct

[3.7.0] - 2019-12-13

Added:

• Added integration with the phpredis extension, which supports persistent Redis connections unlike the existing predis integration. See LaunchDarkly::Integrations::PHPRedis. (Thanks, nicofff!)

[eli-darkly]

@guillaumewuip True. We had not tried to use it that way before, so we were actually unaware that Predis had such a parameter; we took @nicofff's word for it.

[nicofff]

I think it was a case of blindness by previous assumptions. We've always used phpredis, and when we migrated to redis-cluster, I did some testing of both libraries and couldn't get predis to use persistent connections. Maybe it is (was?) a limitation just on cluster connections?
Anyways, I agree with you @guillaumewuip, the release note is misleading. I'd be happy to remediate it, though not sure what's the best option is @eli-darkly

[eli-darkly]

I don't think the changelog verbiage is of huge importance; we can fix it in the next release. No one is going to be looking to the LaunchDarkly SDK release notes to find out whether Predis has this feature.