SOLID Review

A design finding is worth reporting only when the design already costs something or will on the next ordinary change — a bug, an inconsistency, a change that has to touch many places, a unit that cannot be tested. Style, naming taste, "could be more elegant" and textbook purity are never findings.

Scope

Review the changed code and what it calls, read whole — not only the diff hunks. Judge the change against the conventions the repository already follows (its AGENTS.md/CLAUDE.md, existing seams and patterns). A repo that keeps small scripts procedural is not violating SOLID by doing so.

Rules

SRP — Single Responsibility

One unit (function, class, module, service) has to change for two unrelated reasons. Name both reasons in the finding.

• Finding: an HTTP controller that validates input, builds SQL, renders e-mail and spawns a child process; a Drupal form submit handler that also syncs to a remote API.
• Not a finding: a long function that does one thing in several steps.

OCP — Open/Closed

Adding the next variant (a check, a backend, a provider, a content type) requires editing a switch/if chain in several places instead of one registration point (a map, a plugin type, a tagged service, an event subscriber).

• Cite every place the next variant would have to touch.
• Not a finding: a single match in one place that is the registration point.

LSP — Liskov Substitution

An implementation that throws, no-ops or silently changes meaning for part of the contract it claims, so that callers must know which concrete class they have (instanceof checks, "not supported" exceptions, a subclass that ignores an argument).

ISP — Interface Segregation

Callers are forced to depend on methods they never use, or implementers must stub out half an interface. Cite the unused methods and the stubbing implementers.

DIP — Dependency Inversion

Business logic hard-wired to a concrete I/O dependency (network, filesystem, database, clock, child process, a static \Drupal::service() call inside a class that supports injection) so that it cannot be tested or swapped without it — where the codebase already has a seam for that dependency elsewhere (DI container, an injected client, a clock service). Without an existing seam, report it only if it already blocks a test the change needs.

Severity

• high — the design problem has already produced a bug or an inconsistency you can point to (two branches of an if chain disagree, a stubbed method is called in production).
• medium — default: the next ordinary change will have to touch several places or cannot be tested without real I/O.
• low — cleanup that would help but costs nothing today.

Reporting a finding

Each finding carries:

• rule — SRP, OCP, LSP, ISP or DIP;
• location — file:line (every location for OCP);
• cost — one sentence: what breaks or what the next change has to do because of it;
• fix direction — the smallest change that removes the cost, preferring a seam the codebase already has over a new abstraction.

Verify each finding against the code before keeping it: re-read the lines and confirm the cost is real. Fewer, true findings beat a long list. A disagreement with the overall approach of a change is one finding, not a rewrite.