@kylekatarnls @MarkVaughn Would you merge a PR for this? I think it would work quite well in combination with the GitHub renderer (I plan to use both together in my projects).

Hi @lukasbestle by any change, is it the format GitLab uses?
https://docs.gitlab.com/ee/user/project/merge_requests/code_quality.html#implementing-a-custom-tool

@kylekatarnls No, that expects the Code Climate spec, which is not the same as SARIF. SARIF is an official standard while Code Climate is a format of a particular set of tools.

This make me think that the list of rendered could grow very fast if we implement every formats. We rather need to have mapping options.

We have:
#856 which is XML with a custom mapping
SARIF and CodeClimate/GitLab which is JSON with a custom mapping
And maybe #857 could also be use the text with a custom settings (template, prefixes/suffixes)

If we add them all as first-level renderers, we'll have 9 of them. Let's say it would be the very maximum before considering reorganizing this feature.

I see your point, this is definitely something to be considered.

The question is how high the abstraction level should be. I'm generally not a huge fan of abstractions in areas like these as it will make the code much harder to understand, longer than necessary and each change in the "core format" (XML, JSON, text) might break the formats that depend on them.

SARIF is quite specific, it's not just JSON. For example it also includes the rule metadata (description, help text, example, link to the docs etc.). So basically the only thing that would be shared is the json_encode() at the end.

Another argument for keeping a linear list instead of thinking in base formats: As a user I might not even know if I'm looking for a JSON-based format or an XML format. I know that I need SARIF or Checkstyle etc. How these formats work internally is not something I'd consider as a user, it's an implementation detail.

What we could do is to move the renderer registration into a central location. So instead of a method for each renderer and an additional line in the command options for file export, there could be a static array that contains a list of renderer names and classes, which would be easier to maintain and less verbose:

const RENDERERS = [
  'text' => [
    'class'     => '\PHPMD\Renderer\TextRenderer',
    'forExport' => false 
  ],
  'sarif' => [
    'class'     => '\PHPMD\Renderer\SarifRenderer',
    'forExport' => true 
  ],
  // ...
];

Alternatively it could also be a public static so that plugins can register themselves as well.

The forExport attribute would control whether the renderer gets its own --reportfile-{name} option.

I kind of agree with @kylekatarnls in that stuffing the actual library with so many report renderers becomes more and more confusing for users and harder to mantain.

On the other hand, the current approach to use custom renderers isn't really straightforward regarding how should one address the custom renderers class, if it means a path, a fqcn, and if backslashes in its namespace should be escaped. If you aren't running phpmd as a composer dependency (for example if you use a global install, or run it through a linked phive package) it won't see custom renderers unless thay are in the root namespace and the current folder.

Perhaps it would be saner to allow for the cli to pass a config file

I agree that a config file could be useful in general. However I don‘t think that a config file alone can solve the issue of how to get custom renderers loaded. If you declare a class in the config file, PHPMD still needs to know how to load that class.

To be honest I think the ideas we collected in this issue are mostly medium or long term improvements. I don‘t think they should be blockers for new core renderers that are generally useful and not too project-specific.

SARIF as an open and official standard (the format for static analyzer output) should IMO be in the core.

I agree that a config file could be useful in general. However I don‘t think that a config file alone can solve the issue of how to get custom renderers loaded. If you declare a class in the config file, PHPMD still needs to know how to load that class.

As long as the custom renderer keeps the interface contract (if there was one) PHPMD doesn't need to know anyting. In fact, it's working that way right now. I've set up a POC using nothing but

• vanilla PHPMD as of aa7e527,
• your GithubRenderer Add GitHub renderer for GitHub Actions #857
• a Github Action with 4 scenarios
  1.- binary at vendor/bin/phpmd, GitHubRenderer on project root, no namespace
  2.- same as above with globally installed phpmd
  3.- same as 1, but having GithubRenderer on a subfolder and with a namespace
  4. same as 3 with global phpmd binary

It turns out that referencing the custom renderer class in project root and no namespace always works. Namespaced custom renderers will work with local binary, but not with global binary. Why? it is because class existance takes the autoloader in consideration, therefore class does exist.

If it doesn't, including the file doesn't work, because it either gets the path right (but the FQCN wrong) or the other way around. It cannot get both right since method createCustomRenderer assumes the path is just the FQCN with forward slashes relative to cwd

So I wonder if this all could be tackled by adding a config flag like --custom-renderers-folder=<path> so

phpmd src/php/PHPMD  Whatever\Namespace\CustomRenderer cleancode --custom-renderers-folder=/whatever/absolute/or/relative/path

To be honest I think the ideas we collected in this issue are mostly medium or long term improvements. I don‘t think they should be blockers for new core renderers that are generally useful and not too project-specific.

SARIF as an open and official standard (the format for static analyzer output) should IMO be in the core.

I didn't know about SARIF, but I can see now it's a broadly used standard.

Fixed via #865