Re-apply global state in ContainerFactory::postInitializeContainer() even for an already-initialized container#5918
Closed
phpstan-bot wants to merge 1 commit into
Closed
Conversation
…` even for an already-initialized container - Restrict the `spl_object_id()` early-return guard to only skip the expensive `BetterReflection::populate()` call, which wires up one-time global state in the BetterReflection facade per container. - Always re-apply the remaining mutable global state on every call: `ReflectionProviderStaticAccessor`, `PhpVersionStaticAccessor`, `ObjectType::resetCaches()`, the `typeSpecifier` service, `BleedingEdgeToggle` and `ReportUnsafeArrayStringKeyCastingToggle`. - This fixes flaky tests (GenericObjectTypeTest, IntersectionTypeTest): two test classes sharing the same cached container would short-circuit the guard, so global state mutated by one test (e.g. the bleeding-edge toggle controlling sealed array shapes, or a static ReflectionProvider/PhpVersion selecting the ReflectionClass variance stub) leaked into the next. - Add ContainerFactoryPostInitializeTest covering that returning to an already-initialized container restores the bleeding-edge toggle and the static PhpVersion / ReflectionProvider accessors.
ed24914 to
14fc334
Compare
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.
Summary
GenericObjectTypeTestandIntersectionTypeTestfailed intermittently on CI(
ReflectionClass<object>/ReflectionClass<stdClass>variance flipping toNo,and
non-empty-array&hasOffsetValue(0, int)no longer accepted byarray{int, int}).The root cause was leaking global state between tests that share the same cached
DI container, caused by an over-eager early-return guard in
ContainerFactory::postInitializeContainer().Changes
src/DependencyInjection/ContainerFactory.php: thespl_object_id()guard nowonly wraps the expensive
BetterReflection::populate()call. The remaining globalstate is re-applied on every invocation, even when returning to a container
that was already the last initialized one:
ReflectionProviderStaticAccessor::registerInstance(...)PhpVersionStaticAccessor::registerInstance(...)ObjectType::resetCaches()$container->getService('typeSpecifier')BleedingEdgeToggle::setBleedingEdge(...)ReportUnsafeArrayStringKeyCastingToggle::setLevel(...)tests/PHPStan/DependencyInjection/ContainerFactoryPostInitializeTest.php: newregression test.
Root cause
postInitializeContainer()is what binds a container's services to PHPStan'sprocess-global statics (the static reflection-provider / php-version accessors, the
ObjectTypecaches, and theBleedingEdgeToggle/ReportUnsafeArrayStringKeyCastingfeature toggles). It is invoked before every test via the PHPUnit
InitContainerBeforeTestSubscriber/ContainerInitializer.It guarded its whole body with
if (spl_object_id($container) === $lastInitializedContainerId) return;.That guard is correct for the one-time
BetterReflection::populate()work, but italso skipped re-applying the mutable global state. When two test classes share the
same cached container (e.g. two classes with the default config), the second test's
getContainer()call hit the early return, so any global state mutated in themeantime was not restored:
BleedingEdgeTogglecontrols whetherConstantArrayTypebecomes a sealed arrayshape — a leaked
truemakesarray{int, int}sealed, sonon-empty-array&hasOffsetValue(0, int)is no longer accepted (Maybe→No),which is the
IntersectionTypeTestfailure.ReflectionProvider/PhpVersiondetermine whichReflectionClassstub is loaded (
@template-covariant Tfor < 8.4 vs invariant@template Tforthe lazy-objects stub on ≥ 8.4). A leaked accessor flips the inferred variance, so
ReflectionClass<object>↔ReflectionClass<stdClass>isSuperTypeOfresultschange, which is the
GenericObjectTypeTestfailure.The fix moves only the expensive, genuinely once-per-container work behind the guard
and unconditionally re-applies the cheap global-state registration, so returning to
an already-initialized container always restores a consistent global state. All
global setters used during container post-initialization were swept together, so the
whole family is covered rather than just one toggle.
Test
ContainerFactoryPostInitializeTest::testReappliesGlobalStateForAlreadyInitializedContainermakes a container the last-initialized one, simulates another test leaking global
state (a foreign
ReflectionProvider, a bogusPhpVersion, and a flippedBleedingEdgeToggle) without changing the last-initialized container id, then callspostInitializeContainer()again and asserts the bleeding-edge toggle, the staticPhpVersion, and the staticReflectionProviderare all restored to the container'svalues. The test fails on the previous code (the leaked state survives the guard) and
passes with the fix. The full suite,
make phpstan, andmake cs-fixare green.Fixes phpstan/phpstan#14861