Repository: open-telemetry/opentelemetry-php (php)

Issue:
boolean config values
Per https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/sdk-environment-variables.md#boolean-value :
```
Any value that represents a Boolean MUST be set to true only by the case-insensitive string "true", meaning "True" or "TRUE" are also accepted, as true. An SDK MUST NOT extend this definition and define additional values that are interpreted as true. Any value not explicitly defined here as a true value, including unset and empty values, MUST be interpreted as false. If any value other than a true value, case-insensitive string "false", empty, or unset is used, a warning SHOULD be logged to inform users about the fallback to false being applied.
```

We currently accept other values, such as "on", "1" as true, but should not.

Pull request:
disallow non-standard boolean config values
per spec, only 'true' and 'false' are valid boolean values. anything else should be interpreted as false and a warning emitted.

Closes #861 

Patch:
diff --git a/src/Context/Context.php b/src/Context/Context.php
index 314a6118..2488ed24 100644
--- a/src/Context/Context.php
+++ b/src/Context/Context.php
@@ -34,8 +34,6 @@ final class Context implements ContextInterface
     }
 
     /**
-     * @internal
-     *
      * @param ContextStorageInterface&ExecutionContextAwareInterface $storage
      */
     public static function setStorage(ContextStorageInterface $storage): void
diff --git a/src/SDK/Common/Configuration/Configuration.php b/src/SDK/Common/Configuration/Configuration.php
index 68d89b1d..cb45935b 100644
--- a/src/SDK/Common/Configuration/Configuration.php
+++ b/src/SDK/Common/Configuration/Configuration.php
@@ -4,6 +4,8 @@ declare(strict_types=1);
 
 namespace OpenTelemetry\SDK\Common\Configuration;
 
+use InvalidArgumentException;
+use OpenTelemetry\SDK\Behavior\LogsMessagesTrait;
 use OpenTelemetry\SDK\Common\Configuration\Parser\BooleanParser;
 use OpenTelemetry\SDK\Common\Configuration\Parser\ListParser;
 use OpenTelemetry\SDK\Common\Configuration\Parser\MapParser;
@@ -20,6 +22,8 @@ use UnexpectedValueException;
  */
 class Configuration
 {
+    use LogsMessagesTrait;
+
     public static function has(string $name): bool
     {
         return CompositeResolver::instance()->hasVariable($name);
@@ -48,14 +52,20 @@ class Configuration
 
     public static function getBoolean(string $key, bool $default = null): bool
     {
-        return BooleanParser::parse(
-            self::validateVariableValue(
-                CompositeResolver::instance()->resolve(
-                    self::validateVariableType($key, VariableTypes::BOOL),
-                    null === $default ? $default : ($default ? 'true' : 'false')
-                )
+        $resolved = self::validateVariableValue(
+            CompositeResolver::instance()->resolve(
+                self::validateVariableType($key, VariableTypes::BOOL),
+                null === $default ? $default : ($default ? 'true' : 'false')
             )
         );
+
+        try {
+            return BooleanParser::parse($resolved);
+        } catch (InvalidArgumentException $e) {
+            self::logWarning(sprintf('Invalid boolean value "%s" interpreted as "false" for %s', $resolved, $key));
+
+            return false;
+        }
     }
 
     public static function getMixed(string $key, $default = null)
diff --git a/src/SDK/Common/Configuration/KnownValues.php b/src/SDK/Common/Configuration/KnownValues.php
index 2d7548f4..f34a22dd 100644
--- a/src/SDK/Common/Configuration/KnownValues.php
+++ b/src/SDK/Common/Configuration/KnownValues.php
@@ -60,10 +60,6 @@ interface KnownValues
     public const VALUES_BOOLEAN = [
         self::VALUE_TRUE,
         self::VALUE_FALSE,
-        self::VALUE_ON,
-        self::VALUE_OFF,
-        self::VALUE_1,
-        self::VALUE_0,
     ];
 
     public const VALUES_COMPRESSION= [
diff --git a/src/SDK/Common/Configuration/Parser/BooleanParser.php b/src/SDK/Common/Configuration/Parser/BooleanParser.php
index 7ac2d61f..4141c61e 100644
--- a/src/SDK/Common/Configuration/Parser/BooleanParser.php
+++ b/src/SDK/Common/Configuration/Parser/BooleanParser.php
@@ -8,17 +8,8 @@ use InvalidArgumentException;
 
 class BooleanParser
 {
-    private const TRUTHY_VALUES = [
-        'true',
-        'on',
-        '1',
-    ];
-
-    private const FALSY_VALUES = [
-        'false',
-        'off',
-        '0',
-    ];
+    private const TRUE_VALUE = 'true';
+    private const FALSE_VALUE = 'false';
 
     /**
      * @param string|bool $value
@@ -28,11 +19,11 @@ class BooleanParser
         if (is_bool($value)) {
             return $value;
         }
-        if (in_array(strtolower($value), self::TRUTHY_VALUES)) {
+        if (strtolower($value) === self::TRUE_VALUE) {
             return true;
         }
 
-        if (in_array(strtolower($value), self::FALSY_VALUES)) {
+        if (strtolower($value) === self::FALSE_VALUE) {
             return false;
         }
 


Test patch:
diff --git a/tests/Unit/SDK/Common/Configuration/ConfigurationTest.php b/tests/Unit/SDK/Common/Configuration/ConfigurationTest.php
index 71f43845..64b5ead9 100644
--- a/tests/Unit/SDK/Common/Configuration/ConfigurationTest.php
+++ b/tests/Unit/SDK/Common/Configuration/ConfigurationTest.php
@@ -188,8 +188,6 @@ class ConfigurationTest extends TestCase
         return [
             'false' => ['false', true, false],
             'true' => ['true', false, true],
-            'truthy' => ['1', false, true],
-            'falsey' => ['0', true, false],
             'TRUE' => ['TRUE', false, true],
             'FALSE' => ['FALSE', true, false],
         ];
diff --git a/tests/Unit/SDK/Common/Configuration/Parser/BooleanParserTest.php b/tests/Unit/SDK/Common/Configuration/Parser/BooleanParserTest.php
index 2d4a0b95..ed478211 100644
--- a/tests/Unit/SDK/Common/Configuration/Parser/BooleanParserTest.php
+++ b/tests/Unit/SDK/Common/Configuration/Parser/BooleanParserTest.php
@@ -16,17 +16,13 @@ class BooleanParserTest extends TestCase
     private const TRUTHY_VALUES = [
         'bool uppercase' => ['TRUE'],
         'bool lowercase' => ['true'],
-        'state uppercase' => ['ON'],
-        'state lowercase' => ['on'],
-        'int' => ['1'],
+        'bool mixed case' => ['True'],
     ];
 
     private const FALSY_VALUES = [
         'bool uppercase' => ['FALSE'],
         'bool lowercase' => ['false'],
-        'state uppercase' => ['OFF'],
-        'state lowercase' => ['off'],
-        'int' => ['0'],
+        'bool mixed case' => ['False'],
     ];
 
     private const NON_BOOLEAN_VALUES = [
@@ -35,6 +31,17 @@ class BooleanParserTest extends TestCase
         'float' => ['0.5'],
     ];
 
+    private const DISALLOWED_BOOLEAN_VALUES = [
+        ['ON'],
+        ['on'],
+        ['On'],
+        ['1'],
+        ['OFF'],
+        ['off'],
+        ['Off'],
+        ['0'],
+    ];
+
     /**
      * @dataProvider truthyValueProvider
      */
@@ -55,6 +62,20 @@ class BooleanParserTest extends TestCase
         );
     }
 
+    /**
+     * @dataProvider disallowedBooleanProvider
+     */
+    public function test_disallowed_boolean_type_values_throw_exception(string $value): void
+    {
+        $this->expectException(InvalidArgumentException::class);
+        BooleanParser::parse($value);
+    }
+
+    public function disallowedBooleanProvider(): array
+    {
+        return self::DISALLOWED_BOOLEAN_VALUES;
+    }
+
     /**
      * @dataProvider nonBooleanValueProvider
      */
diff --git a/tests/Unit/SDK/SdkTest.php b/tests/Unit/SDK/SdkTest.php
index 69047f31..ab52a78c 100644
--- a/tests/Unit/SDK/SdkTest.php
+++ b/tests/Unit/SDK/SdkTest.php
@@ -41,9 +41,7 @@ class SdkTest extends TestCase
     {
         return [
             ['true', true],
-            ['1', true],
             ['false', false],
-            ['0', false],
         ];
     }