Config: display user-friendly message when invalid generator is passed #771

rodrigoprimo · 2024-12-13T11:56:51Z

Description

This PR improves how Config::processLongArgument() handles the --generator parameter to display a user-friendly message if an invalid generator name is passed. Before, an invalid generator name caused a fatal error.

I have checked, and it is not possible to add custom generators using third-party code (at least not without adding files to the PHPCS directory). This is because the Runner class only loads generators found in the src/Generator/ directory:

PHP_CodeSniffer/src/Runner.php

Lines 97 to 98 in 3d14d00

    
           $class     = 'PHP_CodeSniffer\Generators\\'.$this->config->generator; 
        
           $generator = new $class($ruleset);

So, it should be fine to limit the list of valid generators added in this PR to only the three generators provided by PHPCS.

Suggested changelog entry

Config: display a user-friendly message if an invalid generator name is passed in the command line

Related issues/external references

Fixes #709

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)
- This change is only breaking for integrators, not for external standards or end-users.
Documentation improvement

PR checklist

I have checked there is no other PR open for the same change.
I have read the Contribution Guidelines.
I grant the project the right to include and distribute the code under the BSD-3-Clause license (and I have the right to grant these rights).
I have added tests to cover my changes.
I have verified that the code complies with the projects coding standards.
[Required for new sniffs] I have added XML documentation for the sniff.

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.

rodrigoprimo · 2024-12-13T12:00:04Z

@jrfnl, while working on this PR, I noticed that, besides what I reported in #709, it is possible to trigger a different PHP fatal error if Generator is passed as the name of the generator:

$ phpcs test.php --generator=Generator --standard=Generic
PHP Fatal error:  Uncaught Error: Cannot instantiate abstract class PHP_CodeSniffer\Generators\Generator in src/Runner.php:98
Stack trace:
#0 bin/phpcs(14): PHP_CodeSniffer\Runner->runPHPCS()
#1 {main}
  thrown in src/Runner.php on line 98

This happens because PHPCS tries to instantiate anything that it finds in the src/Generator directory:

PHP_CodeSniffer/src/Runner.php

Lines 97 to 98 in 3d14d00

    
           $class     = 'PHP_CodeSniffer\Generators\\'.$this->config->generator; 
        
           $generator = new $class($ruleset);

The change suggested in this PR prevents this fatal error from happening, at least when using PHPCS in the command line. But I'm mentioning it anyway in case we want to do something else about it.

rodrigoprimo · 2024-12-13T12:03:42Z

Looking into the unrelated test that failed in the CI build.

jrfnl · 2024-12-13T12:21:19Z

Looking into the unrelated test that failed in the CI build.

Eh.. the test failure is not unrelated...

jrfnl · 2024-12-13T12:22:38Z

tests/Core/Ruleset/ShowSniffDeprecationsTest.php

@@ -153,7 +153,7 @@ public static function dataDeprecatedSniffsListDoesNotShowNeedsCsMode()
        return [
            'Standard using deprecated sniffs; documentation is requested' => [
                'standard'       => __DIR__.'/ShowSniffDeprecationsTest.xml',
-                'additionalArgs' => ['--generator=text'],
+                'additionalArgs' => ['--generator=Text'],


Please undo this change. This is perfectly valid. Your PR needs a fix, not this unrelated test.

Eh.. the test failure is not unrelated...

Indeed, I chose a poor word to describe what I meant.

Please undo this change. This is perfectly valid. Your PR needs a fix, not this unrelated test.

@jrfnl, just checking, did you check the commit message? Do you disagree with my assessment describing why I updated the test?

I did not, but have now and I don't agree with your assessment.

The basic premise for the CLI arguments is that they should be handled case-insensitively, so text is perfectly valid and should be accepted.

But on master, phpcs --standard=Generic --generator=text does not work and throws a fatal error. Or maybe it throws a fatal error on Linux (case-sensitive OS) but not on Windows (case-insensitive OS)?

If that is the case, I failed to consider case-insensitive operating systems in this PR. I'm not sure what would be the best approach for this PR to accommodate this.

Maybe perform a case-insensitive check for the generator name when running on Windows and a case-sensitive check when running on other systems? What I don't like about this approach is that PHPCS would continue to behave differently depending on the OS.

Another option is to change PHPCS to ensure that it behaves consistently across different operating systems and handles the --generator CLI argument case-insensitively.

The basic premise for the CLI arguments is that they should be handled case-insensitively

I was not aware of that and I can think of one other example of a CLI argument that is not case-insensitive when PHPCS is running on Linux: --standard=generic. I'm not sure if there are other cases.

phpcs --generator=Text --standard=generic ERROR: the "generic" coding standard is not installed. The installed coding standards are MySource, PEAR, PSR1, PSR2, PSR12, Squiz, Zend and MyStandard

But on master, phpcs --standard=Generic --generator=text does not work and throws a fatal error. Or maybe it throws a fatal error on Linux (case-sensitive OS) but not on Windows (case-insensitive OS)?

Exactly. It is currently (unintentionally) OS-dependent, but shouldn't be.

Another option is to change PHPCS to ensure that it behaves consistently across different operating systems and handles the --generator CLI argument case-insensitively.

IMO, that would be the correct thing to do.

The basic premise for the CLI arguments is that they should be handled case-insensitively

I was not aware of that and I can think of one other example of a CLI argument that is not case-insensitive when PHPCS is running on Linux: --standard=generic. I'm not sure if there are other cases.

That's outside the scope of this PR. Might be worth opening an issue to investigate.

Re: CLI and case-insensitive: squizlabs/PHP_CodeSniffer#1656 (comment)

Thanks for the additional information. I will drop the commit that changed testDeprecatedSniffsListDoesNotShowNeedsCsMode() and update this PR to ensure that the error displayed in Config::processLongArgument() for invalid generator names treats the value of the --generator argument case-insensitively.

Another option is to change PHPCS to ensure that it behaves consistently across different operating systems and handles the --generator CLI argument case-insensitively.

IMO, that would be the correct thing to do.

To implement this, I believe it will be necessary to change Runner::runPHPCS() to load the correct generator class no matter what is the case of the passed generator argument. This is a separate change not directly related to #709. Do you prefer to review this change in this PR or a separate PR?

Why do you think the case-(in)sensitively should be handled in the Runner class instead of in the Config class ?

Because I did not consider normalizing the input in the Config class when setting Config::generator, and was thinking about doing that in the Runner class.

To illustrate what I had in mind, assuming --generator=text is passed, my plan was to do a case-insensitive check in the Config class to see if the generator name passed is valid. Then, on the Runner class the code normalizes the generator name (i.e, modifies the generator name to match the case of the desired file/class name).

After seeing your question, I realized that it makes sense to normalize the generator name in the Config class, and that is probably what you had in mind. Is this correct?

I was also missing that Runner::runPHPCS() creates an instance of Config. Since I was missing that, I was worried about runPHPCS() being called directly and attempting to instantiate a generator without the validation and normalization performed in the Config class.

rodrigoprimo · 2024-12-18T15:01:18Z

@jrfnl, I dropped the commit that changed testDeprecatedSniffsListDoesNotShowNeedsCsMode() and added a new commit to ensure that the code handles the --generator argument in a case-insensitive way. This PR is ready for another review.

jrfnl

@rodrigoprimo Thanks for updating this. I have left some comments inline, mostly nitpicks/dotting of the i's and crossing the t's.

Other than that, this is looking good to go into the 3.12.0 release.

jrfnl · 2024-12-19T00:32:02Z

src/Config.php

+     *
+     * @var array<string, string>
+     */
+    private static $validGenerators = [


Is there a reason for making this property static ?

jrfnl · 2024-12-19T00:32:38Z

src/Config.php

+     * Note: once support for PHP < 5.6 is dropped, this property should be refactored into a class
+     * constant.


Suggested change

* Note: once support for PHP < 5.6 is dropped, this property should be refactored into a class

* constant.

* {@internal Once support for PHP < 5.6 is dropped, this property should be refactored into a class

* constant.}

jrfnl · 2024-12-19T00:34:19Z

src/Config.php

+     * - Keys: lowercase version of the generator name.
+     * - Values: name of the generator PHP class.


Should this be part of the "long description" of the variable or part of the description of the @var type ?

jrfnl · 2024-12-19T00:35:19Z

src/Config.php

+                $lowerCaseGeneratorName = strtolower($generatorName);
+
+                if (isset(self::$validGenerators[$lowerCaseGeneratorName]) === false) {
+                    $validOptions = implode(', ', array_values(self::$validGenerators));


Is the array_values() really needed here ?

Also wondering if for readability, the last comma should be replaced with " and " ?

jrfnl · 2024-12-19T00:36:40Z

src/Config.php

+                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,


Just a suggestion...

Suggested change

'ERROR: "%s" is not a valid generator. Valid options are: %s.'.PHP_EOL.PHP_EOL,

'ERROR: "%s" is not a valid generator. The following generators are supported: %s.'.PHP_EOL.PHP_EOL,

jrfnl · 2024-12-19T00:40:10Z

tests/Core/Config/GeneratorArgTest.php

@@ -23,37 +23,59 @@ 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.


Suggested change

* @param string $argumentValue Generator name passed in the command line.

* @param string $argumentValue Generator name passed on the command line.

jrfnl · 2024-12-19T00:41:39Z

tests/Core/Config/GeneratorArgTest.php

     *
     * @return array<int, array<string>>
     */
-    public static function dataGeneratorNames()
+    public static function dataValidGeneratorNames()
    {
        return [


As this array is now a little more involved, I think it would make it more straight-forward to maintain and to read the tests if the array included test case names + argument names.

github-actions bot added Type: enhancement Core Component: Config & Ruleset & CLI options Status: triage labels Dec 13, 2024

rodrigoprimo removed the Status: triage label Dec 13, 2024

jrfnl reviewed Dec 13, 2024

View reviewed changes

jrfnl added the Status: awaiting feedback label Dec 13, 2024

rodrigoprimo mentioned this pull request Dec 18, 2024

Ensure PHPCS handles CLI arguments case-insensitively across operating systems #774

Open

Ensures the generator CLI parameter is handled case-insensitively

6bb661d

rodrigoprimo force-pushed the generator-param-error-handling branch from 7141e6d to 6bb661d Compare December 18, 2024 14:58

rodrigoprimo removed the Status: awaiting feedback label Dec 18, 2024

jrfnl requested changes Dec 19, 2024

View reviewed changes

jrfnl added this to the 3.12.0 milestone Dec 19, 2024

jrfnl added the Status: awaiting feedback label Dec 19, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Config: display user-friendly message when invalid generator is passed #771

Config: display user-friendly message when invalid generator is passed #771

rodrigoprimo commented Dec 13, 2024

rodrigoprimo commented Dec 13, 2024

rodrigoprimo commented Dec 13, 2024

jrfnl commented Dec 13, 2024

jrfnl Dec 13, 2024

rodrigoprimo Dec 13, 2024 •

edited

Loading

jrfnl Dec 13, 2024

rodrigoprimo Dec 13, 2024

jrfnl Dec 13, 2024

jrfnl Dec 13, 2024

rodrigoprimo Dec 13, 2024

jrfnl Dec 13, 2024

rodrigoprimo Dec 13, 2024

rodrigoprimo commented Dec 18, 2024

jrfnl left a comment

jrfnl Dec 19, 2024

jrfnl Dec 19, 2024

jrfnl Dec 19, 2024

jrfnl Dec 19, 2024

jrfnl Dec 19, 2024

jrfnl Dec 19, 2024

jrfnl Dec 19, 2024

jrfnl Dec 19, 2024

	$class = 'PHP_CodeSniffer\Generators\\'.$this->config->generator;
	$generator = new $class($ruleset);

		* Note: once support for PHP < 5.6 is dropped, this property should be refactored into a class
		* constant.

		* - Keys: lowercase version of the generator name.
		* - Values: name of the generator PHP class.

	'ERROR: "%s" is not a valid generator. Valid options are: %s.'.PHP_EOL.PHP_EOL,
	'ERROR: "%s" is not a valid generator. The following generators are supported: %s.'.PHP_EOL.PHP_EOL,

	* @param string $argumentValue Generator name passed in the command line.
	* @param string $argumentValue Generator name passed on the command line.

Config: display user-friendly message when invalid generator is passed #771

Are you sure you want to change the base?

Config: display user-friendly message when invalid generator is passed #771

Conversation

rodrigoprimo commented Dec 13, 2024

Description

Suggested changelog entry

Related issues/external references

Types of changes

PR checklist

rodrigoprimo commented Dec 13, 2024

rodrigoprimo commented Dec 13, 2024

jrfnl commented Dec 13, 2024

Choose a reason for hiding this comment

rodrigoprimo Dec 13, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rodrigoprimo commented Dec 18, 2024

jrfnl left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rodrigoprimo Dec 13, 2024 •

edited

Loading