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

missing source attribute in checkstyle errorFormat #1205

Closed
NeilWhitworth opened this issue Jul 11, 2018 · 6 comments
Closed

missing source attribute in checkstyle errorFormat #1205

NeilWhitworth opened this issue Jul 11, 2018 · 6 comments

Comments

@NeilWhitworth
Copy link

missing source attribute in checkstyle errorFormat

The checkstyle errorFormat does not include a source attribute. We are attempting to integrate phpstan with our existing Jenkins build system, and are using a plugin to parse checkstyle xml error reports.

unlike #481 this is not causing any errors parsing the xml file, but the Jenkins UI does not show anything useful.

screen shot from phpcs checkstyle report
image

screen shot from phpstan checkstyle report
image

@edigu
Copy link

edigu commented Mar 28, 2019

Huge +1 on this. I am also running PHPStan against a pretty large legacy codebase and when the total issues found above 10K Jenkins UI becomes unresponsive and finding useful report entries without reducing down the level is really challenging and time consuming.

PHPCompatibility plugin has also both Types and Categories:
PHPCompatibility

And the format is like below:

<error line="X" column="Y" severity="(warning|error|info|ignore)" 
 message=".. PHP has reserved all method names with a double underscore prefix for future use."
 source="PHPCompatibility.FunctionNameRestrictions.ReservedFunctionNames.MethodDoubleUnderscore"/>

It would be nice improvement using same approach in PHPStan. To summarize; there are at least two places can be improved:

  • severity : Everything on PHPStan reports is error. There are different types of severities for less / more important issues: http://checkstyle.sourceforge.net/property_types.html#severity
  • source : Sources can be categorized by their scopes for eg: PHPStan.Classes.ConstructorSignature, or PHPStan.Arrays.RepeatingKeys

I also read @ondrejmirtes 's notes under another PR (closed) with a note that class names of Rules does not meant to be human readable.

I made some experiments locally on this improvement and it definitely requires a design aggrement and plan before randomly start working on a PR.

I would like to hear opinions on this. For eg:

  • introducing new properties such as type, category, severity on Error class
  • having a factory for Error such as Error::createFromRuleAndMessage(Rule $rule, string $message)
  • Mapping severities and categories of the problems detected by rules or rule's itselfs in a statically defined configuration arrays etc.
  • Naming and handling Unknown / Unclassified problems which may came from third-party rules and plugins.
  • Answering questions such as is it too late to introduce an ErrorInterface to have an agreement on getting severity, category, message, line etc.. of the error..

I would like to give a hand on implementing this feature after having a solid plan.

@ondrejmirtes
Copy link
Member

when the total issues found above 10K

You're using PHPStan wrong. You cannot have 10K errors on level 0. If you have them, it might simply be a matter of a wrong autoload config. Get the number of issues on level 0 down to 0 and then increase the level to 1, rinse and repeat.

*+1"s do not get features implemented. The main problem is that rules do not currently have user-friendly identifiers suitable for this, but I'd be able to guide someone how to add them, if they'd be interested in the implementation.

@ondrejmirtes
Copy link
Member

Currently, the extensibility of error messages is planned to be done through RuleError interface and RuleErrorBuilder. Rules can either return string[] or RuleError[].

@scaytrase
Copy link

Hi, can we have at least some constant value for source (like phpstan) field until more specific ideas will be done? currently this missing fields break some parsers, so we have to additionally postprocess the output xml there

@ondrejmirtes
Copy link
Member

Implemented: phpstan/phpstan-src@f66cf5b

Here's the list of identifiers: https://phpstan.org/error-identifiers

More context: #8981

@github-actions
Copy link

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 18, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

4 participants