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.

[spawnia]

So this code quality scan widget already appears for PHPStan?

Yes it does, when using artifacts.reports.codequality in the GitLab CI configuration.

There is no difference between eslint, PHPStan or any other tool - the code quality report is just JSON and can be simulated by manually adding a file.

I recently got feedback from GitLab support, they opened https://gitlab.com/gitlab-org/gitlab/-/issues/578172. It appears that since the check_name feature has initially been added, a regression has now caused it to have no effect.

I totally get your stance on not wanting to implement something that has no effect today. May I suggest the following plan:

• maintain the addition of check_name in this pull request, as it targets the intended behavior of GitLab and does not hurt
• add a temporary workaround of prefixing the description with the identifier in this pull request, together with a reference to https://gitlab.com/gitlab-org/gitlab/-/issues/578172 and a short explainer that it should be reverted when GitLab fixes their bug

This solution would make error identifiers work now, and at the worst show them redundantly when the GitLab bug is fixed (better than nothing). What do you think?

[ondrejmirtes]

I'd be fine with just a comment above the line with check_name that would lead to the URL with the GitLab issue. Then I'd merge it. Thank you!

[ondrejmirtes]

No merge commits please, only rebase

[spawnia]

It is so inviting to click the Update branch button that GitHub offers 😅 Thanks for your consideration of this change, I appreciate having the information in the JSON file now and plan to work around the GitLab bug through some jq post-processing to prepend check_name to description.