General Code Review / PR Standards and Guidelines

Posted by admin
Tuesday March 17^th , 2026 7:05 ^p.m.

The following is a collection standards, guidelines, and cultural norms come from common industry practices (Google, Microsoft, many open-source projects, and engineering blogs/roadmaps).

# Drupal Coding & PR Standards

These standards exist to ensure that code reviews and PR feedback are grounded in shared, documented conventions — not personal preference. Blocking a PR based on these rules is always appropriate; blocking one based on undocumented opinion is not.

Standards here are split into two tiers:

- 🚫 **Hard Rules** — Non-negotiable. A PR may be blocked for violating these.
- 💡 **Strong Preferences** — Best practices the team aspires to. Violations are worth a comment and a conversation, but are not grounds for blocking a PR on their own.

-----

# 🚫 Hard Rules

These are not stylistic preferences. They represent security requirements, core Drupal architectural conventions, and basic hygiene that the framework depends on. A PR that violates a Hard Rule should not be merged until the issue is resolved.

-----

## H1. Use Composer for dependency management and autoloading

All dependencies must be declared in `composer.json`. Composer handles autoloading in modern Drupal — manual file loading bypasses it entirely.

> **`require_once` is never acceptable** — not in modules, not in themes, not anywhere in the codebase.

-----

## H2. Never use raw superglobals

Drupal has adopted Symfony’s `HttpFoundation` component since Drupal 8. Raw superglobals — `$_GET`, `$_POST`, `$_REQUEST`, `$_SERVER`, etc. — bypass Drupal’s request handling layer, introduce security risk, and cannot be tested.

Use `\Drupal::request()` or, preferably, inject the `request_stack` service via the container.

-----

## H3. Do not directly instantiate services

Direct instantiation — `new SomeService()` inside a method or function — is not acceptable for Drupal services. Services must be accessed through the service container: via constructor injection (preferred), or `\Drupal::service()` in procedural contexts where injection is not possible.

This is a core architectural requirement of Drupal 8+ and Symfony, not a matter of taste.

|Hardcoded instantiation |Dependency injection |
|-------------------------------------------|-----------------------------------------------|
|Hides dependencies from callers |Declares dependencies explicitly |
|Ties the call site to a concrete class |Accepts any compatible interface implementation|
|Cannot be mocked for unit testing |Fully testable with mock objects |
|Violates Drupal’s architectural conventions|Adheres to Drupal/Symfony standards |

### The “wrapped static call” anti-pattern

A particularly confusing variant of this problem is wrapping a static call inside a service method:

```php
// ❌ Anti-pattern: gives the appearance of a service, but hides a hardcoded dependency
class FooService {
public function doThing(): string {
return Foo::doThing();
}
}
```

This is the worst of both worlds. The dependency on `Foo` is still hidden, still not injectable, and still not mockable. The service wrapper adds indirection without any of the benefits of the service layer.

do this instead:

interface FooInterface {
public function doThing(): string;
}

class Foo implements FooInterface {
public function doThing(): string {
return 'result';
}
}

services:
my_module.foo:
class: Drupal\my_module\Foo

And inject the interface wherever you need it:

use Drupal\my_module\FooInterface;
use Symfony\Component\DependencyInjection\ContainerInterface;

class MyClass {

public function __construct(
protected FooInterface $foo,
) {}

public static function create(ContainerInterface $container): static {
return new static(
$container->get('my_module.foo')
);
}

public function run(): string {
return $this->foo->doThing();
}
}

Use the service container properly, or don’t use a service at all.

-----

## H4. Do not commit commented-out code to production branches

Commented-out code creates noise, obscures intent, and signals unfinished work. It does not belong in a PR.

The only exception is code deliberately preserved for likely reinstatement — which should be rare, and must include an inline comment explaining why it is being kept.

Work-in-progress code belongs in a git stash or a local feature branch.

-----

# 💡 Strong Preferences

These represent the patterns and habits the team is working toward. They reflect how Drupal is designed to be used and will produce better, more maintainable code over time. Violations are worth raising in a review as a learning opportunity — but they are not blocking reasons on their own. Use judgment.

-----

## P1. Prefer OOP over procedural code

Drupal 8+ is built on an object-oriented, Symfony-based architecture. New custom code should follow that model.

**Hook functions are fine** — they are Drupal’s procedural integration layer and are expected and appropriate. The preference for OOP applies to custom business logic, services, plugins, and anything beyond the hook entry point itself.

Where possible, keep hook implementations thin: delegate to a named class or service rather than putting logic directly in the hook function.

-----

## P2. Understand the three tiers of static method usage

Not all static method usage is equal. The key question is whether a static call hides a meaningful dependency or introduces state.

|Pattern |Verdict |
|----------------------------------------------------------------------------------------------|-----------------------------------------------------|
|Hook functions (`hook_form_alter`, etc.) |✅ Fine — Drupal’s expected procedural layer |
|Static calls on true stateless utilities (no side effects, nothing to inject, nothing to mock)|✅ Acceptable with judgment |
|Static calls to stateful services, or hiding a static call inside a service wrapper |❌ Not acceptable — defeats the service layer entirely|

When in doubt: if the thing being called statically could reasonably be a service, it probably should be.

See: [When to use static methods in PHP](https://jamestarleton.com/node/144)

-----

## P3. Declare return types explicitly

PHP return type declarations make code easier to read, catch bugs earlier, and signal intent clearly:

```php
// ✅ Preferred
public function getLabel(): string {
return $this->label;
}

// Acceptable, but less informative
public function getLabel() {
return $this->label;
}
```

This is not a blocking issue, but it is a good habit — especially for service methods, plugin methods, and anything part of a public API.

-----

# PR Review Standards

-----

## What a PR review is — and is not

### What it is

A PR review exists to catch **glaring errors** and **failures to meet the Hard Rules in this document**. It is a quality gate, not a design committee.

### What it is not

A PR review is not an opportunity to:

- Reject a sound, working solution because you would have approached it differently
- Request a rewrite of valid code to match your personal style or preferences
- Impose architectural opinions on work that already meets requirements
- Introduce open-ended delays while debating equivalent approaches

> The codebase is shared. No single person owns it, and no single person’s preferred implementation pattern takes precedence over another’s valid one.

### Multiple valid solutions exist — this is expected and normal

Drupal is a rich framework with many legitimate ways to solve any given problem. As a concrete example, rendering a data table alone has [10 distinct implementation approaches and 40+ patterns](https://jamestarleton.com/node/142) — ranging from zero-code Views UI to fully decoupled React frontends — all valid depending on context. A Block Plugin, a Controller, a Views custom plugin, a Form API table, and an SDC component can all be the right answer to the same ticket.

**If code meets the requirements and does not violate a Hard Rule, it is mergeable.** “I’d have done it differently” is not a blocking reason.

### If you want to refactor someone else’s valid solution

You are welcome to refactor. But:

1. **Merge the original PR first.**
1. Open a new ticket — e.g., *“Refactor: [original ticket title or number]”* — and assign it to yourself.
1. Check out a new branch scoped to that ticket.
1. Do the refactor there, with full ownership and clear traceability.

This keeps work moving, preserves clear ownership of each change, and avoids the bottleneck of hijacking another developer’s PR with scope creep disguised as a review.

### When in doubt, send an RFC before you start

If you are uncertain whether the team will have concerns about your chosen approach, **raise it before writing the code** — not after. A short Slack message is enough:

> *“Any objections to using a custom Block Plugin for this? If not, I’ll start on it.”*

Silence or approval means you’re clear to proceed. This costs minutes upfront and prevents days of rework or review friction later.

-----

## Be a good reviewer — the human side of code review

Technical standards are only part of what makes a code review healthy. How we treat each other in the process matters just as much.

### Assume good intent

The person who wrote the code was trying to solve the problem and do their job well. Start from that assumption. A confusing implementation is more likely the result of time pressure, incomplete context, or a different mental model than it is evidence of carelessness. Review the code, not the person.

### Be kind

Directness is fine. Bluntness without consideration is not. Before submitting a comment, ask yourself whether it would land as constructive feedback or as a verdict. There is almost always a way to say the same thing without making someone feel like they’re being graded in front of the class.

We are colleagues working on a shared problem — not competitors, and not contestants on a PHP trivia show.

### Find something genuine to acknowledge

If a PR has issues worth raising, it almost certainly also has something worth acknowledging — a clean approach to a tricky edge case, a well-named function, a piece of logic that’s easier to follow than it needed to be. Say so. A review that is nothing but corrections is demoralizing even when every correction is valid. One honest positive observation changes the tone of the entire exchange.

### Keep it collaborative, not hierarchical

A review comment is the start of a conversation, not a ruling. If you’re not certain something is wrong, say so. *“I’m not sure this is the right place for this logic — what was your thinking?”* is more useful than *“This is wrong.”* It invites explanation, surfaces context you may have been missing, and occasionally reveals that you were the one with the incomplete picture.

### The goal is better software, not being right

A PR review is not a performance. Nobody wins points for finding the most issues or for defending their approach the longest. The goal is to ship code that works, that the team can maintain, and that doesn’t make the next person’s job harder. Keep that in focus.

-----

## Summary: Blocking vs. non-blocking

|🚫 Blocking (Hard Rule violation) |💡 Non-blocking (comment and discuss) |
|-------------------------------------------|--------------------------------------------|
|`require_once` or manual autoloading |Preference for a different-but-valid pattern|
|Raw superglobals (`$_GET`, `$_POST`, etc.) |Return types missing |
|Direct `new Service()` instantiation |OOP vs. procedural hook implementation |
|Static call hidden inside a service wrapper|Style or naming not covered by standards |
|Commented-out code in production branch |“I would have done it differently” |
|Security vulnerability |Personal architectural opinions |
|Broken functionality / fails requirements | |

Core Cultural Principles (Document these explicitly in team handbook or wiki)

1. “You are not your code” / “Attack the code, never the person”
Feedback must target the code / design / implementation — never imply the author is incompetent, lazy, junior, etc.
Bad: “This is obviously wrong / how did you not see this?”
Good: “This section can lead to X problem because Y — what do you think about Z alternative?”

2. Assume good intent / Assume positive intent / Assume miscommunication over malice
Start from the premise that the author was trying to do the right thing and may have had reasons or context you don’t see yet.
Phrase comments as questions when uncertain: “What’s the reasoning behind choosing this approach here?” instead of declarative statements that sound accusatory.

3. Be kind / Be respectful / Professional tone always
Many teams literally write “Be kind” or “Write comments you would want to receive” in their guidelines.
Avoid sarcasm, passive-aggression, ALL CAPS, excessive exclamation marks, or emojis that can be read as mocking.

4. Focus on improvement, not perfection
The goal is better code and better developers over time — not flawless code today.
Explicitly state in guidelines: “Nitpicks that don’t meaningfully improve maintainability, readability, performance, or correctness should be optional or omitted.”

Practical Rules to Limit Nitpicking and Bike-shedding

5. Tier comments by importance / Use labels or prefixes
Very common and effective:

prefix/label - meaning:

No Prefix: Important / security / correctness / bug

Nit / Optional / Suggestion: Minor, stylistic, personal preference

Question: Seeking understanding

Many teams enforce: anything without a prefix is assumed important; pure nits must be labeled “nit”.

6. Style / formatting issues belong to automation
Enforce: “If a linter / formatter / style-check can catch it → don’t comment on it.”
This eliminates 70–80% of the most demoralizing

7. “One nit per comment is fine — ten nits in twenty comments is noise”
Encourage batching minor suggestions or putting them in one “Nit collection” comment at the end.

8. Praise / positive feedback is required or strongly encouraged
Many teams adopt a soft rule: try to find at least one genuinely good thing to call out per review (especially for juniors or large changes).
This balances the emotional weight and prevents reviews from feeling like pure criticism.

Process Safeguards

9. Definition of “reviewable” / “mergeable”
Clearly document what must be true for a PR to be approvable (tests pass, linter clean, meaningful description, etc.).
Once those are met, purely subjective preferences should not block merge.

10. Time-box nit debates / Have an escalation path
If two people are arguing over a non-blocking preference → either drop it or take it to a quick sync / tech lead decision.
No PR should sit blocked for days over argument about brace placement.

11. Regular retrospectives on review culture
Every so often ask anonymously: “Do code reviews feel constructive or demoralizing?” and “Are we spending too much time on non-substantive comments?”