Fix return value not type safe when default parameter is used

Author: lcharette

CHANGELOG.md

@@ -4,6 +4,9 @@ All notable changes to this project will be documented in this file.
 
 The format is based on [Keep a Changelog](http://keepachangelog.com/en/1.0.0/) and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0.html).
 
+## [5.1.3](https://github.com/userfrosting/framework/compare/5.1.2...5.1.3)
+- [Config] Fix issue with `getBool`, `getString`, `getInt` and `getArray` where a null value could be returned even if a default parameter was provided when the data did in fact return `null`, making the return value not type safe as it should be.
+
 ## [5.1.2](https://github.com/userfrosting/framework/compare/5.1.1...5.1.2)
 - Add Slim [type hinting](https://github.com/slimphp/Slim/releases/tag/4.14.0) & bump minimum slim version
 

src/Config/Config.php

@@ -21,15 +21,17 @@ class Config extends Repository
 {
     /**
      * Get the specified configuration value as bool.
+     * Returns null if the key is not found, unless a default is provided, in
+     * which case the return value is type safe (can't be null).
      *
      * @param string    $key
      * @param bool|null $default
      *
-     * @return ($default is null ? bool|null : bool) Returns null if the key is not found
+     * @return ($default is null ? bool|null : bool)
      */
     public function getBool(string $key, ?bool $default = null): ?bool
     {
-        $value = $this->get($key, $default);
+        $value = $this->get($key, $default) ?? $default;
 
         if (!is_bool($value) && !is_null($value)) {
             throw new TypeException("Config key '$key' doesn't return bool.");
@@ -40,15 +42,17 @@ public function getBool(string $key, ?bool $default = null): ?bool
 
     /**
      * Get the specified configuration value as bool.
+     * Returns null if the key is not found, unless a default is provided, in
+     * which case the return value is type safe (can't be null).
      *
      * @param string      $key
      * @param string|null $default
      *
-     * @return ($default is null ? string|null : string) Returns null if the key is not found
+     * @return ($default is null ? string|null : string)
      */
     public function getString(string $key, ?string $default = null): ?string
     {
-        $value = $this->get($key, $default);
+        $value = $this->get($key, $default) ?? $default;
 
         if (!is_string($value) && !is_null($value)) {
             throw new TypeException("Config key '$key' doesn't return string.");
@@ -59,15 +63,17 @@ public function getString(string $key, ?string $default = null): ?string
 
     /**
      * Get the specified configuration value as bool.
+     * Returns null if the key is not found, unless a default is provided, in
+     * which case the return value is type safe (can't be null).
      *
      * @param string   $key
      * @param int|null $default
      *
-     * @return ($default is null ? int|null : int) Returns null if the key is not found
+     * @return ($default is null ? int|null : int)
      */
     public function getInt(string $key, ?int $default = null): ?int
     {
-        $value = $this->get($key, $default);
+        $value = $this->get($key, $default) ?? $default;
 
         if (!is_int($value) && !is_null($value)) {
             throw new TypeException("Config key '$key' doesn't return int.");
@@ -78,15 +84,17 @@ public function getInt(string $key, ?int $default = null): ?int
 
     /**
      * Get the specified configuration value as bool.
+     * Returns null if the key is not found, unless a default is provided, in
+     * which case the return value is type safe (can't be null).
      *
      * @param string       $key
      * @param mixed[]|null $default
      *
-     * @return ($default is null ? mixed[]|null : mixed[]) Returns null if the key is not found
+     * @return ($default is null ? mixed[]|null : mixed[])
      */
     public function getArray(string $key, ?array $default = null): ?array
     {
-        $value = $this->get($key, $default);
+        $value = $this->get($key, $default) ?? $default;
 
         if (!is_array($value) && !is_null($value)) {
             throw new TypeException("Config key '$key' doesn't return array.");

tests/Config/ConfigTest.php

@@ -20,6 +20,7 @@ class ConfigTest extends TestCase
         'string' => 'foobar',
         'int'    => 92,
         'array'  => ['foo', 'bar'],
+        'null'   => null,
     ];
 
     public function testGetBool(): void
@@ -30,6 +31,12 @@ public function testGetBool(): void
         $this->assertSame(false, $repo->getBool('missing', false));
         $this->assertSame(null, $repo->getBool('missing'));
         $this->assertSame($repo->get('missing'), $repo->getBool('missing')); // Same default behavior as "get"
+
+        // Value is null, but not the default. Default should still be used.
+        $this->assertSame(null, $repo->getBool('null'));
+        $this->assertSame(false, $repo->getBool('null', false));
+
+        // Exception
         $this->expectException(TypeException::class);
         $repo->getBool('string');
     }
@@ -42,6 +49,12 @@ public function testGetString(): void
         $this->assertSame('barfoo', $repo->getString('missing', 'barfoo'));
         $this->assertSame(null, $repo->getString('missing'));
         $this->assertSame($repo->get('missing'), $repo->getString('missing')); // Same default behavior as "get"
+
+        // Value is null, but not the default. Default should still be used.
+        $this->assertSame(null, $repo->getString('null'));
+        $this->assertSame('non-null', $repo->getString('null', 'non-null'));
+
+        // Exception
         $this->expectException(TypeException::class);
         $repo->getString('bool');
     }
@@ -54,6 +67,12 @@ public function testGetInt(): void
         $this->assertSame(29, $repo->getInt('missing', 29));
         $this->assertSame(null, $repo->getInt('missing'));
         $this->assertSame($repo->get('missing'), $repo->getInt('missing')); // Same default behavior as "get"
+
+        // Value is null, but not the default. Default should still be used.
+        $this->assertSame(null, $repo->getInt('null'));
+        $this->assertSame(123, $repo->getInt('null', 123));
+
+        // Exception
         $this->expectException(TypeException::class);
         $repo->getInt('string');
     }
@@ -66,6 +85,12 @@ public function testGetArray(): void
         $this->assertSame(['bar', 'foo'], $repo->getArray('missing', ['bar', 'foo']));
         $this->assertSame(null, $repo->getArray('missing'));
         $this->assertSame($repo->get('missing'), $repo->getArray('missing')); // Same default behavior as "get"
+
+        // Value is null, but not the default. Default should still be used.
+        $this->assertSame(null, $repo->getArray('null'));
+        $this->assertSame(['non-null'], $repo->getArray('null', ['non-null']));
+
+        // Exception
         $this->expectException(TypeException::class);
         $repo->getArray('string');
     }