[BUGFIX] Restore Context and DI container state#734
Open
sbuerk wants to merge 1 commit into
Open
Conversation
FrameworkState::push()/pop() back up and restore framework state around frontend sub-requests fired with executeFrontendSubRequest(). Two pieces of global state were not covered so far: the Context singleton and the globally registered dependency injection container. executeFrontendSubRequest() boots the frontend application through Bootstrap::init(), which registers a fresh container and fills the Context with the aspects of the frontend request, most notably the language aspect. As neither was restored afterwards, code running after a sub-request within the test scope silently continued with frontend request state, for instance Extbase repository calls resolving records in the language of the sub-request instead of the default language. push() now stashes a clone of the current Context along with the container instance, reset() applies a pristine Context and drops the container, and pop() puts both back in place. Context is a singleton whose instance reference is held in many places, so its aspects are copied over using reflection instead of exchanging the object itself. The core change identifying the leak is: https://review.typo3.org/c/Packages/TYPO3.CMS/+/94931 It works around the leak in the affected test case by resetting the language aspect by hand. That workaround is verified to be obsolete with this patch by the core change: https://review.typo3.org/c/Packages/TYPO3.CMS/+/94945 which requires this branch, runs the core test suites against it and reverts the workaround again. The class is further hardened along the way: a State array shape is declared and applied to the state stack payload, giving proper type information for the pushed and popped values, and pop() now throws a dedicated exception instead of running into undefined array access when the stack is empty. Releases: main, 10
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
1 participant
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.
FrameworkState::push()/pop()back up and restore framework state aroundfrontend sub-requests fired with
executeFrontendSubRequest(). Two pieces ofglobal state were not covered so far: the
Contextsingleton and the globallyregistered dependency injection container.
executeFrontendSubRequest()boots the frontend application throughBootstrap::init(), which registers a fresh container and fills theContextwith the aspects of the frontend request, most notably the language aspect. As
neither was restored afterwards, code running after a sub-request within the
test scope silently continued with frontend request state, for instance Extbase
repository calls resolving records in the language of the sub-request instead of
the default language.
push()now stashes a clone of the currentContextalong with the containerinstance,
reset()applies a pristineContextand drops the container, andpop()puts both back in place.Contextis a singleton whose instancereference is held in many places, so its aspects are copied over using
reflection instead of exchanging the object itself.
The class is further hardened along the way: a
Statearray shape is declaredand applied to the state stack payload, giving proper type information for the
pushed and popped values, and
pop()now throws a dedicated exception insteadof running into undefined array access when the stack is empty.
Related core changes
works around it in
BlogPostEditingControllerTestby resetting the languageaspect by hand.
runs the core test suites against it and reverts that workaround again,
verifying that the state restore here makes it obsolete.
Test results
Local quality gates of this repository (PHP 8.2, podman), all green:
Build/Scripts/runTests.sh -s cgl -nBuild/Scripts/runTests.sh -s lintBuild/Scripts/runTests.sh -s phpstanBuild/Scripts/runTests.sh -s unit— 127 tests, 337 assertionsCore side verification runs through the linked change
94945.Release line:
main(10.x). A backport to older branches can be consideredlater.