Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 8 additions & 1 deletion .github/workflows/static-analysis.yml
Original file line number Diff line number Diff line change
Expand Up @@ -18,11 +18,18 @@ jobs:
uses: "shivammathur/setup-php@v2"
with:
coverage: "none"
extensions: "relay"
extensions: "redis, relay"
php-version: "8.4"

- name: "Install dependencies with Composer"
uses: "ramsey/composer-install@v2"

- name: "Compile Symfony container"
run: |
php -r "
require 'vendor/autoload.php';
(new Snc\RedisBundle\Tests\Functional\App\Kernel('test', true))->boot();
"

- name: "Run a static analysis with vimeo/psalm"
run: "vendor/bin/psalm --show-info=false --stats --output-format=github --threads=$(nproc)"
3 changes: 2 additions & 1 deletion composer.json
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@
"phpunit/phpunit": "^9.6.34 || ^10.5.63",
"predis/predis": "^3.1",
"psalm/plugin-phpunit": "^0.19.3",
"psalm/plugin-symfony": "^5.0",
"psr/http-message": "^2",
"seec/phpunit-consecutive-params": "^1.1.4",
"symfony/browser-kit": "^6.4 || ^7.3 || ^8.0",
Expand All @@ -49,7 +50,7 @@
"symfony/twig-bundle": "^6.4 || ^7.3 || ^8.0",
"symfony/web-profiler-bundle": "^6.4 || ^7.3 || ^8.0",
"symfony/yaml": "^6.4 || ^7.3 || ^8.0",
"vimeo/psalm": "^5 || ^6 || ^7"
"vimeo/psalm": "^6 || ^7"
},
"conflict": {
"ext-redis": "<6.1.0",
Expand Down
1 change: 1 addition & 0 deletions phpunit.xml.dist
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
<php>
<ini name="error_reporting" value="-1"/>
<env name="SHELL_VERBOSITY" value="-1"/>
<env name="REDIS_DSNS" value='["redis://sncredis@127.0.0.1/3","redis://sncredis@127.0.0.1/4","redis://sncredis@127.0.0.1/5"]'/>
</php>
<testsuites>
<testsuite name="RedisBundle Test Suite">
Expand Down
9 changes: 9 additions & 0 deletions psalm.xml.dist
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
<?xml version="1.0"?>
<psalm
errorLevel="4"
findUnusedIssueHandlerSuppression="false"
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

this is a no go. perhaps can be done inline if possible

xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
xmlns="https://getpsalm.org/schema/config"
xsi:schemaLocation="https://getpsalm.org/schema/config vendor/vimeo/psalm/config.xsd"
Expand All @@ -19,8 +20,16 @@
<referencedMethod name="Symfony\Component\Config\Definition\Builder\NodeBuilder::end"/>
</errorLevel>
</UnusedMethodCall>
<UndefinedInterfaceMethod>
<errorLevel type="suppress">
<referencedMethod name="Symfony\Component\Config\Definition\Builder\NodeParentInterface::end"/>
</errorLevel>
</UndefinedInterfaceMethod>
</issueHandlers>
<plugins>
Comment on lines +17 to 29
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

better to do inline suppression

<pluginClass class="Psalm\PhpUnitPlugin\Plugin"/>
<pluginClass class="Psalm\SymfonyPsalmPlugin\Plugin">
<containerXml>tests/Functional/App/var/cache/test/Snc_RedisBundle_Tests_Functional_App_KernelTestDebugContainer.xml</containerXml>
</pluginClass>
</plugins>
</psalm>
2 changes: 1 addition & 1 deletion src/DependencyInjection/Compiler/LoggingPass.php
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ public function process(ContainerBuilder $container): void
$arguments = $option->getArgument(0);

$connectionFactoryId = sprintf('snc_redis.%s_connectionfactory', $clientAlias);
$connectionFactoryDef = new Definition((string) $container->getParameter('snc_redis.connection_factory.class'));
$connectionFactoryDef = new Definition($container->getParameter('snc_redis.connection_factory.class'));
if ($container->getParameter('kernel.debug')) {
$connectionFactoryDef->addMethodCall('setStopwatch', [new Reference('debug.stopwatch', ContainerInterface::NULL_ON_INVALID_REFERENCE)]);
}
Expand Down
13 changes: 7 additions & 6 deletions src/DependencyInjection/SncRedisExtension.php
Original file line number Diff line number Diff line change
Expand Up @@ -180,12 +180,13 @@ private function loadPredisClient(array $client, ContainerBuilder $container): v
}

$optionId = sprintf('snc_redis.client.%s_options', $client['alias']);
$optionDef = new Definition((string) $container->getParameter('snc_redis.client_options.class'));
$optionDef = new Definition($container->getParameter('snc_redis.client_options.class'));
$optionDef->addArgument($client['options']);
$container->setDefinition($optionId, $optionDef);
$clientDef = new Definition($client['class'] ?? (string) $container->getParameter('snc_redis.client.class'));
$clientDef = new Definition($client['class'] ?? $container->getParameter('snc_redis.client.class'));
$clientDef->addTag('snc_redis.client', ['alias' => $client['alias']]);
if ($connectionCount === 1 && !isset($client['options']['cluster']) && !isset($client['options']['replication'])) {

if ($connectionCount === 1 && !isset($client['options']['replication'])) {
$clientDef->addArgument(new Reference(sprintf('snc_redis.connection.%s_parameters.%s', $connectionAliases[0], $client['alias'])));
} else {
$connections = [];
Expand All @@ -206,7 +207,7 @@ private function loadPredisClient(array $client, ContainerBuilder $container): v
*/
private function loadPredisConnectionParameters(string $clientAlias, array $options, ContainerBuilder $container, object $dsn): void
{
$parametersClass = (string) $container->getParameter('snc_redis.connection_parameters.class');
$parametersClass = $container->getParameter('snc_redis.connection_parameters.class');
$parameterId = sprintf('snc_redis.connection.%s_parameters.%s', $options['alias'], $clientAlias);

$parameterDef = new Definition($parametersClass);
Expand Down Expand Up @@ -261,7 +262,7 @@ private function loadMonolog(array $config, ContainerBuilder $container): void
{
$ref = new Reference(sprintf('snc_redis.%s', $config['monolog']['client']));

$def = new Definition((string) $container->getParameter('snc_redis.monolog_handler.class'), [
$def = new Definition($container->getParameter('snc_redis.monolog_handler.class'), [
$ref,
$config['monolog']['key'],
]);
Expand All @@ -277,6 +278,6 @@ private function loadMonolog(array $config, ContainerBuilder $container): void
#[Override]
public function getConfiguration(array $config, ContainerBuilder $container): ConfigurationInterface
{
return new Configuration((bool) $container->getParameter('kernel.debug'));
return new Configuration($container->getParameter('kernel.debug'));
}
}
32 changes: 31 additions & 1 deletion src/Factory/PredisParametersFactory.php
Original file line number Diff line number Diff line change
Expand Up @@ -9,21 +9,51 @@
use Snc\RedisBundle\DependencyInjection\Configuration\RedisDsn;

use function array_filter;
use function array_map;
use function array_merge;
use function constant;
use function count;
use function defined;
use function is_a;
use function is_array;
use function is_string;
use function sprintf;
use function str_replace;

/** @internal */
class PredisParametersFactory
{
/**
* @param class-string<ParametersInterface> $class
* @param array<string, mixed> $options
* @param string|list<string>|list<list<string>> $dsn
*
* @return ParametersInterface|list<ParametersInterface>
*/
public static function create(array $options, string $class, string|array $dsn): ParametersInterface|array
{
if (is_string($dsn)) {
$dsn = [$dsn];
}

// json:/csv: env processors can produce a single-element array wrapping the actual list
if (count($dsn) === 1 && is_array($dsn[0])) {
$dsn = $dsn[0];
}

$parameters = array_map(
static fn (string $d) => static::createFromSingleDsn($options, $class, $d),
$dsn,
);

return count($parameters) === 1 ? $parameters[0] : $parameters;
}

/**
* @param class-string<ParametersInterface> $class
* @param array<string, mixed> $options
*/
public static function create(array $options, string $class, string $dsn): ParametersInterface
private static function createFromSingleDsn(array $options, string $class, string $dsn): ParametersInterface
{
if (!is_a($class, ParametersInterface::class, true)) {
throw new InvalidArgumentException(sprintf('%s::%s requires $class argument to implement %s', self::class, __METHOD__, ParametersInterface::class));
Expand Down
24 changes: 24 additions & 0 deletions tests/Factory/PredisParametersFactoryTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -185,4 +185,28 @@ public function testCreateMergesSslWithTlsVersion(): void
$this->assertSame(STREAM_CRYPTO_METHOD_TLSv1_2_CLIENT, $ssl['crypto_type']);
$this->assertFalse($ssl['verify_peer']);
}

/**
* @testWith ["redis://localhost"]
* [["redis://localhost"]]
* [[["redis://localhost"]]]
*/
public function testCreateReturnsSingleParameters(string|array $dsn): void
{
$result = PredisParametersFactory::create([], Parameters::class, $dsn);
$this->assertInstanceOf(Parameters::class, $result);
}

/**
* @param array<int, string|array<int, string>> $dsn
*
* @testWith [["redis://host1", "redis://host2"]]
* [[["redis://host1", "redis://host2"]]]
*/
public function testCreateReturnsMultipleParameters(array $dsn): void
{
$result = PredisParametersFactory::create([], Parameters::class, $dsn);
$this->assertIsArray($result);
$this->assertCount(2, $result);
}
}
20 changes: 15 additions & 5 deletions tests/Functional/App/Controller/Controller.php
Original file line number Diff line number Diff line change
Expand Up @@ -13,18 +13,28 @@

namespace Snc\RedisBundle\Tests\Functional\App\Controller;

use Predis\ClientInterface;
use Redis;
use RedisArray;
use Relay\Relay;
use Symfony\Bundle\FrameworkBundle\Controller\AbstractController;
use Symfony\Component\HttpFoundation\JsonResponse;

class Controller extends AbstractController
{
public function __invoke(Redis $redis): JsonResponse
/** @param iterable<Redis|ClientInterface|Relay|RedisArray> $clients */
public function __construct(private iterable $clients)
{
$redis->set('foo', 'bar');
}

public function __invoke(): JsonResponse
{
$result = null;
foreach ($this->clients as $client) {
$client->set('foo', 'bar');
$result = $client->get('foo');
}

return new JsonResponse([
'result' => $redis->get('foo'),
]);
return new JsonResponse(['result' => $result]);
}
}
22 changes: 11 additions & 11 deletions tests/Functional/App/config.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -35,11 +35,9 @@ snc_redis:
cluster:
type: predis
alias: cluster
dsn:
- redis://sncredis@127.0.0.1/3
- redis://sncredis@127.0.0.1/4
- redis://sncredis@127.0.0.1/5
dsn: "%env(json:REDIS_DSNS)%"
options:
cluster: predis
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

why?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The cluster: predis option is required here because the DSN is an env var resolved at runtime via json:. At compile time Symfony sees a single opaque placeholder, so it cannot count the connections — Predis needs to know the aggregation mode upfront in order to build the right connection factory.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

That's the whole point why env vars were not supported for Predis. Predis needs to know connectionCount at build time, where env vars are not resolved yet. Think of a solution.

prefix: foo
connection_timeout: 10
connection_persistent: true
Expand All @@ -59,20 +57,14 @@ snc_redis:
with_acl:
type: predis
alias: with_acl
dsn: redis://localhost/1
dsn: redis://snc_redis:snc_password@localhost:7099/1
logging: false
options:
parameters:
username: my_user
password: sncredis

services:
_defaults:
autowire: true
autoconfigure: true
public: false
bind:
Predis\Client $predisReplication: '@snc_redis.predis_replication'

Redis: '@snc_redis.default'

Expand All @@ -87,6 +79,14 @@ services:
resource: './Controller'
tags: ['controller.service_arguments']

Snc\RedisBundle\Tests\Functional\App\Controller\PredisReplication:
bind:
Predis\Client $predisReplication: '@snc_redis.predis_replication'

Snc\RedisBundle\Tests\Functional\App\Controller\Controller:
arguments:
$clients: !tagged_iterator snc_redis.client

var_dumper.cli_dumper:
class: Symfony\Component\VarDumper\Dumper\CliDumper
arguments: ['/dev/null']
4 changes: 2 additions & 2 deletions tests/Functional/IntegrationTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -69,8 +69,8 @@ public function testIntegration(): void
$this->assertInstanceOf(RedisDataCollector::class, $collector);
$this->assertInstanceOf(ResetInterface::class, $container = $this->client->getKernel()->getContainer());
$this->assertInstanceOf(RedisLogger::class, $redisLogger = $container->get('test.snc_redis.logger'));
$this->assertSame(5, $redisLogger->getNbCommands());
$this->assertCount(5, $collector->getCommands());
$this->assertSame(13, $redisLogger->getNbCommands());
$this->assertCount(13, $collector->getCommands());
$container->reset();
$this->assertSame(0, $redisLogger->getNbCommands());
}
Expand Down
Loading