review

Jean-Beru · Jean-Beru · commit 47d16ba46d13 · 2025-03-17T17:07:50.000+01:00
diff --git a/src/Symfony/Bundle/FrameworkBundle/DependencyInjection/Compiler/FeatureFlagPass.php b/src/Symfony/Bundle/FrameworkBundle/DependencyInjection/Compiler/FeatureFlagPass.php
@@ -45,6 +45,9 @@ public function process(ContainerBuilder $container): void
                 if (!$r->hasMethod($method)) {
                     throw new \RuntimeException(\sprintf('Invalid feature method "%s": method "%s::%s()" does not exist.', $serviceId, $r->getName(), $method));
                 }
+                if (!$r->getMethod($method)->isPublic()) {
+                    throw new \RuntimeException(\sprintf('Invalid feature method "%s": method "%s::%s()" must be public.', $serviceId, $r->getName(), $method));
+                }
 
                 $features[$featureName] = $container->setDefinition(
                     '.feature_flag.feature',
diff --git a/src/Symfony/Bundle/FrameworkBundle/Tests/Fixtures/FeatureFlag/InvalidMethodVisibilityFeature.php b/src/Symfony/Bundle/FrameworkBundle/Tests/Fixtures/FeatureFlag/InvalidMethodVisibilityFeature.php
@@ -0,0 +1,23 @@
+<?php
+
+/*
+ * This file is part of the Symfony package.
+ *
+ * (c) Fabien Potencier <fabien@symfony.com>
+ *
+ * For the full copyright and license information, please view the LICENSE
+ * file that was distributed with this source code.
+ */
+
+namespace Symfony\Bundle\FrameworkBundle\Tests\Fixtures\FeatureFlag;
+
+use Symfony\Component\FeatureFlag\Attribute\AsFeature;
+
+#[AsFeature(method: 'resolve')]
+class InvalidMethodVisibilityFeature
+{
+    protected function resolve(): bool
+    {
+        return true;
+    }
+}
diff --git a/src/Symfony/Bundle/FrameworkBundle/Tests/Functional/FeatureFlagTest.php b/src/Symfony/Bundle/FrameworkBundle/Tests/Functional/FeatureFlagTest.php
@@ -55,6 +55,14 @@ public function testFeatureFlagAssertionsWithInvalidMethod()
         static::bootKernel(['test_case' => 'FeatureFlag', 'root_config' => 'config_with_invalid_method.yml']);
     }
 
+    public function testFeatureFlagAssertionsWithInvalidMethodVisibility()
+    {
+        $this->expectException(\RuntimeException::class);
+        $this->expectExceptionMessage('Invalid feature method "Symfony\Bundle\FrameworkBundle\Tests\Fixtures\FeatureFlag\InvalidMethodVisibilityFeature": method "Symfony\Bundle\FrameworkBundle\Tests\Fixtures\FeatureFlag\InvalidMethodVisibilityFeature::resolve()" must be public.');
+
+        static::bootKernel(['test_case' => 'FeatureFlag', 'root_config' => 'config_with_invalid_method_visibility.yml']);
+    }
+
     public function testFeatureFlagAssertionsWithDifferentMethod()
     {
         $this->expectException(\LogicException::class);
diff --git a/src/Symfony/Bundle/FrameworkBundle/Tests/Functional/app/FeatureFlag/config_with_invalid_method_visibility.yml b/src/Symfony/Bundle/FrameworkBundle/Tests/Functional/app/FeatureFlag/config_with_invalid_method_visibility.yml
@@ -0,0 +1,11 @@
+imports:
+    - { resource: ../config/default.yml }
+
+framework:
+    http_method_override: false
+    feature_flag:
+        enabled: true
+
+services:
+    Symfony\Bundle\FrameworkBundle\Tests\Fixtures\FeatureFlag\InvalidMethodVisibilityFeature:
+        autoconfigure: true
diff --git a/src/Symfony/Component/FeatureFlag/Debug/TraceableFeatureChecker.php b/src/Symfony/Component/FeatureFlag/Debug/TraceableFeatureChecker.php
@@ -40,15 +40,17 @@ public function isEnabled(string $featureName): bool
 
     public function getValue(string $featureName): mixed
     {
+        $value = $this->decorated->getValue($featureName);
+
         $this->resolvedValues[$featureName] ??= [
             'status' => 'resolved',
-            'value' => $this->decorated->getValue($featureName),
+            'value' => $value,
             'calls' => 0,
         ];
 
         ++$this->resolvedValues[$featureName]['calls'];
 
-        return $this->decorated->getValue($featureName);
+        return $value;
     }
 
     /**
diff --git a/src/Symfony/Component/FeatureFlag/FeatureChecker.php b/src/Symfony/Component/FeatureFlag/FeatureChecker.php
@@ -32,7 +32,7 @@ public function isEnabled(string $featureName): bool
 
     public function getValue(string $featureName): mixed
     {
-        if (isset($this->cache[$featureName])) {
+        if (array_key_exists($featureName, $this->cache)) {
             return $this->cache[$featureName];
         }
 
diff --git a/src/Symfony/Component/FeatureFlag/LICENSE b/src/Symfony/Component/FeatureFlag/LICENSE
@@ -1,4 +1,4 @@
-Copyright (c) 2024-present Fabien Potencier
+Copyright (c) 2025-present Fabien Potencier
 
 Permission is hereby granted, free of charge, to any person obtaining a copy
 of this software and associated documentation files (the "Software"), to deal

Original file line number	Diff line number	Diff line change
`@@ -45,6 +45,9 @@ public function process(ContainerBuilder $container): void`
`45`	`45`	`if (!$r->hasMethod($method)) {`
`46`	`46`	`throw new \RuntimeException(\sprintf('Invalid feature method "%s": method "%s::%s()" does not exist.', $serviceId, $r->getName(), $method));`
`47`	`47`	`}`
	`48`	`+ if (!$r->getMethod($method)->isPublic()) {`
	`49`	`+ throw new \RuntimeException(\sprintf('Invalid feature method "%s": method "%s::%s()" must be public.', $serviceId, $r->getName(), $method));`
	`50`	`+ }`
`48`	`51`
`49`	`52`	`$features[$featureName] = $container->setDefinition(`
`50`	`53`	`'.feature_flag.feature',`
Original file line number	Diff line number	Diff line change
`@@ -55,6 +55,14 @@ public function testFeatureFlagAssertionsWithInvalidMethod()`
`55`	`55`	`static::bootKernel(['test_case' => 'FeatureFlag', 'root_config' => 'config_with_invalid_method.yml']);`
`56`	`56`	`}`
`57`	`57`
	`58`	`+ public function testFeatureFlagAssertionsWithInvalidMethodVisibility()`
	`59`	`+ {`
	`60`	`+ $this->expectException(\RuntimeException::class);`
	`61`	`+ $this->expectExceptionMessage('Invalid feature method "Symfony\Bundle\FrameworkBundle\Tests\Fixtures\FeatureFlag\InvalidMethodVisibilityFeature": method "Symfony\Bundle\FrameworkBundle\Tests\Fixtures\FeatureFlag\InvalidMethodVisibilityFeature::resolve()" must be public.');`
	`62`	`+`
	`63`	`+ static::bootKernel(['test_case' => 'FeatureFlag', 'root_config' => 'config_with_invalid_method_visibility.yml']);`
	`64`	`+ }`
	`65`	`+`
`58`	`66`	`public function testFeatureFlagAssertionsWithDifferentMethod()`
`59`	`67`	`{`
`60`	`68`	`$this->expectException(\LogicException::class);`
Original file line number	Diff line number	Diff line change
`@@ -32,7 +32,7 @@ public function isEnabled(string $featureName): bool`
`32`	`32`
`33`	`33`	`public function getValue(string $featureName): mixed`
`34`	`34`	`{`
`35`		`- if (isset($this->cache[$featureName])) {`
	`35`	`+ if (array_key_exists($featureName, $this->cache)) {`
`36`	`36`	`return $this->cache[$featureName];`
`37`	`37`	`}`
`38`	`38`
Original file line number	Diff line number	Diff line change
`@@ -1,4 +1,4 @@`
`1`		`-Copyright (c) 2024-present Fabien Potencier`
	`1`	`+Copyright (c) 2025-present Fabien Potencier`
`2`	`2`
`3`	`3`	`Permission is hereby granted, free of charge, to any person obtaining a copy`
`4`	`4`	`of this software and associated documentation files (the "Software"), to deal`