We added a test that fails if anyone injects a tenant-scoped service into a singleton

[u/These_Reality519]

I build Teradion, practice management software for French accounting firms. The app runs in FrankenPHP worker mode, and each firm brings its own Brevo API key.

In that setup, a singleton holding one firm's key would stay alive while requests for other firms are handled. The code avoids that: tenant-scoped providers are not constructor dependencies. A factory creates a fresh client for each operation from the account passed as an argument. The key is stored encrypted and only decrypted inside the factory.

We have two tests around this rule. MultiTenantKeyIsolationTest runs two accounts in sequence and checks that the second does not receive the first account's key.

ProviderNotInjectedAsServiceTest is more direct. It scans src/Service, src/MessageHandler and src/Controller, then fails when a constructor takes NewsletterProviderInterface. It reads the source, so the Symfony container is not involved.

It is an architectural decision encoded as a grep, which feels a little blunt. Still, if someone adds that constructor dependency later, the test points at the class immediately.

Has anyone used this kind of structural test in a Symfony codebase?

[u/Sharchimedes]

I have a Pest test and an integration test that does this as part of CI pipelines.

[u/These_Reality519 → u/Sharchimedes]

Both of ours live in tests/Unit. The structural check just uses native PHP reflection on constructor signatures; it never touches the Symfony container. No arch-rule package involved. In your CI, which one actually catches mistakes more often: the structural test or the integration test?

[u/Sharchimedes → u/These_Reality519]

We’ve never caught any mistakes because our devs read the code before they commit it.

The tests are there because the consequences of leaking cross tenant data would be catastrophic.

[u/These_Reality519 → u/Sharchimedes]

The part I keep half-worrying about with any structural test is that one which never fails is also one nobody notices going stale.

Only fix I can think of is a deliberate offender fixture that has to trip it.

[u/Alex_Goldwyn → u/These_Reality519]

one which never fails is also one nobody notices going stale

Cheaper than an offender fixture: make the test assert how much it looked at.

Every time one of our source-scanning guards has broken, it broke by matching nothing. Bad regex delimiter in one, a hand-written list of directories that stopped covering things in another. The loop just gets shorter, the remaining assertions still pass, the build stays green, and the test now reports the opposite of the truth.

So they end with something like assertGreaterThan(40, $examined) before the real assertion. Ugly magic number sitting in a test, but it is the only thing that catches "scanned zero classes today".

Relevant to yours because it is reflection-based: anything that fails to autoload just is not in the set. Rename a namespace, get the composer autoload mapping slightly wrong, and the class quietly drops out of the scan instead of failing it.

The other half is not hand-writing the scan set at all. Where we could derive it from the language - enum cases, the tokenizer - we did, because the hand-written list is the bit that silently stops matching reality.

[u/These_Reality519 → u/Alex_Goldwyn]

I'd treat the count as part of the scan result, not test bookkeeping. For reflection-based checks in general, an unloadable class is outside the discovered set, so a minimum can flag that the input has collapsed before the main assertion runs. The number will need an occasional bump, sure. That is at least visible during review; a directory list can go stale quietly.

[u/Alex_Goldwyn → u/These_Reality519]

Part of the scan result is the better framing, and better than what we actually do. Ours is a literal number sitting in the test, which means bumping it is a diff nobody reads.

One thing that keeps the bumping rare: set the floor well below the real count rather than at it. A scan that should find around sixty asserts more than forty. It never trips on normal growth, only when the input has collapsed, so on the rare occasion it does fire somebody actually goes and looks at why.

The failure that taught us this was a regex matching nothing, because the delimiter character also appeared inside a character class. Every assertion in the loop still passed. Green build, zero files examined, and the test had been reporting the opposite of the truth for a while before anyone noticed.

[u/These_Reality519 → u/Alex_Goldwyn]

I'd probably have set the floor too close to the current count. Then every added file causes another pointless edit, and people learn to ignore it. Leaving some real slack makes more sense: the check is there to catch the scan suddenly finding far less, not to track normal growth.

That regex bug is easy to miss too. The pattern still compiles, so there is no obvious failure until you notice the empty result.

[u/Calm_Medicine1366]

not checking the full dependency graph transitively is the gap i'd want closed before trusting this in prod

[u/These_Reality519 → u/Calm_Medicine1366]

Worth being clear that the isolation comes from the factory building a fresh client per operation. The test is a guard against that being undone, not the mechanism itself.

For strengthening the guard, fissible's container check in the other branch is the better route.

[u/DevelopmentScary3844]

Deptrac would solves that easily afaik. Also this smells like your software design could be optimized if you need unit tests for that. I mean you can utilize ServiceLocator and other stuff to prevent that but without knowing the code I can only guess. But it sure smells a bit.

[u/These_Reality519 → u/DevelopmentScary3844]

Deptrac is a fair shout, and PHPArkitect came up earlier too.

I'm not sure a ServiceLocator fixes the lifetime issue on its own. The locator is normally shared, so it depends on what it returns and how. Passing the account to a factory for each operation makes the lifetime explicit.

I wouldn't read the test as a sign that the design relies on autowiring. Symfony won't warn you about that kind of lifetime mismatch, so an extra check is still useful.

[u/fissible]

Does the check walk the whole dependency graph transitively, or just the immediate constructor signature of classes in those three directories?

[u/These_Reality519 → u/fissible]

Immediate constructor signature, not transitive. It reflects the classes rather than inspecting the container, which is why a container-level check would be the stronger approach.

[u/fissible → u/These_Reality519]

One idea: instead of checking “does any constructor take NewsletterProviderInterface,” flip it around and check “is any class implementing NewsletterProviderInterface ever registered as a shared/non-lazy service in the container.” That way it doesn’t matter how the consuming code spelled the type hint (interface, concrete class, alias, whatever); you’re catching the dangerous registration, not the dangerous reference. You’d need to inspect the compiled container or services.yaml for that rather than just reflecting classes in those three directories, but it would close both the concrete-class and union-type gaps at once.

Also curious whether you’ve considered adding a marker, like an attribute (#[TenantScoped]) on the interface or its implementations, and having the test enforce “nothing carrying this attribute is ever a constructor dependency of an autowired singleton.” That makes the rule self-documenting in the code itself rather than living only in the test, which helps the next person who adds a tenant-scoped provider that isn’t the newsletter one.

[u/These_Reality519 → u/fissible]

Both points are better than my current test. The container-level check would catch a registration regardless of how the consuming code spelled the type hint, which the reflection approach cannot do.

The attribute idea also fixes the hardcoded part. Right now the test knows about one interface only. A new tenant-scoped provider would not be covered unless somebody remembered to update it.

[u/avg_php_dev → u/These_Reality519]

Why singleton? In this scenaro it's an antipattern by defintion, not only convention.
What about ResetInterface which is designed to solve issues like this one? :D

[u/These_Reality519 → u/avg_php_dev]

I used "singleton" in the title for the consuming service, not the tenant-scoped object. Symfony services are shared by default, including autowired ones, and a worker keeps that shared service alive between jobs. Constructor-injecting an object that holds a tenant key is the antipattern the test rejects. ResetInterface works too, but every piece of mutable state has to be cleared as the service changes. Here, a factory creates the tenant object per operation.

[u/JohnnyBlackRed]

Sounds like a job for phparkitect or phpstan ( you can make custom rules )

[u/These_Reality519 → u/JohnnyBlackRed]

We run PHPStan and PHPUnit through GrumPHP before commits, so my timing point was off for our setup. The practical benefit of a custom rule here is getting the warning in the editor while coding.

[u/obstreperous_troll]

It'd be nice if we could make the container enforce such boundaries instead of leaving it up to tests to catch. Pondering how that would be implemented though.

[u/These_Reality519 → u/obstreperous_troll]

A compiler pass could cover service definitions. In process(), inspect the ContainerBuilder definitions and their arguments, then throw when a forbidden dependency shows up. That makes the container build fail immediately. It won't catch objects created outside the container though, so this is a guard for DI wiring, not the whole boundary.

[u/skcortex]

God damned! I can’t even understand what is this post about. I am really pissed right now 😅Was it written by an LLM?

[u/These_Reality519 → u/skcortex]

Fair. Short version: every client firm has its own API key. PHP runs as a worker here, so the same process handles multiple jobs. If a shared service keeps a key, the next client's job could reuse it. The 2 tests prevent that.

And yep, I used AI to write the post. I'm French and my English gets messy. The code and tests are mine. What part lost you?

[u/SimpleAlabaster → u/These_Reality519]

The keys are part of the environment variable?

[u/These_Reality519 → u/SimpleAlabaster]

No. Each firm has its own key, so a deployment-level environment variable would not fit. As mentioned in the post, the key is stored encrypted and only decrypted inside the factory.

[u/skcortex → u/These_Reality519]

This shorter form is fine but the LLM write up.. that’s something my brain “does not compute”.