[3.x] Build custom Redis client based on the installed extension

[phansys]

Q	A
Branch	master
Bug fix?	no
New feature?	no
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets	#399
License	MIT
Doc PR	n/a

Fixes #399.

This PR reverts the changes made at #405 and #487.

[phansys]

@B-Galati, how do you propose to load the cache generated class? Currently, I'm generating the file as snc_redis_client.php inside the cache dir:

SncRedisBundle/Client/Phpredis/ClientCacheWarmer.php

Line 41 in e1da2fd

file_put_contents($filename, $this->clientBuilder->getClassContents());

[B-Galati]

Good question.
Could be done with a custom autoloader like doctrine is doing.
You will need to double check because I am not 💯 sure.

[B-Galati]

@curry684 Any advice about the implementation? It looks like you thought about it a lot in the past.

[B-Galati]

To detect the phpredis usage, it could be a parameter set by the extension as soon as a phpredis client is declared like here

SncRedisBundle/DependencyInjection/SncRedisExtension.php

Lines 247 to 295 in 521d237

	protected function loadPhpredisClient(array $client, ContainerBuilder $container)
	{
	$connectionCount = count($client['dsns']);
	if (1 !== $connectionCount) {
	throw new \RuntimeException('Support for RedisArray is not yet implemented.');
	}
	/** @var \Snc\RedisBundle\DependencyInjection\Configuration\RedisDsn $dsn */
	$dsn = $client['dsns'][0];
	$phpredisId = sprintf('snc_redis.%s', $client['alias']);
	$phpRedisVersion = phpversion('redis');
	if (version_compare($phpRedisVersion, '4.0.0') >= 0 && $client['logging']) {
	$client['logging'] = false;
	@trigger_error(sprintf('Redis logging is not supported on PhpRedis %s and has been automatically disabled, disable logging in config to suppress this warning', $phpRedisVersion), E_USER_WARNING);
	}
	$phpredisClientclass = $container->getParameter('snc_redis.phpredis_client.class');
	if ($client['logging']) {
	$phpredisClientclass = $container->getParameter('snc_redis.phpredis_connection_wrapper.class');
	}
	$phpredisDef = new Definition($phpredisClientclass);
	$phpredisDef->setFactory(array(
	new Definition(PhpredisClientFactory::class, [new Reference('snc_redis.logger')]),
	'create'
	));
	$phpredisDef->addArgument($phpredisClientclass);
	$phpredisDef->addArgument((string) $dsn);
	$phpredisDef->addArgument($client['options']);
	$phpredisDef->addArgument($client['alias']);
	$phpredisDef->addTag('snc_redis.client', array('alias' => $client['alias']));
	$phpredisDef->setPublic(false);
	// Older version of phpredis extension do not support lazy loading
	$minimumVersionForLazyLoading = '4.1.1';
	$supportsLazyServices = version_compare($phpRedisVersion, $minimumVersionForLazyLoading, '>=');
	$phpredisDef->setLazy($supportsLazyServices);
	if (!$supportsLazyServices) {
	@trigger_error(
	sprintf('Lazy loading Redis is not supported on PhpRedis %s. Please update to PhpRedis %s or higher.', $phpRedisVersion, $minimumVersionForLazyLoading),
	E_USER_WARNING
	);
	}
	$container->setDefinition($phpredisId, $phpredisDef);
	$container->setAlias(sprintf('snc_redis.phpredis.%s', $client['alias']), new Alias($phpredisId, true));
	$container->setAlias(sprintf('snc_redis.%s_client', $client['alias']), $phpredisId);
	}

[phansys]

The suggested changes were addressed, I think. Now I'm facing 2 new issues:

• The cache seems to be not cleared after the test CustomPhpredisClientTest, causing other tests to fail;
• Installing Redis 4.3.0 is causing segfault at Travis builds.

[B-Galati]

@phansys Looks like you fix them! Nice! What were the issues?

[phansys]

These are the changes I've done to solve the described issues:

• Check against the class existence regardless the cache file exists or not;
• Bundle the Redis library before installing it (pecl bundle redis), avoiding to force "yes" for all the prompts triggered when running pecl install redis. Maybe some of the flags was causing the segfault.
  Now I must try to solve the issues with the client for 3.1.6 version.

[phansys]

I think everything is working now @B-Galati. Thank you for your support!

[B-Galati]

@phansys my pleasure!

I'll play with it and give you some feedback!
This feature is gonna be awesome 👍

@curry684 Would you have time to check it out too? The more we review the better ;-)

[B-Galati]

@phansys What about using symfony/proxy-manager-bridge instead of creating our own logic?

[phansys]

@phansys What about using symfony/proxy-manager-bridge instead of creating our own logic?

Since we need to intercept each call to Redis' methods, I think we should use AccessInterceptorValueHolderFactory; but I don't know how to achieve that with symfony/proxy-manager-bridge.
I've never developed a proxy using this library. Could you please point me in the right direction?

Thank you.

[B-Galati]

@phansys Sorry i never used it either :/

But I see 2 benefits, the underlying lib (Ocramius/ProxyManager) is battle tested and it will make less complicated code to maintain.

(I could be missing some points that would make the usage of this lib impossible or hard)

[qharnay]

Hi, it's been a while since last comment ... Is there any news of acceptance for this PR ?

Cheers

[B-Galati]

Hey, I am not sure about the maintenance cost of this solution.

I am currently not having time to look at such a big feature and maintain it after the release.

What's your opinion @curry684?

[curry684]

I am currently not having time to look at such a big feature and maintain it after the release.

Same issue here... I think the intended direction is the right way (as discussed long ago) but indeed it's far from trivial to both develop and maintain, and depending on phpredis itself it may either be fragile or more resilient than what we have now.

If we go forward on this I would most certainly prefer using the proxy manager as that's indeed battle tested and well maintained.

[ostrolucky]

@phansys for example what to use here, please check https://github.com/Ocramius/ProxyManager/blob/2.14.x/examples/access-interceptor-scope-localizer.php That's pretty generic example. I've been playing with this some weeks ago in context of sncredis and this example I came up with could help too:

(static function () : void {
    $config = new Configuration();
    $config->setGeneratorStrategy(new FileWriterGeneratorStrategy(new FileLocator(__DIR__.'/generated')));
    $config->setProxiesTargetDir(__DIR__.'/generated');
    // then register the autoloader
    spl_autoload_register($config->getProxyAutoloader());

    $factory = new AccessInterceptorScopeLocalizerFactory($config);
    $foo     = new FluentCounter();

    $proxy   = $factory->createProxy(
        $foo,
        [
            'fluentMethod' => static function (AccessInterceptorInterface $proxy, FluentCounter $realInstance) use (&$time) : void {
                $time = microtime(true);
            },
        ],
        [
            'fluentMethod' => static function (AccessInterceptorInterface $proxy, FluentCounter $realInstance) use (&$time) : void {
                echo "fluentMethod #{$realInstance->counter} took ".(microtime(true) - $time)."!\n";
            },
        ]
    );

    var_dump(\Safe\class_parents($proxy));
    $proxy->fluentMethod()->fluentMethod()->fluentMethod()->fluentMethod();

    echo 'The proxy counter is now at ' . $proxy->counter . "\n";
    echo 'The real instance counter is now at ' . $foo->counter . "\n";
})();

Current solution with custom generator indeed is not something we will merge.

If you think it's better, perhaps you could also use laminas code generator, that's a library behind ocramius proxy manager. I was playing with that idea, because it seems we can't avoid having to create reflection classes and iterate over Redis classes anyways - ProxyManager requires listing all the methods we want to proxify.

With that one, it would be something like this:

<?php

declare(strict_types=1);

namespace ProxyManager\Example\AccessInterceptorScopeLocalizer;

use Laminas\Code\Generator\ClassGenerator;
use Laminas\Code\Generator\MethodGenerator;
use Laminas\Code\Generator\ParameterGenerator;
use Laminas\Code\Reflection\MethodReflection;
use ProxyManager\Generator\Util\ClassGeneratorUtils;
use Psr\Log\LoggerInterface;

require_once __DIR__ . '/../vendor/autoload.php';

$reflClass = new \ReflectionClass(\RedisCluster::class);
$classGenerator = new ClassGenerator('SncLoggingAwareRedisClient');
$classGenerator->setExtendedClass($reflClass->getName());

foreach ($reflClass->getMethods(\ReflectionMethod::IS_PUBLIC) as $method) {
    $forwardedParams = [];
    if ($method->getName()[0] === '_') {
        continue;
    }

    foreach ($method->getParameters() as $parameter) {
        $forwardedParams[] = ($parameter->isVariadic() ? '...' : '') . '$' . $parameter->getName();
    }

    $methodGenerator = MethodGenerator::copyMethodSignature(new MethodReflection($reflClass->getName(), $method->getName()));

    if ($method->isConstructor()) {
        $logger = new ParameterGenerator('logger', LoggerInterface::class, null, $method->getNumberOfParameters());
        $logger->setDefaultValue(null);
        $methodGenerator->setParameter($logger);
        $methodGenerator->setBody("
        parent::__construct(" . implode(', ', $forwardedParams) . '");
        $this->logger = $logger;
');
    } else {
        $methodGenerator->setBody("
        \$startTime = microtime(true);
        \$return = parent::{$method->getName()}(" . implode(', ', $forwardedParams) . ");
        \$duration = (microtime(true) - \$startTime) * 1000;

        if (null !== \$this->logger) {
            \$this->logger->logCommand(\$this->getCommandString({$method->getName()}, func_get_args(), \$duration, \$this->alias, false);
        }

        return \$return;
");
    }

    ClassGeneratorUtils::addMethodIfNotFinal($reflClass, $classGenerator, $methodGenerator);
}

echo $classGenerator->generate();

[ostrolucky]

Superseded by #636