Added ability to baseline violations

[frankdekker]

Added the ability to baseline violations. #2543

Features

Create baseline
phpcs src --report-baseline=phpcs.baseline.xml --basepath=/path/to/project/root

• Writes phpcs.baseline.xml in the current working directory. Pref be the root of the project
• basepath will be subtracted from the filepaths in the baseline xml.

Use default baseline
Will by default search in the working directory for phpcs.baseline.xml
phpcs src

Use custom location baseline
phpcs src --baselineFile=<path/to/baseline.xml>

Changes

Added classes to read the baseline file and setup data structure.
Added check in File::addMessage to check if the message is baselined.
Added --baselineFile to cli options
Added unit tests for the newly added files.

Documentation

My question, how do I update the documentation to add a section about the baseline feature?

[sergey-solo]

Just want to point out that this implementation doesn't baseline individual violations. Instead, it baselines specific sniffs for specific files. Which is different from what one might expect :)

[frankdekker]

Just want to point out that this implementation doesn't baseline individual violations. Instead, it baselines specific sniffs for specific files. Which is different from what one might expect :)

Correct, will point that out in the documentation. It's a technical choice I took to avoid basing it on line number. Because that will become quickly annoying when you modify files :).

[gsherwood]

I'm not convinced that this is a useful way of doing a baseline. When entire sniffs (or even sniff codes) are baselined for a file, adding new code with violations to that file wont show any errors if they happen to be the same sort of errors already in the file. For projects that need a baseline, or when introducing a new check to a standard, I think this is a pretty likely case.

I'd likely only accept a baseline feature if it was able to distinguish between violations in existing code and violations in new code. Anything else feels like an ongoing support issue for me as the feature wont work the way people expect and I'll have to defend the design decision for it. I'm absolutely happy to do this, but only if I believe the feature works the way it should.

This is one of those things where we really need a discussion about the solution design before a PR. There was some of that back on #2543, but it's been a while and no real solution was ever outlined. If you want to give this a go, it would be a terrific feature to have and I'd be happy to discuss any ideas you have for the solution over on that other issue.

[frankdekker]

I'm not convinced that this is a useful way of doing a baseline. When entire sniffs (or even sniff codes) are baselined for a file, adding new code with violations to that file wont show any errors if they happen to be the same sort of errors already in the file. For projects that need a baseline, or when introducing a new check to a standard, I think this is a pretty likely case.

I'd likely only accept a baseline feature if it was able to distinguish between violations in existing code and violations in new code. Anything else feels like an ongoing support issue for me as the feature wont work the way people expect and I'll have to defend the design decision for it. I'm absolutely happy to do this, but only if I believe the feature works the way it should.

This is one of those things where we really need a discussion about the solution design before a PR. There was some of that back on #2543, but it's been a while and no real solution was ever outlined. If you want to give this a go, it would be a terrific feature to have and I'd be happy to discuss any ideas you have for the solution over on that other issue.

Completely understandable. This baseline mechanism will indeed suppress new issues in the same file. I'm happy to brainstorm how to make this feature more robust. The problem at hand is that we need a unique way to detect where the violations came from within the file, without explicitly counting tokens or line numbers.

I'll propose an approach in #2543

[frankdekker]

@gsherwood
Improved the baseline calculation based on the approach I suggested in #2543

Now a code signature will be calculated and added to the baseline report file. The code signature is the hash of the code on the line of the violation, and 1 line before and after.

During standard inspection when comparing against the baseline, the same signature will be calculated for encountered violations and compared against the baseline signature. The violation will only be skipped if the code signature is identical to the one in the baseline.

As mentioned, this will prevent a sniff being disabled for the entire file. The sniff will now be specifically disabled for the calculated code signature. Modifications in the file won't break the baseline, unless you edit one of the 3 lines around the baseline. At this point I feel it's quite useful to resolve the violation anyway, as you're editing that close to it.

I would love to hear your thoughts about this solution.

Regards

Frank Dekker

Example baseline:

<?xml version="1.0" encoding="UTF-8"?> <phpcs-baselineversion="3.6.1"> <violationfile="src\Baseline\BaselineSet.php"sniff="PEAR.Commenting.FunctionComment.MissingParamName"signature="54accce13bb236dc361cf9d75895c274fce619d5"/> <violationfile="src\Baseline\BaselineSet.php"sniff="PEAR.Commenting.FunctionComment.MissingParamTag"signature="25a91da1dadc60a04a9a6615502ad4aa4fd397c2"/> <violationfile="src\Baseline\BaselineSet.php"sniff="PEAR.Functions.FunctionDeclaration.SpaceBeforeOpenParen"signature="edcfdf132eb46b842349404740ba7b8f224d82e1"/> </phpcs-baseline>

[frankdekker]

@gsherwood It's been a while. You had any chance yet to look at the latest proposal with the code signatures?

[Potherca]

Being able to have a baseline would be much appreciated.

@gsherwood Is there anything that can be done to move this forward?

[frankdekker]

Being able to have a baseline would be much appreciated.

Is there anything that can be done to move this forward?

@Potherca
At our company we really needed the baseline, so in the meantime we created the following package:
https://github.com/123inkt/php-codesniffer-baseline

Might be an nice alternative for now.

[Housik]

@gsherwood Hi Greg, would you be please so kind and take a look to Frank solution? It looks like it now mets your requirements about baseline functionality for phpcs. I am one of the many waiting for this feature, it will help us to rewrite large project. Thank you in advance! Martin ;-)

[gsherwood]

@Housik This isn't something I have time to look at.

[BafS]

Is there a chance to see this PR merged? It would be really useful for us too, especially when working with big codebases.

[frankdekker]

Is there a chance to see this PR merged? It would be really useful for us too, especially when working with big codebases.

You can use https://github.com/123inkt/php-codesniffer-baseline in the meantime.

[BafS]

@frankdekker unfortunately it didn't work well with our CI (because of editing a vendor file, which should be immutable).

I created a new report for that, without having the need to change a vendor, it's hacky but it works:

<?phpdeclare(strict_types=1); usePHP_CodeSniffer\Files\File; usePHP_CodeSniffer\Reports\Full; usePHP_CodeSniffer\Reports\Report; finalclass BaselineReport implements Report{publicconstBASELINE_FILE = __DIR__ . '/baseline.php'; privateFull$fullReport; publicfunction__construct(){$this->fullReport = newFull()} privatefunctioncleanReportFromBaseline(array$baseline, array$report): array{$filename = $report['filename']; if (!isset($baseline[$filename])){return$report} $baselineSources = $baseline[$filename]; $messages = &$report['messages']; foreach ($messagesas$ka => $messageLine){foreach ($messageLineas$kb => $messagesOff){foreach ($messagesOffas$kc => $messageList){$source = $messageList['source'] ?? ''; if (isset($baselineSources[$source]) && $baselineSources[$source]['occurrences'] > 0){$type = $messages[$ka][$kb][$kc]['type']; // Decrement the warnings/errors to keep in sync with the removal of messages in the reportmatch ($type){'WARNING' => $report['warnings']--, 'ERROR' => $report['errors']--, }; unset($messages[$ka][$kb][$kc]); $baselineSources[$source]['occurrences']--} } } } return$report} publicfunctiongenerateFileReport($report, File$phpcsFile, $showSources = false, $width = 80): bool{$baseline = []; if (is_file(self::BASELINE_FILE)){$baseline = requireself::BASELINE_FILE; if (!is_array($baseline)){$baseline = []} } $report = $this->cleanReportFromBaseline($baseline, $report); return$this->fullReport->generateFileReport($report, $phpcsFile, $showSources, $width)} publicfunctiongenerate($cachedData, $totalFiles, $totalErrors, $totalWarnings, $totalFixable, $showSources = false, $width = 80, $interactive = false, $toScreen = true): void{// `Full` report only uses $cachedData$this->fullReport->generate($cachedData, $totalFiles, $totalErrors, $totalWarnings, $totalFixable, $showSources, $width, $interactive, $toScreen)} }

Usable like: ./vendor/bin/phpcs '--report-App\Dev\PHP_CodeSniffer\GenerateBaselineReport'.

And to generate the baseline

<?phpdeclare(strict_types=1); usePHP_CodeSniffer\Files\File; usePHP_CodeSniffer\Reports\Report; useApp\Serializer\PhpEncoder; finalclass GenerateBaselineReport implements Report{privatestaticarray$baseline = []; publicfunctiongenerateFileReport($report, File$phpcsFile, $showSources = false, $width = 80): bool{$filename = $report['filename']; /** @var array<string, int> $occurrences */$occurrences = []; $messages = &$report['messages']; foreach ($messagesas$messageLine){foreach ($messageLineas$messagesOff){foreach ($messagesOffas$messageList){$occurrences[$messageList['source']] ??= 0; $occurrences[$messageList['source']]++} } } foreach ($occurrencesas$source => $occurrence){self::$baseline[$filename][$source]['occurrences'] = $occurrence} returntrue} publicfunctiongenerate( $cachedData, $totalFiles, $totalErrors, $totalWarnings, $totalFixable, $showSources = false, $width = 80, $interactive = false, $toScreen = true, ): void{global$argv; $phpEncoder = newPhpEncoder(); $out = $phpEncoder->encode(self::$baseline, PhpEncoder::FORMAT, [ 'strict' => true, ]); $inputString = implode('', $argv); $out = str_replace('return ', "// Generated with: $inputString\nreturn ", $out); echo$out} }

Usable like: ./vendor/bin/phpcs --report-App\\Dev\\PHP_CodeSniffer\\GenerateBaselineReport --sniffs=SlevomatCodingStandard.TypeHints.ReturnTypeHint,SlevomatCodingStandard.TypeHints.DeclareStrictTypes > app/Dev/baseline.php

[peterjaap]

@frankdekker it would be great to also have notices for false positives. See my explanation here; 123inkt/php-codesniffer-baseline#7

PHPStan also does this, it's called "unmatched ignored errors", see https://phpstan.org/user-guide/baseline#generating-the-baseline

@gsherwood what can we do for you to move this PR forward? Maybe @jrfnl can take a look since for some reason primarily Dutch people are active in this PR? 😃

[peterjaap]

@BafS I wanted to give your solution a whirl, but I'm missing the PhpEncoder dep. Judging from the method signature, it's not https://github.com/Riimu/Kit-PHPEncoder?

[BafS]

@peterjaap right, it's from Symfony\Component\Serializer\Encoder\EncoderInterface but with my own implementation. I edited my answer to use the native var_export function.

[forrest79]

Hi to all, thanks for this PR and the discussion in it. Like you, we also need a baseline and better ability to ignore valid errors. I look over all your solutions and prepared a package, that works in a similar way as PHPStan (also with outdated info). There must still be some magic and hacking, but it works without hard changing files in the vendor. We're using this for a couple of weeks in our production CI and everything looks good. You can find it here https://github.com/forrest79/PHPCS-Ignores. I will be glad for your comment and improvement. Still, I'm hoping, that PHPCS adds something like this as a new feature :-)

[frankdekker]

Hi to all, thanks for this PR and the discussion in it. Like you, we also need a baseline and better ability to ignore valid errors. I look over all your solutions and prepared a package, that works in a similar way as PHPStan (also with outdated info). There must still be some magic and hacking, but it works without hard changing files in the vendor. We're using this for a couple of weeks in our production CI and everything looks good. You can find it here https://github.com/forrest79/PHPCS-Ignores. I will be glad for your comment and improvement. Still, I'm hoping, that PHPCS adds something like this as a new feature :-)

Hihi, also nice trick to hook into PHPCS reporting. And same here, still hoping PHPCS comes with a build-in implementation :).

[Morgy93]

This is an interesting approach: https://github.com/isaaceindhoven/php-code-sniffer-baseliner

Alternatives:

• https://github.com/123inkt/php-codesniffer-baseline
• https://github.com/forrest79/PHPCS-Ignores

[DanielRuf]

Does anybody know, if there was some relevant progress for PHPCS baseline generation?

What is currently used by most and recommended regarding this?

• https://github.com/isaaceindhoven/php-code-sniffer-baseliner seems abandoned
• https://github.com/123inkt/php-codesniffer-baseline should be better done via composer-patches IMHO
• https://github.com/forrest79/PHPCS-Ignores seems a bit cumbersome

[peterjaap]

We use the 123inkt one!

Just run this; vendor/bin/phpcs app/design/frontend app/code --extensions=php,phtml --report=\\DR\\CodeSnifferBaseline\\Reports\\Baseline --report-file=phpcs.baseline.xml --basepath=.

And you're good to go.

I'm not sure how composer patches would help here?

[DanielRuf]

@peterjaap regarding composer-patches:

As PHP_Codesniffer doesn't have a nice and clean way to add an extension, this package will inject a single line of code into the /vendor/squizlabs/php_codesniffer/src/Files/File.php upon composer install or composer update. While this is a fragile solution, this is only until #3387 is accepted or another baseline method has been added.

That a package deliberately modifies code of another package is quite interesting. Not sure if this behavior is allowed for composer packages by design and if this will work forever. Applying some patch via sed, patch or git is my preferred solution.

[peterjaap]

@DanielRuf yes that's also a way to go. I prefer having it in one package though so its more easily trackable, but that's personal preference.

[frankdekker]

That a package deliberately modifies code of another package is quite interesting. Not sure if this behavior is allowed for
composer packages by design and if this will work forever. Applying some patch via sed, patch or git is my preferred solution.

I do not know if it's allowed either, but I got this idea from https://github.com/slevomat/coding-standard

which uses: dealerdirect/phpcodesniffer-composer-installer

This packagewill create/update vendor/squizlabs/php_codesniffer/CodeSniffer.conf in the vendor folder to specify the directory for extra ruleset inclusion for phpcs.

I agree modifying code is a whole other level, hence the disclaimer in the package

[tomcat0090]

I'm looking at this pull request with eager eyes.