From ceca2d67088a25162280418b8642521cf642d525 Mon Sep 17 00:00:00 2001 From: aubes <3941035+aubes@users.noreply.github.com> Date: Mon, 28 Sep 2026 18:11:05 +0200 Subject: [PATCH] feat: validate provider services and fix logger injection --- CHANGELOG.md | 4 + phpstan.neon | 3 + .../Compiler/SetProviderPass.php | 42 +-- .../Compiler/SetProviderPassTest.php | 299 ++++++++++++++++++ tests/Fixtures/ProviderWithMissingParent.php | 12 + 5 files changed, 342 insertions(+), 18 deletions(-) create mode 100644 tests/DependencyInjection/Compiler/SetProviderPassTest.php create mode 100644 tests/Fixtures/ProviderWithMissingParent.php diff --git a/CHANGELOG.md b/CHANGELOG.md index 0af39da..6dc24cf 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -14,9 +14,12 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - `EnvVarProvider` and `RedisProvider` now return a `PARSE_ERROR` (and the default value) for raw values that do not match the requested type, instead of silently casting them (`"abc"` as integer used to resolve to `0`, `"banana"` as boolean to `false`). - `InMemoryProvider` returns a `TYPE_MISMATCH` instead of casting for integers other than `0`/`1` requested as boolean (e.g. `2` or `-1`, previously `true`), non-string values requested as string (e.g. `42`, previously `"42"`), and floats requested as integer (e.g. `1.5`, previously `1`). - `ResolutionDetailsTrait::toBool()` is replaced by `parseBool()`, `parseInt()`, `parseFloat()`, and `parseObject()`. +- Provider services (`provider`, `providers`) must now declare their class, as Symfony already requires for services built by a factory. A provider created by a factory, or inheriting its class from a `parent` under an id that is not a class name, now fails at compile time without an explicit `class` option instead of skipping validation. ### Fixed +- A provider service whose class does not exist now fails with an explicit "cannot be found" message instead of "must implement Provider", and a provider class whose parent class or interface is missing reports that missing class. +- Providers now receive the application logger when MonologBundle is not installed; previously they got none. A logger already set on a provider service (e.g. a dedicated Monolog channel) is no longer overridden. - `flags` and `providers`: keys are now kept as declared. Dashes were converted to underscores (a flag declared as `new-checkout` could only be evaluated as `new_checkout`), and an object flag holding a `name` key was renamed after that value. The undocumented list form `flags: [{name: ..., value: ...}]` is no longer supported. - `RedisProvider` now logs Redis client failures at `error` level. Previously, an unavailable Redis silently resolved every flag to its default value, with nothing in the logs. @@ -25,6 +28,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - Run `composer update open-feature/sdk` if your lock file pins a version below 2.3.0. - Code calling `OpenFeatureAPI::getInstance()` directly now gets an instance distinct from the bundle's `API` service (different provider, hooks, and evaluation context). Inject the `API` or `Client` service instead. - **Check your raw flag values.** `EnvVarProvider` and `RedisProvider` no longer cast unparsable values: a boolean flag set to anything other than `true`/`false`/`1`/`0`/`yes`/`no`/`on`/`off`/empty (e.g. `FEATURE_X=enabled`, previously `false`) or a numeric flag with a non-numeric value (for integers, decimal or exponent notation such as `10.0` or `1e3` too; leading zeros such as `08` are accepted) now resolves to the default value with a `PARSE_ERROR`. `InMemoryProvider` flags declared with a mismatching type (e.g. `max_items: 1.5` read as integer, `label: 42` read as string) now resolve to the default value with a `TYPE_MISMATCH`, like with typed providers such as flagd. In Twig, `{{ feature_value('max_items') }}` without a default reads the flag as a string and now renders `''`: pass a typed default (`feature_value('max_items', 10)`). These errors are not logged by the SDK: check the `open_feature` profiler panel in dev (error column), or register a hook that logs `ResolutionDetails::getError()` in `after()`. +- **Provider services created by a factory, or defined through `parent` under an id that is not a class name,** must set the `class` option (e.g. `class: App\FeatureFlag\MyProvider` next to `factory:`; `OpenFeature\interfaces\provider\Provider` is accepted when the concrete class is unknown), otherwise the container fails to compile with `Class "" used for OpenFeature provider service "..." cannot be found`. - Custom providers using `ResolutionDetailsTrait::toBool()` must switch to `parseBool($flagKey, $raw, $defaultValue)`, which returns a `ResolutionDetails` instead of a `bool`. ## [0.3.0] - 2026-06-15 diff --git a/phpstan.neon b/phpstan.neon index 1c4ffac..7835b26 100644 --- a/phpstan.neon +++ b/phpstan.neon @@ -7,3 +7,6 @@ parameters: paths: - src - tests + excludePaths: + # Extends a missing class on purpose (SetProviderPass error reporting test) + - tests/Fixtures/ProviderWithMissingParent.php diff --git a/src/DependencyInjection/Compiler/SetProviderPass.php b/src/DependencyInjection/Compiler/SetProviderPass.php index f2aa3b1..0c18ae7 100644 --- a/src/DependencyInjection/Compiler/SetProviderPass.php +++ b/src/DependencyInjection/Compiler/SetProviderPass.php @@ -12,6 +12,7 @@ use OpenFeature\interfaces\provider\Provider; use Symfony\Component\DependencyInjection\Compiler\CompilerPassInterface; use Symfony\Component\DependencyInjection\ContainerBuilder; +use Symfony\Component\DependencyInjection\ContainerInterface; use Symfony\Component\DependencyInjection\Definition; use Symfony\Component\DependencyInjection\Reference; @@ -34,11 +35,7 @@ public function process(ContainerBuilder $container): void return; } - $definition = $this->validateProvider($container, $providerId); - - if ($container->has('logger')) { - $definition->addMethodCall('setLogger', [new Reference('logger')]); - } + $this->injectLogger($this->validateProvider($container, $providerId)); $container->getDefinition(API::class) ->addMethodCall('setProvider', [new Reference($providerId)]); @@ -49,15 +46,10 @@ public function process(ContainerBuilder $container): void */ private function registerMultiProvider(ContainerBuilder $container, array $providers): void { - $hasLogger = $container->has('logger'); $providerData = []; foreach ($providers as $name => $providerId) { - $definition = $this->validateProvider($container, $providerId); - - if ($hasLogger) { - $definition->addMethodCall('setLogger', [new Reference('logger')]); - } + $this->injectLogger($this->validateProvider($container, $providerId)); $providerData[] = ['name' => (string) $name, 'provider' => new Reference($providerId)]; } @@ -72,22 +64,36 @@ private function registerMultiProvider(ContainerBuilder $container, array $provi }; $definition = new Definition(MultiProvider::class, [$providerData, $strategyDefinition]); - - if ($hasLogger) { - $definition->addMethodCall('setLogger', [new Reference('logger')]); - } + $this->injectLogger($definition); $container->getDefinition(API::class) ->addMethodCall('setProvider', [$definition]); } + private function injectLogger(Definition $definition): void + { + // Keeps a logger already set on the service (explicit call or LoggerAwareInterface autoconfiguration) + if ($definition->hasMethodCall('setLogger')) { + return; + } + + // Optional: without MonologBundle, FrameworkBundle's LoggerPass registers the default logger after this pass + $definition->addMethodCall('setLogger', [new Reference('logger', ContainerInterface::IGNORE_ON_INVALID_REFERENCE)]); + } + private function validateProvider(ContainerBuilder $container, string $providerId): Definition { $definition = $container->findDefinition($providerId); - $class = $definition->getClass(); + /** @var null|string $class */ + $class = $container->getParameterBag()->resolveValue($definition->getClass()); + + // Runs before parent resolution, like the core voter and env var processor passes: the class must be set on the service itself + if (!$reflection = $container->getReflectionClass($class)) { + throw new \InvalidArgumentException(\sprintf('Class "%s" used for OpenFeature provider service "%s" cannot be found. Set the "class" option on the service, including when it is created by a factory or inherits from a parent.', $class, $providerId)); + } - if ($class !== null && !\is_subclass_of($class, Provider::class)) { - throw new \InvalidArgumentException(\sprintf('The service "%s" (class "%s") configured as OpenFeature provider must implement "%s".', $providerId, $class, Provider::class)); + if (!$reflection->implementsInterface(Provider::class)) { + throw new \InvalidArgumentException(\sprintf('OpenFeature provider service "%s" (class "%s") must implement interface "%s".', $providerId, $reflection->getName(), Provider::class)); } return $definition; diff --git a/tests/DependencyInjection/Compiler/SetProviderPassTest.php b/tests/DependencyInjection/Compiler/SetProviderPassTest.php new file mode 100644 index 0000000..579e46d --- /dev/null +++ b/tests/DependencyInjection/Compiler/SetProviderPassTest.php @@ -0,0 +1,299 @@ +createContainer(provider: 'app.provider'); + $container->register('app.provider', InMemoryProvider::class); + + $this->process($container); + + $this->assertEquals( + [['setProvider', [new Reference('app.provider')]]], + $container->getDefinition(API::class)->getMethodCalls(), + ); + } + + // No "logger" service yet: without MonologBundle, it is registered after this pass + public function testInjectsAnOptionalLoggerIntoTheProvider(): void + { + $container = $this->createContainer(provider: 'app.provider'); + $container->register('app.provider', InMemoryProvider::class); + + $this->process($container); + + $this->assertEquals( + [['setLogger', [new Reference('logger', ContainerInterface::IGNORE_ON_INVALID_REFERENCE)]]], + $container->getDefinition('app.provider')->getMethodCalls(), + ); + } + + /** @param array $providers */ + #[DataProvider('provideSingleAndMultiProvider')] + public function testKeepsALoggerAlreadySetOnTheProvider(?string $provider, array $providers): void + { + $container = $this->createContainer(provider: $provider, providers: $providers); + $container->register('app.provider', InMemoryProvider::class) + ->addMethodCall('setLogger', [new Reference('app.feature_logger')]); + + $this->process($container); + + $this->assertEquals( + [['setLogger', [new Reference('app.feature_logger')]]], + $container->getDefinition('app.provider')->getMethodCalls(), + ); + } + + /** @return array}> */ + public static function provideSingleAndMultiProvider(): array + { + return [ + 'single provider' => ['app.provider', []], + 'multi-provider' => [null, ['local' => 'app.provider']], + ]; + } + + public function testValidatesAndConfiguresTheServiceBehindAnAlias(): void + { + $container = $this->createContainer(provider: 'app.provider'); + $container->register('app.real_provider', InMemoryProvider::class); + $container->setAlias('app.provider', 'app.real_provider'); + + $this->process($container); + + $this->assertEquals( + [['setLogger', [new Reference('logger', ContainerInterface::IGNORE_ON_INVALID_REFERENCE)]]], + $container->getDefinition('app.real_provider')->getMethodCalls(), + ); + $this->assertEquals( + [['setProvider', [new Reference('app.provider')]]], + $container->getDefinition(API::class)->getMethodCalls(), + ); + } + + public function testRejectsAClassThatIsNotAProvider(): void + { + $container = $this->createContainer(provider: 'app.provider'); + $container->register('app.provider', \stdClass::class); + + $this->expectException(\InvalidArgumentException::class); + $this->expectExceptionMessage('OpenFeature provider service "app.provider" (class "stdClass") must implement interface "OpenFeature\interfaces\provider\Provider".'); + + $this->process($container); + } + + public function testRejectsAClassThatDoesNotExist(): void + { + $container = $this->createContainer(provider: 'app.provider'); + $container->register('app.provider', 'App\Missing\Provider'); + + $this->expectException(\InvalidArgumentException::class); + $this->expectExceptionMessage('Class "App\Missing\Provider" used for OpenFeature provider service "app.provider" cannot be found. Set the "class" option on the service, including when it is created by a factory or inherits from a parent.'); + + $this->process($container); + } + + public function testReportsTheMissingParentOfAProviderClass(): void + { + $container = $this->createContainer(provider: 'app.provider'); + $container->register('app.provider', ProviderWithMissingParent::class); + + $this->expectException(\ReflectionException::class); + $this->expectExceptionMessage(\sprintf('Class "Aubes\OpenFeatureBundle\Tests\Fixtures\Missing\ParentProvider" not found while loading "%s".', ProviderWithMissingParent::class)); + + $this->process($container); + } + + public function testRequiresTheClassOfAFactoryProvider(): void + { + $container = $this->createContainer(provider: 'app.provider'); + $container->register('app.provider')->setFactory([InMemoryProvider::class, 'new']); + + $this->expectException(\InvalidArgumentException::class); + $this->expectExceptionMessage('Class "" used for OpenFeature provider service "app.provider" cannot be found. Set the "class" option on the service, including when it is created by a factory or inherits from a parent.'); + + $this->process($container); + } + + public function testAcceptsAFactoryProviderWithItsClass(): void + { + $container = $this->createContainer(provider: 'app.provider'); + $container->register('app.provider', InMemoryProvider::class)->setFactory([InMemoryProvider::class, 'new']); + + $this->process($container); + + $this->assertCount(1, $container->getDefinition(API::class)->getMethodCalls()); + } + + public function testAcceptsAFactoryProviderDeclaredWithTheProviderInterface(): void + { + $container = $this->createContainer(provider: 'app.provider'); + $container->register('app.provider', Provider::class)->setFactory([InMemoryProvider::class, 'new']); + + $this->process($container); + + $this->assertCount(1, $container->getDefinition(API::class)->getMethodCalls()); + } + + public function testRequiresTheClassOfAChildProvider(): void + { + $container = $this->createContainer(provider: 'app.child_provider'); + $container->register('app.base_provider', InMemoryProvider::class)->setAbstract(true); + $container->setDefinition('app.child_provider', new ChildDefinition('app.base_provider')); + + $this->expectException(\InvalidArgumentException::class); + $this->expectExceptionMessage('Class "" used for OpenFeature provider service "app.child_provider" cannot be found. Set the "class" option on the service, including when it is created by a factory or inherits from a parent.'); + + $this->process($container); + } + + public function testBuildsAMultiProviderInDeclarationOrder(): void + { + $container = $this->createContainer(providers: [ + 'custom' => 'app.custom_provider', + 'local' => InMemoryProvider::class, + ]); + $container->register('app.custom_provider', InMemoryProvider::class); + $container->register(InMemoryProvider::class); + + $this->process($container); + + $definition = $this->getMultiProviderDefinition($container); + $this->assertEquals([ + ['name' => 'custom', 'provider' => new Reference('app.custom_provider')], + ['name' => 'local', 'provider' => new Reference(InMemoryProvider::class)], + ], $definition->getArgument(0)); + + $strategy = $definition->getArgument(1); + $this->assertInstanceOf(Definition::class, $strategy); + $this->assertSame(FirstMatchStrategy::class, $strategy->getClass()); + } + + public function testBuildsTheFirstSuccessfulStrategy(): void + { + $container = $this->createContainer( + providers: ['local' => InMemoryProvider::class], + strategy: ['type' => 'first_successful', 'fallback' => null], + ); + $container->register(InMemoryProvider::class); + + $this->process($container); + + $strategy = $this->getMultiProviderDefinition($container)->getArgument(1); + $this->assertInstanceOf(Definition::class, $strategy); + $this->assertSame(FirstSuccessfulStrategy::class, $strategy->getClass()); + } + + public function testBuildsTheComparisonStrategyWithItsFallbackProvider(): void + { + $container = $this->createContainer( + providers: ['custom' => 'app.custom_provider', 'local' => InMemoryProvider::class], + strategy: ['type' => 'comparison', 'fallback' => 'local'], + ); + $container->register('app.custom_provider', InMemoryProvider::class); + $container->register(InMemoryProvider::class); + + $this->process($container); + + $strategy = $this->getMultiProviderDefinition($container)->getArgument(1); + $this->assertInstanceOf(Definition::class, $strategy); + $this->assertSame(ComparisonStrategy::class, $strategy->getClass()); + $this->assertEquals([new Reference(InMemoryProvider::class)], $strategy->getArguments()); + } + + public function testInjectsTheLoggerIntoTheMultiProviderAndEachProvider(): void + { + $container = $this->createContainer(providers: [ + 'custom' => 'app.custom_provider', + 'local' => InMemoryProvider::class, + ]); + $container->register('app.custom_provider', InMemoryProvider::class); + $container->register(InMemoryProvider::class); + + $this->process($container); + + $setLogger = [['setLogger', [new Reference('logger', ContainerInterface::IGNORE_ON_INVALID_REFERENCE)]]]; + $this->assertEquals($setLogger, $container->getDefinition('app.custom_provider')->getMethodCalls()); + $this->assertEquals($setLogger, $container->getDefinition(InMemoryProvider::class)->getMethodCalls()); + $this->assertEquals($setLogger, $this->getMultiProviderDefinition($container)->getMethodCalls()); + } + + public function testRejectsAnInvalidProviderInAMultiProvider(): void + { + $container = $this->createContainer(providers: ['bad' => 'app.bad_provider']); + $container->register('app.bad_provider', \stdClass::class); + + $this->expectException(\InvalidArgumentException::class); + $this->expectExceptionMessage('OpenFeature provider service "app.bad_provider" (class "stdClass") must implement interface "OpenFeature\interfaces\provider\Provider".'); + + $this->process($container); + } + + /** + * Only the parameters set by the extension and the API service: the pass does not need the rest of the bundle. + * + * @param array $providers + * @param array{type: string, fallback: null|string} $strategy + */ + private function createContainer(?string $provider = null, array $providers = [], array $strategy = ['type' => 'first_match', 'fallback' => null]): ContainerBuilder + { + $container = new ContainerBuilder(new ParameterBag([ + 'open_feature.provider' => $provider, + 'open_feature.providers' => $providers, + 'open_feature.strategy' => $strategy, + ])); + $container->register(API::class, OpenFeatureAPI::class); + + return $container; + } + + /** + * Replays the compile order: ResolveClassPass (priority 100) fills the class of FQCN-named services first. + */ + private function process(ContainerBuilder $container): void + { + (new ResolveClassPass())->process($container); + (new SetProviderPass())->process($container); + } + + private function getMultiProviderDefinition(ContainerBuilder $container): Definition + { + /** @var list}> $methodCalls */ + $methodCalls = $container->getDefinition(API::class)->getMethodCalls(); + $this->assertCount(1, $methodCalls); + $this->assertSame('setProvider', $methodCalls[0][0]); + + $definition = $methodCalls[0][1][0]; + $this->assertInstanceOf(Definition::class, $definition); + $this->assertSame(MultiProvider::class, $definition->getClass()); + + return $definition; + } +} diff --git a/tests/Fixtures/ProviderWithMissingParent.php b/tests/Fixtures/ProviderWithMissingParent.php new file mode 100644 index 0000000..4106ede --- /dev/null +++ b/tests/Fixtures/ProviderWithMissingParent.php @@ -0,0 +1,12 @@ +