Name the compiled container after the version that compiled it - #845
Merged
Merged
Conversation
php-di writes the compiled container once and reuses whatever file it finds under that class name, without revalidating the definitions behind it, so the name is the only invalidation there is — and it carried the module but not the version. var/cache is runtime state that survives a deployment and no upgrade step clears it, so upgrading in place left the previous release's compiled container in front of the new code. Any constructor signature or definition that changed between the two then fatals with a TypeError on every request, web and API alike, and it cannot be recovered from through the UI: the container is built in Base.php, before Init runs, so the upgrade page is unreachable. Constructor signatures changed three times in recent work alone, and this turned up by accident — a live API request answering with a TypeError about an argument of a class that no longer takes it, from a container compiled before that change. The comment already above this code had the insight and stopped one timescale short: it explains that a shared name let whichever entry point compiled first decide the others' bindings, which is the same reuse, across modules rather than across releases. compiledContainerName() now builds the name from both. The previous release's file is left where it is rather than deleted, since a request arriving mid-deployment may still be using it. Demonstrated with a previous release's container left at the old fixed name: the old naming loads it and the page dies, the new naming ignores it. Both entry points were then re-checked from a cleared cache, answering 200 and 401. Three tests cover the naming rule — per version, per module, and a valid class name, since the normalised version is dotted and a dot does not compile. Base.php itself stays untested as bootstrap code; the rule was extracted so the part that carries the fix is testable.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The defect
php-di writes the compiled container once and reuses whatever file it finds under that class name,
without revalidating the definitions behind it. The name is therefore the only invalidation
there is — and it carried the module but not the version.
var/cacheis runtime state that survives a deployment, and no upgrade step clears it. So upgradingsysPass in place leaves the previous release's compiled container in front of the new code, and
any constructor signature or DI definition that changed between the two fatals with a
TypeError—on every request, web and API alike.
Nothing recovers from it through the UI either: the container is built in
Base.php, beforeInitruns, so the upgrade page that would fix things cannot be reached.
This is not hypothetical for this codebase — constructor signatures changed three times in recent
work alone (#830
Adapter, #834Init, #839AuthTokenBase). I hit it by accident: a live APIrequest returned
from a compiled container predating #830.
The comment already above this code had the right insight — it explains that a shared name let
whichever entry point compiled first decide the others' bindings — and stopped one timescale short:
the same reuse applies across releases, not just across modules.
The fix
compiledContainerName($module, $version)builds the name from both. New code compiles a new file;the old one is left in place rather than deleted, since a request arriving mid-deployment may still
be using it.
Demonstrated, not just tested
With a previous release's container left at the old fixed name:
Both entry points were then re-checked from a cleared cache — web
200, API401for anunauthenticated call — and the files written are now
CompiledContainerWeb40021031301.phpandCompiledContainerApi40021031301.php.Test
Three in
FunctionsTest: two versions do not share a name, two modules do not share a name, and theresult is a valid PHP class name — the normalised version is dotted (
400.21031301), and a dot in aclass name does not compile.
Base.phpitself stays untested, asCLAUDE.mdrecords for bootstrap code; the naming rule wasextracted precisely so the part that carries the fix is testable.
PHPStan level 6 and PHPCS clean.
Also
CLAUDE.mdgains this under the DI section, including the practical half: a stale compiledcontainer is the first thing to suspect when a local instance fatals on a constructor signature you
have just changed. It is not a bug in the change — clear
var/cache. I nearly reported one as aproduction defect on exactly that basis.