From 3b6c8c6ecb194b0715dae30856cd42e2a18b125c Mon Sep 17 00:00:00 2001 From: Rodrigo Primo Date: Fri, 22 Nov 2024 14:10:42 -0300 Subject: [PATCH 1/3] Config: display user-friendly message when invalid generator is passed This commit improves how `Config::processLongArgument()` handles the `--generator` parameter. Now it will show a user-friendly message if an invalid generator name is passed. Before, an invalid generator name caused a fatal error. --- src/Config.php | 15 ++++++- tests/Core/Config/GeneratorArgTest.php | 61 +++++++++++++++++++++++--- 2 files changed, 68 insertions(+), 8 deletions(-) diff --git a/src/Config.php b/src/Config.php index 2bcc78e6ec..8a94edf22c 100644 --- a/src/Config.php +++ b/src/Config.php @@ -1233,7 +1233,20 @@ public function processLongArgument($arg, $pos) break; } - $this->generator = substr($arg, 10); + $generatorName = substr($arg, 10); + $validGenerators = [ + 'Text', + 'HTML', + 'Markdown', + ]; + + if (in_array($generatorName, $validGenerators, true) === false) { + $error = 'ERROR: "'.$generatorName.'" is not a valid generator. Valid options are: Text, HTML, and Markdown.'.PHP_EOL.PHP_EOL; + $error .= $this->printShortUsage(true); + throw new DeepExitException($error, 3); + } + + $this->generator = $generatorName; self::$overriddenDefaults['generator'] = true; } else if (substr($arg, 0, 9) === 'encoding=') { if (isset(self::$overriddenDefaults['encoding']) === true) { diff --git a/tests/Core/Config/GeneratorArgTest.php b/tests/Core/Config/GeneratorArgTest.php index f0b52db157..389d3e4af4 100644 --- a/tests/Core/Config/GeneratorArgTest.php +++ b/tests/Core/Config/GeneratorArgTest.php @@ -25,27 +25,27 @@ final class GeneratorArgTest extends TestCase * * @param string $generatorName Generator name. * - * @dataProvider dataGeneratorNames + * @dataProvider dataValidGeneratorNames * * @return void */ - public function testGenerators($generatorName) + public function testValidGenerators($generatorName) { $config = new ConfigDouble(["--generator=$generatorName"]); $this->assertSame($generatorName, $config->generator); - }//end testGenerators() + }//end testValidGenerators() /** - * Data provider for testGenerators(). + * Data provider for testValidGenerators(). * - * @see self::testGenerators() + * @see self::testValidGenerators() * * @return array> */ - public static function dataGeneratorNames() + public static function dataValidGeneratorNames() { return [ ['Text'], @@ -53,7 +53,7 @@ public static function dataGeneratorNames() ['Markdown'], ]; - }//end dataGeneratorNames() + }//end dataValidGeneratorNames() /** @@ -76,4 +76,51 @@ public function testOnlySetOnce() }//end testOnlySetOnce() + /** + * Ensure that an exception is thrown for an invalid generator. + * + * @param string $generatorName Generator name. + * + * @dataProvider dataInvalidGeneratorNames + * + * @return void + */ + public function testInvalidGenerator($generatorName) + { + $exception = 'PHP_CodeSniffer\Exceptions\DeepExitException'; + $message = 'ERROR: "'.$generatorName.'" is not a valid generator. Valid options are: Text, HTML, and Markdown.'; + + if (method_exists($this, 'expectException') === true) { + // PHPUnit 5+. + $this->expectException($exception); + $this->expectExceptionMessage($message); + } else { + // PHPUnit 4. + $this->setExpectedException($exception, $message); + } + + new ConfigDouble(["--generator={$generatorName}"]); + + }//end testInvalidGenerator() + + + /** + * Data provider for testInvalidGenerator(). + * + * @see self::testInvalidGenerator() + * + * @return array> + */ + public static function dataInvalidGeneratorNames() + { + return [ + ['InvalidGenerator'], + ['Text,HTML'], + ['TEXT'], + [''], + ]; + + }//end dataInvalidGeneratorNames() + + }//end class From 6bb661dcd7a9654bf43ee41ac7dfcc1e5ebd5410 Mon Sep 17 00:00:00 2001 From: Rodrigo Primo Date: Wed, 18 Dec 2024 11:55:35 -0300 Subject: [PATCH 2/3] Ensures the generator CLI parameter is handled case-insensitively --- src/Config.php | 40 +++++++++++++++++++------- tests/Core/Config/GeneratorArgTest.php | 39 +++++++++++++++++++------ 2 files changed, 59 insertions(+), 20 deletions(-) diff --git a/src/Config.php b/src/Config.php index 8a94edf22c..cfbae896b9 100644 --- a/src/Config.php +++ b/src/Config.php @@ -198,6 +198,23 @@ class Config */ private static $executablePaths = []; + /** + * A list of valid generators. + * + * - Keys: lowercase version of the generator name. + * - Values: name of the generator PHP class. + * + * Note: once support for PHP < 5.6 is dropped, this property should be refactored into a class + * constant. + * + * @var array + */ + private static $validGenerators = [ + 'text' => 'Text', + 'html' => 'HTML', + 'markdown' => 'Markdown', + ]; + /** * Get the value of an inaccessible property. @@ -1233,20 +1250,21 @@ public function processLongArgument($arg, $pos) break; } - $generatorName = substr($arg, 10); - $validGenerators = [ - 'Text', - 'HTML', - 'Markdown', - ]; - - if (in_array($generatorName, $validGenerators, true) === false) { - $error = 'ERROR: "'.$generatorName.'" is not a valid generator. Valid options are: Text, HTML, and Markdown.'.PHP_EOL.PHP_EOL; - $error .= $this->printShortUsage(true); + $generatorName = substr($arg, 10); + $lowerCaseGeneratorName = strtolower($generatorName); + + if (isset(self::$validGenerators[$lowerCaseGeneratorName]) === false) { + $validOptions = implode(', ', array_values(self::$validGenerators)); + $error = sprintf( + 'ERROR: "%s" is not a valid generator. Valid options are: %s.'.PHP_EOL.PHP_EOL, + $generatorName, + $validOptions + ); + $error .= $this->printShortUsage(true); throw new DeepExitException($error, 3); } - $this->generator = $generatorName; + $this->generator = self::$validGenerators[$lowerCaseGeneratorName]; self::$overriddenDefaults['generator'] = true; } else if (substr($arg, 0, 9) === 'encoding=') { if (isset(self::$overriddenDefaults['encoding']) === true) { diff --git a/tests/Core/Config/GeneratorArgTest.php b/tests/Core/Config/GeneratorArgTest.php index 389d3e4af4..72949409e9 100644 --- a/tests/Core/Config/GeneratorArgTest.php +++ b/tests/Core/Config/GeneratorArgTest.php @@ -23,17 +23,18 @@ final class GeneratorArgTest extends TestCase /** * Ensure that the generator property is set when the parameter is passed a valid value. * - * @param string $generatorName Generator name. + * @param string $argumentValue Generator name passed in the command line. + * @param string $expectedPropertyValue Expected value of the generator property. * * @dataProvider dataValidGeneratorNames * * @return void */ - public function testValidGenerators($generatorName) + public function testValidGenerators($argumentValue, $expectedPropertyValue) { - $config = new ConfigDouble(["--generator=$generatorName"]); + $config = new ConfigDouble(["--generator=$argumentValue"]); - $this->assertSame($generatorName, $config->generator); + $this->assertSame($expectedPropertyValue, $config->generator); }//end testValidGenerators() @@ -48,9 +49,30 @@ public function testValidGenerators($generatorName) public static function dataValidGeneratorNames() { return [ - ['Text'], - ['HTML'], - ['Markdown'], + [ + 'Text', + 'Text', + ], + [ + 'HTML', + 'HTML', + ], + [ + 'Markdown', + 'Markdown', + ], + [ + 'TEXT', + 'Text', + ], + [ + 'tEXt', + 'Text', + ], + [ + 'html', + 'HTML', + ], ]; }//end dataValidGeneratorNames() @@ -88,7 +110,7 @@ public function testOnlySetOnce() public function testInvalidGenerator($generatorName) { $exception = 'PHP_CodeSniffer\Exceptions\DeepExitException'; - $message = 'ERROR: "'.$generatorName.'" is not a valid generator. Valid options are: Text, HTML, and Markdown.'; + $message = 'ERROR: "'.$generatorName.'" is not a valid generator. Valid options are: Text, HTML, Markdown.'; if (method_exists($this, 'expectException') === true) { // PHPUnit 5+. @@ -116,7 +138,6 @@ public static function dataInvalidGeneratorNames() return [ ['InvalidGenerator'], ['Text,HTML'], - ['TEXT'], [''], ]; From 541e2efb0e5505262a78ca6c27101e231b4ad729 Mon Sep 17 00:00:00 2001 From: Rodrigo Primo Date: Thu, 19 Dec 2024 19:22:35 -0300 Subject: [PATCH 3/3] Apply suggestions from code review Co-authored-by: Juliette <663378+jrfnl@users.noreply.github.com> --- src/Config.php | 46 +++++++++++++------------- tests/Core/Config/GeneratorArgTest.php | 40 +++++++++++----------- 2 files changed, 43 insertions(+), 43 deletions(-) diff --git a/src/Config.php b/src/Config.php index cfbae896b9..7e3714555c 100644 --- a/src/Config.php +++ b/src/Config.php @@ -170,6 +170,21 @@ class Config */ private $cliArgs = []; + /** + * A list of valid generators. + * + * {@internal Once support for PHP < 5.6 is dropped, this property should be refactored into a + * class constant.} + * + * @var array Keys are the lowercase version of the generator name, while values + * are the associated PHP generator class. + */ + private $validGenerators = [ + 'text' => 'Text', + 'html' => 'HTML', + 'markdown' => 'Markdown', + ]; + /** * Command line values that the user has supplied directly. * @@ -198,23 +213,6 @@ class Config */ private static $executablePaths = []; - /** - * A list of valid generators. - * - * - Keys: lowercase version of the generator name. - * - Values: name of the generator PHP class. - * - * Note: once support for PHP < 5.6 is dropped, this property should be refactored into a class - * constant. - * - * @var array - */ - private static $validGenerators = [ - 'text' => 'Text', - 'html' => 'HTML', - 'markdown' => 'Markdown', - ]; - /** * Get the value of an inaccessible property. @@ -1253,18 +1251,20 @@ public function processLongArgument($arg, $pos) $generatorName = substr($arg, 10); $lowerCaseGeneratorName = strtolower($generatorName); - if (isset(self::$validGenerators[$lowerCaseGeneratorName]) === false) { - $validOptions = implode(', ', array_values(self::$validGenerators)); - $error = sprintf( - 'ERROR: "%s" is not a valid generator. Valid options are: %s.'.PHP_EOL.PHP_EOL, + if (isset($this->validGenerators[$lowerCaseGeneratorName]) === false) { + $lastOption = array_pop($this->validGenerators); + $validOptions = implode(', ', $this->validGenerators); + $validOptions .= ' and '.$lastOption; + $error = sprintf( + 'ERROR: "%s" is not a valid generator. The following generators are supported: %s.'.PHP_EOL.PHP_EOL, $generatorName, $validOptions ); - $error .= $this->printShortUsage(true); + $error .= $this->printShortUsage(true); throw new DeepExitException($error, 3); } - $this->generator = self::$validGenerators[$lowerCaseGeneratorName]; + $this->generator = $this->validGenerators[$lowerCaseGeneratorName]; self::$overriddenDefaults['generator'] = true; } else if (substr($arg, 0, 9) === 'encoding=') { if (isset(self::$overriddenDefaults['encoding']) === true) { diff --git a/tests/Core/Config/GeneratorArgTest.php b/tests/Core/Config/GeneratorArgTest.php index 72949409e9..f0fffd033f 100644 --- a/tests/Core/Config/GeneratorArgTest.php +++ b/tests/Core/Config/GeneratorArgTest.php @@ -23,7 +23,7 @@ final class GeneratorArgTest extends TestCase /** * Ensure that the generator property is set when the parameter is passed a valid value. * - * @param string $argumentValue Generator name passed in the command line. + * @param string $argumentValue Generator name passed on the command line. * @param string $expectedPropertyValue Expected value of the generator property. * * @dataProvider dataValidGeneratorNames @@ -49,29 +49,29 @@ public function testValidGenerators($argumentValue, $expectedPropertyValue) public static function dataValidGeneratorNames() { return [ - [ - 'Text', - 'Text', + 'Text generator passed' => [ + 'argumentValue' => 'Text', + 'expectedPropertyValue' => 'Text', ], - [ - 'HTML', - 'HTML', + 'HTML generator passed' => [ + 'argumentValue' => 'HTML', + 'expectedPropertyValue' => 'HTML', ], - [ - 'Markdown', - 'Markdown', + 'Markdown generator passed' => [ + 'argumentValue' => 'Markdown', + 'expectedPropertyValue' => 'Markdown', ], - [ - 'TEXT', - 'Text', + 'Uppercase Text generator passed' => [ + 'argumentValue' => 'TEXT', + 'expectedPropertyValue' => 'Text', ], - [ - 'tEXt', - 'Text', + 'Mixed case Text generator passed' => [ + 'argumentValue' => 'tEXt', + 'expectedPropertyValue' => 'Text', ], - [ - 'html', - 'HTML', + 'Lowercase HTML generator passed' => [ + 'argumentValue' => 'html', + 'expectedPropertyValue' => 'HTML', ], ]; @@ -110,7 +110,7 @@ public function testOnlySetOnce() public function testInvalidGenerator($generatorName) { $exception = 'PHP_CodeSniffer\Exceptions\DeepExitException'; - $message = 'ERROR: "'.$generatorName.'" is not a valid generator. Valid options are: Text, HTML, Markdown.'; + $message = 'ERROR: "'.$generatorName.'" is not a valid generator. The following generators are supported: Text, HTML and Markdown.'; if (method_exists($this, 'expectException') === true) { // PHPUnit 5+.