Rules

Back to the README

What each rule asks for. To change a limit or switch a rule off, see Configuration.

The Festi standard (phpcs)

Layout and syntax

Taken from the Generic, PEAR and Squiz standards:

  • Allman braces for functions and classes; PEAR control-structure and function-call signatures.
  • No tab indent, no short open tag, Unix line endings.
  • No global functions.
  • Lines of at most 120 characters.
  • Short array syntax: [], not array() (auto-fixable).

Naming

Rule Asks for Good Bad
Festi.NamingConventions.PrivateMemberUnderscore a private property starts with _, promoted constructor parameters included private string $_name; private string $name;
Festi.NamingConventions.PrivateMethodUnderscore a private method starts with _ private function _load() private function load()
Festi.NamingConventions.MethodNameConnectiveWord no Or, And or One as a word of a method name; a method does one thing findUser() findOrCreateUser()
Festi.NamingConventions.EntityIdNaming entity ids named by one convention, below

Only private takes the underscore. protected is part of a class's contract with its subclasses, so like public it stays unprefixed. Test methods are exempt from the connective-word rule.

Entity ids

Kind Form Good Bad
Method ID suffix getProjectID() getIdProject(), getUserId()
Constant _ID suffix FIELD_AGENT_ID FIELD_ID_AGENT, ID_PROJECT
Variable holding an id $id prefix $idProject, $_idProject $projectId, $projectID
Variable mentioning an id ID, ending the name $keysByID, $cacheByUserID $keysById, $userIdCache

So a DTO pairs the property $_idProject with the getter getProjectID(). Only a constant's name is checked, never its value: const FIELD_AGENT_ID = 'id_agent' is fine. getIdentifier(), $identity and isValid() are not ids and are left alone, as is the bare FIELD_ID.

Id is never written in a variable name. What the name becomes depends on what the variable holds:

Written Holds Becomes
$projectId, $userID an id $idProject, $idUser
$resolvedByUserId an id: the user who resolved it $idResolvedByUser
$keysById, $uidByCallId a map keyed by id $keysByID, $uidByCallID
$postWithoutAnId, $byToolId something related to an id $postWithoutAnID, $byToolID
$hasParentId a yes-or-no $hasParentID
$lastEventId = [...] an array $lastEventID
$userIdCache, $rowProjectIdLookup a cache, a lookup reorder it: $cacheByUserID

The message carries the exact new name, except for the last row. There the words have to be reordered so that what the variable holds comes first and ID ends the name, which depends on what the variable holds. That case is reported as VariableIdNamingUnclear with the pattern to follow, and asks for a judge.

Every use of a badly named variable is reported, so one wrong name in a long method is several errors.

Documentation

Festi.Commenting.PublicMethodDocBlock — every public method has a docblock with an @param for each parameter and an @return.

/**
 * @param int $idProject Project to load.
 *
 * @return Project
 */
public function getProject(int $idProject): Project

@return is not needed for a void or never method. {@inheritDoc} is accepted. private, protected, magic (__*) and test* methods are not checked, and neither is any file under tests/.

This is about documentation being present, which a type checker does not ask for: a fully typed getProjectID(): int satisfies PHPStan and Phan with no docblock at all.

Arrays

Rule Asks for Good Bad
Festi.Arrays.MultiLineArrayElements an array with more than one element is written one element per line see below ['a' => 1, 'b' => 2]
Festi.Arrays.EmptyArrayComparison check emptiness with empty() if (empty($rows)) if ($rows === [])
Festi.Functions.NoArrayArgument no array literal as a call argument: name it first, or pass a value object see below send(['to' => $user]) 
// MultiLineArrayElements, with ArrayIndent and the Squiz newline rules
$options = [
    'to' => $user,
    'subject' => $subject,
];

// NoArrayArgument: the array gets a name before it is passed
$mailer->send($options);

A destructuring target ([$a, $b] = f();) is not an array literal and is left alone. A comparison against a non-empty literal ($a === [1, 2]) is a real value check and is left alone too.

empty() is broader than === []: it is also true for 0, '0', '', null and false. On a value that is always an array the two agree. Where a scalar can legitimately reach the comparison, keep the strict form and add // phpcs:ignore with a one-line reason.

Strings and conditions

Festi.Strings.ConcatenationSpacing — no spaces around .: $first.' '.$last. A wrapped expression keeps the . at the end of the line it continues from.

Festi.ControlStructures.TrailingBooleanOperator — a condition that wraps keeps && / || at the end of the line it continues from (auto-fixable):

// bad                      // good
if ($isEnabled              if ($isEnabled &&
    && $hasPermission           $hasPermission
) {                         ) {

It applies wherever the operator appears: if, while, ternaries, assignments, return. The fix lengthens the previous line, so run the standard again after a bulk phpcbf.

Routing methods

Festi.Functions.RoutingMethodTypeHint — every parameter of a routing method on a *Plugin class has a type declaration, because the router casts URL captures to the declared type:

// bad: URL captures arrive untyped and are cast by hand
public function onDisplayAgentSkills(Response &$response, $idProject, $idAgent): bool
{
    $idProject = (int) $idProject;

// good
public function onDisplayAgentSkills(Response &$response, int $idProject, int $idAgent): bool

A routing method is a public method whose name starts with onDisplay, onUpdate, onAjax, onJson, doAjaxResponse or doJsonResponse.

Types

Every file checks its types strictly, and every parameter, return value and property says what it is.

Rule Asks for Good Bad
SlevomatCodingStandard.TypeHints.DeclareStrictTypes the file opens with declare(strict_types=1); a file without it
SlevomatCodingStandard.TypeHints.ParameterTypeHint a type on every parameter function load(string $name) function load($name)
SlevomatCodingStandard.TypeHints.ReturnTypeHint a return type on every function function load(): Project function load()
SlevomatCodingStandard.TypeHints.PropertyTypeHint a type on every property private int $_weight; private $_weight;

The type is native where PHP can express it and in the docblock where it cannot:

/** @var resource */
private $_handle;

/**
 * @param string[] $tags What an array holds is said in the docblock.
 *
 * @return array<string, int>
 */
public function count(array $tags): array

An array whose items are not described is reported (MissingTraversableTypeHintSpecification). A docblock tag that repeats a native type is not asked to go: PublicMethodDocBlock asks for it.

Overriding an untyped method. PHP refuses to add a parameter type the parent method does not have. A method that overrides one, a framework hook for example, says so and is left alone:

/**
 * {@inheritDoc}
 */
public function process(File $phpcsFile, $stackPtr): void

phpcbf adds a native type wherever the docblock already names one, and adds the declare. Review what it did to a public method: a subclass elsewhere that overrides it has to follow.

Architecture

Festi.Architecture.DomainLayerDependency — the domain depends on nothing around it. A class whose namespace has a Domain segment must not name another layer of its own context, nor reach into the application through a service locator.

namespace Plugins\Agents\Domain\Service;

// bad: the domain names the layer that stores it
use Plugins\Agents\Store\MySqlTaskStore;

// bad: the constructor no longer says what the class needs
$db = Core::getInstance()->db;

// good: the domain declares a port, the outer layer implements it
use Plugins\Agents\Domain\Port\ITaskStore;

public function __construct(ITaskStore $tasks)
Code Reported when domain code
OutwardDependency names a class of its own context outside Domain and Contracts, or any class in an Application, Infrastructure, Persistence or Adapter(s) namespace
ServiceLocator calls Core::getInstance()

Left alone: its own model, ports and exceptions, the contracts of its context, the domain and contracts of another context, framework base classes (ValuesObject, Entity), libraries, and all test code. A file with no namespace is not checked.

Complexity of one function

Three measures, all reported as errors:

Sniff Measures Reports above
Generic.Metrics.CyclomaticComplexity decision points 10
Generic.Metrics.NestingLevel depth of nested control structures 3
SlevomatCodingStandard.Complexity.Cognitive cognitive complexity, the measure SonarQube reports as php:S3776 15

A function over a limit fails a whole-tree run. For a change, the function it edits was usually over the limit before it, so both festi-phpcs-diff and festi-quality --diff measure the same function at the base revision and report only what the change added or raised (Usage). A fix made inside an old, complex function is therefore not blocked; making that function more complex is. Configuration shows how a project turns one back into a warning.

Class-design limits (PHPMD)

phpcs looks at one function at a time. PHPMD measures the class.

Rule Measures Limit
ExcessiveClassComplexity the cyclomatic complexity of every method, summed 50
CouplingBetweenObjects other types the class depends on 13
NPathComplexity execution paths through one method 200
TooManyMethods methods, accessors not counted 20
TooManyFields fields 10
ParameterCount parameters of a method or function 4
ParameterCount parameters of a constructor 5

Each limit stands for a design principle:

  • A long parameter list is a value object nobody named. Past four, some of the parameters belong together.
  • A long constructor is a class with more than one job. Its parameters are collaborators, so one more is allowed than for a method.
  • Many fields is an aggregate that has grown. An entity over ten usually holds a value object.
  • Many dependencies is dependency inversion not happening. A composition root is exempt, because knowing many classes is its job: classes named *Plugin or *Factory are not measured for coupling.

tests/ is never measured: a scenario class is long by design. Under festi-quality --diff these are compared with the base revision in the same way as the complexity rules.

Phan plugins

Enabled through the project's .phan/config.php; the template lists all of them.

PlaneSeparationPlugin

Issue Reported when Fix
FestiPlaneSeparation getPluginInstance() is called outside a composition root resolve the plugin in the facade and pass what is needed down
FestiCrossPluginDao a DAO is constructed by hand, new RunnersObject($db) use $this->object, a linked DAO, or DataAccessObject::getInstance

Resolving another plugin from inside a domain service couples the planes of an application (admin, api, workers) and hides a global dependency from the service's constructor.

Composition roots may resolve plugins: file scope (index.php, cron entry points), and classes named *Plugin, *Worker, *Tool or *Mode. Facades and DAOs may construct DAOs. new AgentValuesObject($row) is a value object, not a DAO, and is fine.

SqlInjectionConcatPlugin

PhanPluginSqlInjectionValueConcat — SQL built by concatenating a value straight after a comparison operator:

// reported
$sql = 'SELECT * FROM users WHERE '.$column.'='.$value;

// fine: the value goes through a sanitiser
$sql = 'SELECT * FROM users WHERE '.$column.'='.$this->quote($value);

quote(), getSqlCondition() and an (int) or (float) cast count as sanitisers. The rule looks only inside functions that build SQL: ones that call a query method or contain a SQL keyword.

Quotes written around a value do not make it safe, because the value can close them. These are reported too:

$sql = "SELECT * FROM users WHERE name = '".$name."'";
$sql = "SELECT * FROM users WHERE name LIKE '%".$name."%'";
$sql = "SELECT * FROM users WHERE name = '$name'";

SecurityGuardArgumentPlugin

Issue Reported when
PhanPluginSecurityGuardDisabled a call passes a literal false to a parameter named like an authorisation guard: $store->loadRowByPrimaryKey($id, false)
PhanPluginSecurityGuardDefaultsOff a method declares such a parameter defaulting to false, so every caller opts out unless it remembers to opt in

CorePlugin

Reports nothing. It tells Phan that getPluginInstance('Jimbo') returns a JimboPlugin, so calls on the result are type-checked.