Include identifier in gitlab error format

[spawnia]

As described in the example in https://docs.gitlab.com/ci/testing/code_quality/#code-quality-report-format:

[
  {
    "description": "'unused' is assigned a value but never used.",
    "check_name": "no-unused-vars",
    "fingerprint": "7815696ecbf1c96e6894b779456d330e",
    "severity": "minor",
    "location": {
      "path": "lib/index.js",
      "lines": {
        "begin": 42
      }
    }
  }
]

[spawnia]

Hm, this does not seem to have the desired effect for me.

I tried with a custom formatter in a project and got the following result where check_name is now included:

$ vendor/bin/phpstan analyse -vv --memory-limit=2G --configuration=$CONFIG_FILE --no-progress --error-format=gitLabWithIdentifiers > phpstan-report.json
Result cache not used because the metadata do not match: projectConfig, analysedPaths, composerLocks, composerInstalled, executedFilesHashes
Result cache is saved.
Elapsed time: 3 minutes 18 seconds
Used memory: 4.82 GB
Running after_script
00:01
Running after script...
$ cat phpstan-report.json
[
    {
        "description": "Method App\\Models\\Examination::isOrderEntryTestPatient() should return string but returns bool.",
        "check_name": "return.type",
        "fingerprint": "ace6a50ade5c69cc6067300f8fa486f9fd499184e1ff3f48a9aed96424ec3542",
        "severity": "major",
        "location": {
            "path": "app/Models/Examination.php",
            "lines": {
                "begin": 1085
            }
        }
    }
]

However, the display shown in GitLab did not change:

Perhaps we can include the identifier in the description? And maybe also the tip, if present.

[ondrejmirtes]

I think you should ask GitLab support first.

[Koretech10]

GitLab specifies this in the documentation (https://docs.gitlab.com/ci/testing/code_quality/#code-quality-report-format) :

Name	Type	Description
check_name	String	A unique name representing the check, or rule, associated with this violation.

Is the code quality report handled correctly if there is a NULL value for check_name ?

[spawnia]

@Koretech10 It seems to me that check_name is described in the docs, but is not used for anything. I could not spot a difference in what happens in the UI with it present or not. At least it did not break anything 😅 Maybe they will add something in the future? I can test what happens with null to make sure that also does not break stuff.

[spawnia]

I had some success with the following custom implementation:

<?php declare(strict_types=1);
// See https://github.com/phpstan/phpstan-src/blob/1.12.32/src/Command/ErrorFormatter/GitlabErrorFormatter.php

namespace App\PHPStan;

use Nette\Utils\Json;
use PHPStan\Command\AnalysisResult;
use PHPStan\Command\ErrorFormatter\ErrorFormatter;
use PHPStan\Command\Output;
use PHPStan\File\RelativePathHelper;

/**
 * @see https://docs.gitlab.com/ee/user/project/merge_requests/code_quality.html#implementing-a-custom-tool
 */
final readonly class GitLabWithIdentifiersErrorFormatter implements ErrorFormatter
{
    public function __construct(
        private RelativePathHelper $relativePathHelper,
    ) {}

    public function formatErrors(AnalysisResult $analysisResult, Output $output): int
    {
        $errorsArray = [];

        foreach ($analysisResult->getFileSpecificErrors() as $fileSpecificError) {
            $error = [
                'description' => "{$fileSpecificError->getIdentifier()}: {$fileSpecificError->getMessage()}",
...

services:
  errorFormatter.gitLabWithIdentifiers:
    class: App\PHPStan\GitLabWithIdentifiersErrorFormatter

@ondrejmirtes Given the check_name is ineffective, would you accept it if I were to change the pull request to include the identifier in description?

[ondrejmirtes]

Please ask GitLab support first and then get back. Thanks.

[spawnia]

I did submit a GitLab support request (for my own reference: https://support.gitlab.com/hc/en-us/requests/662951), and am now awaiting feedback.

[spawnia]

GitLab support did answer and sent me a link to https://gitlab.com/gitlab-org/gitlab/-/merge_requests/117402, which introduces both the documentation and usafe of check_name. According to that, it should be used in merge request widgets. While it may not have worked in my project for some reason, the intent is there and adding check_name does not hurt, so I think this pull request is good to go. I plan to contribute a follow up merge request to GitLab to ensure check_name is used wherever possible.

[ondrejmirtes]

So what's this code quality scan widget? How can PHPStan make one?

[spawnia]

The code quality scan widget is just one of a couple of ways that GitLab has to show the results of a code quality report. The role of PHPStan is only to produce the code quality report file, and I believe it should deliver information that is as accurate (already the case) and complete (missing field added through this pull request) as possible. The role of GitLab is then to take that report and display the information within in a useful and hopefully also complete way - that part currently seems bugged, but I plan to fix it in GitLab itself.

[spawnia]

In regards to the GitLab documentation - the widget is described here: https://docs.gitlab.com/ci/testing/code_quality/#merge-request-widget. The part that concerns PHPStan is described here: https://docs.gitlab.com/ci/testing/code_quality/#code-quality-report-format. The check_name field is clearly documented, and as evidenced by the answer from GitLab support and the linked merged request from GitLab, it is supposed to be used that way.

[ondrejmirtes]

So this code quality scan widget already appears for PHPStan? If not, what makes it do appear for eslint but not for PHPStan?

I'm asking because I don't want to merge PR that's essentially noop today, and I generally want to improve the experience for GitLab PHPStan users.