Merge pull request #52 from ChiragAgg5k/fix/loose-bool-string-coercion

Author: ChiragAgg5k

src/CLI/CLI.php

@@ -7,6 +7,8 @@
 use Utopia\DI\Container;
 use Utopia\Servers\Hook;
 use Utopia\Validator;
+use Utopia\Validator\Boolean;
+use Utopia\Validator\Nullable;
 
 class CLI
 {
@@ -302,7 +304,7 @@ protected function getParams(Hook $hook): array
 
             $this->validate($key, $param, $value);
 
-            $params[$this->camelCaseIt($key)] = $value;
+            $params[$this->camelCaseIt($key)] = $this->coerce($param['validator'], $value);
         }
 
         foreach ($hook->getDependencies() as $dependency) {
@@ -410,6 +412,50 @@ protected function validate(string $key, array $param, $value): void
         }
     }
 
+    /**
+     * Coerce string CLI inputs to native PHP types based on the param's validator.
+     *
+     * CLI args arrive as strings via getopt. When the validator is `Boolean`
+     * (loose mode), strings like "true"/"false"/"1"/"0" pass validation but
+     * remain strings. If the action callback declares a `bool` parameter, PHP's
+     * implicit string-to-bool cast turns any non-empty string except "0" into
+     * `true` -- so `--commit=false` silently becomes `true`. Validators in
+     * utopia-php are pure (validate only, never mutate), so the coercion has
+     * to happen here at the dispatch boundary.
+     *
+     * Only string inputs are coerced; bool defaults are passed through
+     * untouched. Strings that `filter_var` doesn't recognise (including the
+     * empty string, which bypasses `validate()` for optional params) are
+     * passed through unchanged so callers that use `''` as a sentinel for
+     * "not set" keep working.
+     *
+     * @param  Validator|callable  $validator
+     * @param  mixed  $value
+     * @return mixed
+     */
+    protected function coerce(Validator|callable $validator, mixed $value): mixed
+    {
+        if (! is_string($value) || $value === '') {
+            return $value;
+        }
+
+        if (\is_callable($validator)) {
+            $validator = $validator();
+        }
+
+        while ($validator instanceof Nullable) {
+            $validator = $validator->getValidator();
+        }
+
+        if (! ($validator instanceof Boolean)) {
+            return $value;
+        }
+
+        $coerced = \filter_var($value, FILTER_VALIDATE_BOOLEAN, FILTER_NULL_ON_FAILURE);
+
+        return $coerced === null ? $value : $coerced;
+    }
+
     public function setContainer(Container $container): self
     {
         $this->parentContainer = $container;

tests/CLI/CLITest.php

@@ -7,6 +7,8 @@
 use Utopia\CLI\CLI;
 use Utopia\DI\Container;
 use Utopia\Validator\ArrayList;
+use Utopia\Validator\Boolean;
+use Utopia\Validator\Nullable;
 use Utopia\Validator\Text;
 
 class CLITest extends TestCase
@@ -289,6 +291,121 @@ public function testMatch()
         $this->assertEquals(null, $cli->match());
     }
 
+    /**
+     * @return iterable<string, array{0: string, 1: bool}>
+     */
+    public static function looseBooleanValuesProvider(): iterable
+    {
+        yield '"false" string' => ['false', false];
+        yield '"true" string' => ['true', true];
+        yield '"0" string' => ['0', false];
+        yield '"1" string' => ['1', true];
+    }
+
+    /**
+     * Regression: --flag=false used to arrive as the literal string "false",
+     * which PHP's implicit string-to-bool cast turned into `true` at the
+     * `bool $flag` parameter boundary. The CLI dispatcher now coerces string
+     * inputs whose validator is `Boolean` to a real PHP bool.
+     *
+     * @dataProvider looseBooleanValuesProvider
+     */
+    public function testBooleanParamCoercesStringInput(string $input, bool $expected): void
+    {
+        $captured = null;
+
+        $cli = new CLI(new Generic(), ['test.php', 'build', '--commit='.$input]);
+
+        $cli
+            ->task('build')
+            ->param('commit', false, new Boolean(true), 'Commit changes', true)
+            ->action(function (bool $commit) use (&$captured) {
+                $captured = $commit;
+            });
+
+        $cli->run();
+
+        $this->assertSame($expected, $captured);
+    }
+
+    public function testBooleanParamUsesDefaultWhenOmitted(): void
+    {
+        $captured = null;
+
+        $cli = new CLI(new Generic(), ['test.php', 'build']);
+
+        $cli
+            ->task('build')
+            ->param('commit', false, new Boolean(true), 'Commit changes', true)
+            ->action(function (bool $commit) use (&$captured) {
+                $captured = $commit;
+            });
+
+        $cli->run();
+
+        $this->assertFalse($captured);
+    }
+
+    public function testBooleanParamCoercionUnwrapsNullable(): void
+    {
+        $captured = 'untouched';
+
+        $cli = new CLI(new Generic(), ['test.php', 'build', '--commit=false']);
+
+        $cli
+            ->task('build')
+            ->param('commit', null, new Nullable(new Boolean(true)), 'Commit changes', true)
+            ->action(function (bool $commit) use (&$captured) {
+                $captured = $commit;
+            });
+
+        $cli->run();
+
+        $this->assertFalse($captured);
+    }
+
+    /**
+     * Empty-string params bypass `validate()` when optional, so they reach
+     * `coerce()` un-validated. We must NOT silently turn them into `false`
+     * (callers like Cloud's `Patch.php` use `''` as a "not set" sentinel and
+     * later resolve it to `null`/three-state).
+     */
+    public function testBooleanParamPreservesEmptyStringSentinel(): void
+    {
+        $captured = 'untouched';
+
+        $cli = new CLI(new Generic(), ['test.php', 'build']);
+
+        $cli
+            ->task('build')
+            ->param('commit', '', new Boolean(true), 'Commit changes', true)
+            ->action(function ($commit) use (&$captured) {
+                $captured = $commit;
+            });
+
+        $cli->run();
+
+        $this->assertSame('', $captured);
+    }
+
+    public function testNonBooleanValidatorPassesValueThroughUnchanged(): void
+    {
+        $captured = null;
+
+        $cli = new CLI(new Generic(), ['test.php', 'build', '--name=false']);
+
+        $cli
+            ->task('build')
+            ->param('name', '', new Text(64), 'A name')
+            ->action(function (string $name) use (&$captured) {
+                $captured = $name;
+            });
+
+        $cli->run();
+
+        $this->assertSame('false', $captured);
+    }
+
     public function testEscaping()
     {
         ob_start();