Extending

Back to the README

Every rule a reviewer keeps asking for by hand belongs here, where it is enforced in every project's pipeline and in the review bot.

Where things go

To add Put it in Register it in
a phpcs sniff Festi/Sniffs/<Category>/<Name>Sniff.php Festi/ruleset.xml
a PHPMD rule src/Phpmd/<Name>.php phpmd/ruleset.xml
a Phan plugin phan/plugins/<Name>Plugin.php phan/project-config.dist.php
a tool in the summary src/Quality/<Name>Tool.php QualityCommandFactory::create()
a CI job ci/static-analysis.gitlab-ci.yml

Adding a phpcs sniff

  1. Write Festi/Sniffs/<Category>/<Name>Sniff.php, class Festi\Sniffs\<Category>\<Name>Sniff, implementing PHP_CodeSniffer\Sniffs\Sniff.
  2. Reference it in Festi/ruleset.xml: <rule ref="Festi.<Category>.<Name>"/>.
  3. Make whatever a project may want to change a public property, so a project ruleset can set it (see checkVariables on EntityIdNaming).
  4. Write its scenarios in tests/Standard/ (Testing).
  5. Describe it in Rules, with a good and a bad example.

Guidelines that have paid for themselves:

  • Write the false positives first. List the code that looks like a violation and is not, make those scenarios pass, then write the rule.
  • Measure before shipping. Run the sniff over two or three real Festi projects. A rule that reports hundreds of hits either pins a convention the code does not follow or needs a narrower definition.
  • Give the fix in the message. $projectId should say $idProject.
  • Be auto-fixable only when the fix cannot change behaviour.
  • Say what the message is about. A sniff that judges a whole function or a signature reports on the function line, which a change usually leaves alone. Add it to WHOLE_FUNCTION_SNIFFS or SIGNATURE_SNIFFS in src/Diff/ChangeScope.php, or it disappears from every changed-lines run.
  • Keep a sniff inside its own directory. A sniff may use a helper in Festi/Sniffs/, not a class in src/: when the standard is loaded by path, phpcs autoloads the standard's directory and nothing else.

Adding a PHPMD rule

Use one of PHPMD's own rules when it measures what is wanted: reference it in phpmd/ruleset.xml and write every threshold out, so the number can be read there.

<rule ref="rulesets/codesize.xml/TooManyFields">
 <properties>
  <property name="maxfields" value="10"/>
 </properties>
</rule>

For a measure PHPMD does not have, write a rule class in src/Phpmd/ extending PHPMD\AbstractRule and declare it by class name:

<rule name="ParameterCount"
      message="The {0} {1} has {2} parameters. Keep it to {3} or fewer."
      class="Festi\CodingStandard\Phpmd\ParameterCount">
 <priority>2</priority>
 <properties>
  <property name="maximum" value="4"/>
 </properties>
</rule>

Scenarios go in tests/Standard/ClassDesignRulesTest.php. A limit must also be shown to be overridable: add the case to tests/Standard/ProjectOverrideTest.php.

Adding a Phan plugin

Write phan/plugins/<Name>Plugin.php extending Phan\PluginV3, return an instance at the end of the file, and add it to the $festiPlugins list in phan/project-config.dist.php.

  • Read anything a project may want to change from Config::getValue('plugin_config'), with the framework's names as the default.
  • Decide by name rather than by ancestry when the framework class may be missing. A tool that analyses a project without its vendor/ directory cannot resolve DataAccessObject; an ancestry test reports nothing there and looks clean, a name test still works.
  • Use a post-analysis visitor on declaration nodes if the rule must work under quick_mode.

Scenarios go in tests/Phan/, and the rule in Rules.

Adding a tool to festi-quality

A tool is one class implementing Quality\QualityTool:

interface QualityTool
{
    public function getName(): string;

    public function inspect(QualityScope $scope): ToolResult;
}

and one line in QualityCommandFactory::create().

A binary that prints a JSON report extends Quality\JsonReportingTool, which finds the binary, runs it and tells a report from a crash. The tool then says three things:

class PhpstanTool extends JsonReportingTool
{
    public function getName(): string
    {
        return 'phpstan';
    }

    protected function getBinaryOverride(): string
    {
        return 'PHPSTAN_BIN';
    }

    protected function getArguments(QualityScope $scope): array
    {
        $arguments = ['analyse', '--error-format=json', '--no-progress'];

        return array_merge($arguments, $scope->getPaths());
    }

    protected function summarise(array $report, QualityScope $scope): ToolResult
    {
        $issues = [];
        foreach ($report['files'] ?? [] as $file => $fileReport) {
            foreach ($fileReport['messages'] as $message) {
                $line = (int) $message['line'];
                $issue = new Issue($file, $line, $line, $message['identifier'], $message['message']);
                if ($scope->covers($issue)) {
                    $issues[] = $issue;
                }
            }
        }

        return ToolResult::found($this->getName(), $issues);
    }
}

The contract a tool keeps:
Never throw A missing or broken tool returns ToolResult::failed(), so one broken tool does not hide what the others found.
Three outcomes found() or ran() when it ran, skipped() when it does not apply, failed() when it broke. An empty report must never stand in for the last two.
Honour the scope Hand $scope->getPaths() to the binary when it takes paths, and keep an issue only when $scope->covers($issue). That one call handles the whole project, named paths and a change alike.
Say where Build an Issue with the file, the line and, for a measure of a whole function or class, its last line. --list and --diff both depend on it.
Say how much For a measure of a whole function or class, call $issue->measuring('<Rule> of <Class::method>', $figure). Wrapped in BaseComparedTool, the tool is then compared with the base revision under --diff. The subject must read the same at both revisions: no line number, no figure.
Count only if you must A source that gives counts without places (SonarQube's facets) returns ToolResult::ran() and says in its note how the scope was applied.

getReasonToSkip() can be overridden for a tool that needs a config file the project may not have.

Asking for a judge

Some rules can tell that a line is wrong and cannot tell what would be right. A rule in that position ends its message with a token:

<llm_as_judge:entity-id-naming>

A reviewing agent that reads the report hands that finding to a language model, which proposes the fix or says the rule does not apply. In a pipeline, and to a person reading the output, the token is a tag at the end of an ordinary message and changes nothing.

Two things keep this safe to use from any sniff or Phan plugin:

  • The token names a topic and nothing else. What the model is asked, and the examples it is shown, belong to the reader's registry under that topic. A report never carries instructions, so code under review cannot steer a model by imitating a tool message.
  • The answer is checked by code. A proposed name is accepted only if it passes the rule that asked.

Use it for the case a rule genuinely cannot decide, never to skip writing the deterministic part: every message with a token costs a model call on each review.

Topic Asked by Question
entity-id-naming Festi.NamingConventions.EntityIdNaming.VariableIdNamingUnclear what to call a variable whose id sits inside its name

Testing

composer install
vendor/bin/phpunit

Tests are written as scenarios: the name says what a developer may rely on, not which method is called.

Directory Proves How
tests/Standard/ what each phpcs and PHPMD rule reports and does not report a snippet is handed to the real phpcs or phpmd with the shipped ruleset
tests/Phan/ what each Phan plugin reports a snippet is analysed by the real phan
tests/Quality/ what festi-quality says, for each scope and each tool outcome the binaries are replaced by recorded reports
tests/Diff/ which lines count as changed, and what festi-phpcs-diff keeps git is replaced by recorded answers

The rule scenarios mock nothing, so they also fail when a ruleset edit switches a rule off. Three helpers run the real tools:

$messages = (new StandardProbe())->sniff('<?php $projectId = 1;');   // phpcs, the Festi standard
$violations = (new PhpmdProbe())->measure($code);                   // PHPMD, the shipped ruleset
$issues = (new PhanProbe($pluginFile))->analyse($code);             // Phan, one plugin

StandardProbe and PhpmdProbe take a ruleset path instead, to prove a project override.

The package follows its own rules

tests/OwnStandardTest.php runs the shipped phpcs standard, PHPMD ruleset and Phan baseline over this repository and expects nothing from any of them. A new rule is therefore met here first, which is how an over-eager sniff or an unworkable limit is found before a consuming project finds it. The same check by hand:

bin/festi-quality

When a new rule fails on the package's own code, fix the code. If the rule is wrong about it, the rule has a false positive and that is the bug.

Releasing

  1. Tag X.Y.Z with no v prefix. Consumers name the tag verbatim in their CI include: ref:, and a ref that does not exist stops their pipeline from being created.
  2. A new rule turns somebody's pipeline red, so it takes a minor version at least.
  3. Bump the ^X.Y constraint and the CI ref: in the consuming projects.