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

[rodrigoprimo]

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.

[rodrigoprimo]

@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]

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

[jrfnl]

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

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

[rodrigoprimo]

@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]

Is there a reason for making this property static ?

[rodrigoprimo]

I made this property static because I see its value as common for all instances of this class and not something that will change depending on the instance of the class. Do you see other reasons not to make it static?

Anyway, I had to change it to an object property to be able to manipulate its value when generating the error message (copying the value of the property before manipulating it seemed too much). See #771 (comment)

[jrfnl]

Is the array_values() really needed here ?

[jrfnl]

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

[rodrigoprimo]

Is the array_values() really needed here ?

Ops, no. It was added by mistake.

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

Yes, I think it is slightly better, and I used it in the original version. But then, when I moved to store the list of valid generators in a class property and used implode() to get the string with the name of the generators, I removed "and", assuming that the text is good enough without it, still correct and the code is slightly simpler.

As you are asking, I'm assuming you prefer the version with the "and," so I went ahead and added it. I'm not super happy with my approach using array_pop(), but I couldn't think of anything better. Please let me know if you have another suggestion.

Using array_pop() meant that I had to change Config::$validGenerators to an object property instead of a class property which is related to another comment you left.

[jrfnl]

Ah, but this is supposed to be an unchangeable array (to become a class constant later on as discussed in previous comments) and the use of array_pop() explicitly changes the array (by reference) and would block the future change to make this a class constant. See: https://3v4l.org/QrM61

While the change of the property value would not currently be problematic (as a second --generator=... argument is ignored anyway), it does make the code less stable, so I'd like to see an alternative solution for this.

[rodrigoprimo]

Thanks for your additional comments, @jrfnl. I pushed a new commit addressing all of them. Please take another look when you get a chance.