Rules instead of review

A model can write three hundred lines in a minute. Reading those three hundred lines takes me forty. I spend half of that time writing comments about things I already wrote last week, in a different pull request. Responsibility for code quality has moved almost entirely to the second developer. And that is a bottleneck you cannot fix by hiring one more.

TL;DR

Everything I write about here lives in my boilerplate: symfony-boilerplate-ddd.

What the human does, what the machine does

Code review has two layers, and only one of them needs a human.

The first one is questions you cannot answer from the repository alone. Does this domain model match what we discussed with the business? Is this trade-off worth its price? Will the name of this method still make sense in a year? That is work for a human, and it will stay that way for a long time.

The second one is checking, for the twentieth time, whether somebody injected EntityManager into the domain again. That is work for a machine, and it always was. As long as people wrote all the code, good will was enough, plus the memory of the teammate who happened to look at the pull request. That stopped working when the amount of generated code grew faster than the number of people who have to read it.

There is a second reason too, and it is less obvious. A model does not know about decisions that are not in its context. It was not in the meeting where you agreed that Doctrine mapping belongs in the infrastructure layer. It knows what you put in the prompt, and what a tool tells it when it runs the tests. If your team contract lives only in people's heads, the model will never learn it. If it lives as a rule, the model gets it back on every run of the analyser, and it fixes itself before the pull request even reaches me.

Deptrac guards the layers

I will start with the simplest thing, because it gives the most for the lowest cost. Deptrac assigns classes to layers and checks that dependencies between them point in an allowed direction.

It was built in a time when the layer diagram lived on Confluence, and following it was a matter of discipline. Somebody drew boxes with arrows, the team agreed to respect them, and six months later the domain was importing EntityManager, because that was faster on a Friday afternoon. The idea was simple: move that diagram from Confluence into the repository, so you can check it instead of believing in it.

That idea has not changed. What changed is the reader. Deptrac used to be a control tool. You ran it rarely: during a bigger refactor, or before a release, to see how far you had drifted from the design. Today it is mostly an information tool for someone who has never seen that diagram. A model has no access to Confluence. It has access to the output of composer test:architecture, which tells it plainly that App\Order\Domain\Order has no right to depend on Doctrine\ORM\EntityManager.

That is the whole trick. One command turns an architectural decision into feedback you can get in a loop, on every change instead of once a quarter.

layers:
  - name: Domain
    collectors:
      - type: classLike
        value: '^App\\[A-Za-z0-9]+\\Domain\\'

  - name: Application
    collectors:
      - type: classLike
        value: '^App\\[A-Za-z0-9]+\\Application\\'

ruleset:
  Domain: ~

  Application:
    - Domain

  Infrastructure:
    - Domain
    - Application
    - Vendor

Domain: ~ is an empty list of allowed dependencies, which means the domain cannot depend on anything but itself. The application layer sees the domain. The infrastructure layer sees everything, including vendor code.

The Vendor layer is defined in a more interesting way. It is not a list of packages. It is defined the other way round: everything that is not App\, with a short allow list of built in PHP types (DateTimeImmutable, Throwable, Countable and similar). Because of that, every new package from Packagist is forbidden in the domain by default, and you do not have to add anything to the config.

The responsibilities of every layer, written down in fifty lines of YAML and checked with one command. Nothing is cheaper than that.

Custom PHPStan rules

Everything has already been written about PHPStan levels and about turning on level: max. People talk much less about the rules: section, which lets you add your own rules to the analyser. Rules that have nothing to do with types, and everything to do with how your team agreed to work.

A rule is a single class with two methods. getNodeType() narrows the syntax tree down to the node you care about, and processNode() decides whether it is an error:

final class NoAmbientClockRule implements Rule
{
    private const DOMAIN_NAMESPACE_MARKER = "\\Domain\\";

    public function getNodeType(): string
    {
        return New_::class;
    }

    public function processNode(Node $node, Scope $scope): array
    {
        if ($this->isDomainNamespace((string)$scope->getNamespace())) {
            return [];
        }

        if (!$node->class instanceof Name || $node->class->toLowerString() !== "datetimeimmutable") {
            return [];
        }

        $argument = $node->getArgs()[0]->value ?? null;

        if ($argument === null || ($argument instanceof String_ && strtolower($argument->value) === "now")) {
            return [
                RuleErrorBuilder::message(
                    "Do not read the ambient clock — inject Psr\\Clock\\ClockInterface (symfony/clock provides it) and take the current time from its now(). (Domain classes are exempt, they may stamp themselves.)",
                )->identifier("symfonyBoilerplate.noAmbientClock")->build(),
            ];
        }

        return [];
    }
}

Thirty lines, and they guard a habit you normally catch much later, when you have to write a test for something that depends on "now". Look at identifier. I will come back to it in a moment, because it is the most underrated part of this API.

Registering the rule is one line in phpstan.neon:

rules:
    - SymfonyBoilerplate\PhpStan\NoAmbientClockRule

14 rules that hold the contract

I have fourteen of them in the boilerplate. Not because I like rules, but because each one replaced a comment I had written more than once.

You can see three kinds of contract here. A few rules guard layers, which means the things Deptrac will not catch, because they happen inside a single namespace. A few guard interfaces. If a class implements RequestInterface, the argument resolver will deserialize it, so it has to be final and immutable, and the rule writes down where and how you are allowed to implement that interface. The rest guard plain habits, the kind that sound like nitpicking in a pull request comment.

Write error messages for the model

We handed most of the typing over to models, so it is worth looking at error messages from your tools as input, not output, because they are what goes back into the loop. This is where the potential of custom rules gets wasted most often. Compare two messages from the same repository.

This one is bad. It is mine, and it needs fixing:

Use DateTimeImmutable, not mutable DateTime.

And this one is good:

Domain class names the framework type "%s" — the domain layer stays framework-free; depend on an interface you own and adapt the framework type in the Infrastructure layer.

The difference is not the length. The first one says what is not allowed. The second one says what to do instead, and it names the layer where you should do it. For a human that is the difference between annoyance and a hint. For a model it is the difference between getting stuck and fixing the code in the next iteration, because the output of the analyser is exactly what it gets back in its context.

A rule message is not a log line. It is the one piece of documentation that will definitely be read. Write it like a paragraph from your onboarding docs.

The second thing is identifier. It is the address of a rule, and you can use it to silence single cases instead of the whole rule:

ignoreErrors:
    -
        identifier: symfonyBoilerplate.noAmbientClock
        path: tests/*
        reportUnmatched: false

Reading the clock in tests is fine. The exception here is explicit, described and limited to one path, instead of being hidden in @phpstan-ignore-next-line scattered around the code.

Downsides?

To be fair:

• A rule is code you have to maintain. You test it, you fix it, you update it together with PHPStan.
• False positives hurt. A rule that blocks correct code will land in ignoreErrors within a week and stop protecting anything.
• You cannot write down meaning as a rule. Not one of these fourteen will tell you that a domain model is wrong. They free up time for that question, but they do not answer it.
• Adding this to an existing project hurts. You have to start with a baseline and pay the debt down slowly. Otherwise the first run prints a few hundred errors and everybody turns it off.

Summary

There is one rule of thumb: if you wrote the same pull request comment for the third time, it is not a note about the code any more. It is a missing rule.

This used to work on the memory of a teammate, and as long as code was written at human speed, that was enough. Today it is not enough, and not because models write worse code. It is because they write faster than we can read. A contract you write down as a rule enforces itself on every run, and it goes back to the model as a hint before anyone opens a pull request.

What is left for the human is the part of code review where a human is actually needed.

Have questions? Reach out on LinkedIn.