Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Feature request] Allow requesting a custom report using the report FQN #1942

Closed
jrfnl opened this issue Mar 20, 2018 · 1 comment · Fixed by #1948
Closed

[Feature request] Allow requesting a custom report using the report FQN #1942

jrfnl opened this issue Mar 20, 2018 · 1 comment · Fixed by #1948

Comments

@jrfnl
Copy link
Contributor

jrfnl commented Mar 20, 2018

Ok, I've been looking into creating some custom reports for external rulesets.

As far as I can see, you can either use one of the reports shipped with PHPCS or point PHPCS to a file which contains a custom report class using --report=./path/to/report-class.php on the command-line or using <arg name="report" value="./path/to/report-class.php"/> in a ruleset.

Now, the problem with that is that this relies on the path to the custom report file being stable and predictable.

  • An absolute path will work, but is not portable.
  • A relative path will work but only when the path is relative to the directory from which PHPCS is called - not when the path is relative to the ruleset/standard which adds the report -.
    • This is fine in a project Composer based install where you can do ./vendor/org/external-standard/Reports/CustomReport.php.
    • This is problematic for registered global Composer installed standards.
    • This is problematic for registered external rulesets installed in any other way.

PHPCS 3.0.1 via commit 1a59851 introduced the ability for external rulesets to add a custom autoload file in a ruleset using:
<autoload>./path/to/relative/file.php</autoload>.

This now allows for some flexibility with regards to loading non-sniff files where PHPCS does not need to translate sniff names to file and class names.

So, having said all that, to make the ability to have custom report formats more portable, I would very much like to suggest to add the option to allow loading custom report classes using their fully qualified classname.

For an external standard to offer a new custom report format, they would need to use the custom autoload option to sort out the loading of the file, but the Reporter.php class could then just instantiate the class based on the passed class name.

To show it in code:

In the external ruleset:

<autoload>./autoload.php</autoload>

<!-- Optionally add the report if it should be the default for this external standard. -->
<!-- Alternatively, an end-user could choose to use the report using
    `--report=\StandardNamespace\Reports\MyCustomReport` on the command line -->
<arg name="report" value="\StandardNamespace\Reports\MyCustomReport"/>

In the autoload.php file for the external standard, something along the lines of:

spl_autoload_register(function ($class) {
    // Only try & load our own classes.
    if (stripos($class, 'StandardNamespace') !== 0) {
        return;
    }

    $file = realpath(__DIR__) . DIRECTORY_SEPARATOR . strtr($class, '\\', DIRECTORY_SEPARATOR) . '.php';

    if (file_exists($file)) {
        include_once $file;
    }
});

To allow for this, the report loading/instantiation logic in the Reporter.php class would need only minimal changes:

  • It would need to detect when a FQN is passed as the report name
  • It should trigger the autoloading of the file using a class_exists() and throw an exception if the class is not found.

if (strpos($type, '.') !== false) {
// This is a path to a custom report class.
$filename = realpath($type);
if ($filename === false) {
$error = "ERROR: Custom report \"$type\" not found".PHP_EOL;
throw new DeepExitException($error, 3);
}
$reportClassName = Autoload::loadFile($filename);
} else {
$reportClassName = 'PHP_CodeSniffer\Reports\\'.$type;
}
$reportClass = new $reportClassName();
if (false === ($reportClass instanceof Report)) {
throw new RuntimeException('Class "'.$reportClassName.'" must implement the "PHP_CodeSniffer\Report" interface.');
}

@gsherwood What do you think ? Would you be open to adding this option ?

If there is a green light, I'd be happy to make the proposed changes to the Reporter.php file.

@gsherwood
Copy link
Member

Would you be open to adding this option ?

Yep.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants