-
Notifications
You must be signed in to change notification settings - Fork 545
Include identifier in gitlab error format
#4421
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
Include identifier in gitlab error format
#4421
Conversation
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:
imagePerhaps we can include the identifier in the description? And maybe also the tip, if present.
I think you should ask GitLab support first.
Koretech10
commented
Oct 8, 2025
|
GitLab specifies this in the documentation (https://docs.gitlab.com/ci/testing/code_quality/#code-quality-report-format) :
Is the code quality report handled correctly if there is a NULL value for |
@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.
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?
Please ask GitLab support first and then get back. Thanks.
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.
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.
e1c705f to
9cea543
Compare
So what's this code quality scan widget? How can PHPStan make one?
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.
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.
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.
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_namein 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?
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!
As described in the example in https://docs.gitlab.com/ci/testing/code_quality/#code-quality-report-format: ```json [ { "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 } } } ] ``` GitLab currently has a bug that prevents `check_name` from showing, see https://gitlab.com/gitlab-org/gitlab/-/issues/578172. Thus, an explanatory comment and a TODO for its eventual removal were added.
No merge commits please, only rebase
18c3fbe to
6510581
Compare
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.
I did rebase and push a single commit, this should be good to go. The failing test cases seem unrelated to me.
99020f8
into
phpstan:2.1.x
Thank you!
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 } } } ]