fix(testing): register extenders before enabling test extensions (#4600) - #5039
Open
wakqasahmed wants to merge 1 commit into
Open
fix(testing): register extenders before enabling test extensions (#4600)#5039wakqasahmed wants to merge 1 commit into
wakqasahmed wants to merge 1 commit into
Conversation
OverrideExtensionManagerForTests called enable() before extend(), so any resolving() callback an extender registers on ExtensionManager during extend() (e.g. Extend\Locales, which hooks LocaleManager) was registered too late -- enable() had already resolved the singleton via onEnable(), and a resolving() callback never fires retroactively. In practice this meant extension-shipped locale files never loaded into the translator during integration tests. Registering extenders first fixes that, but naively also moving enable() after 'booted = true' breaks isEnabled()'s early return in ExtensionManagerIncludeCurrent, which gates on booted specifically so enable() runs its full body (migrations, assets, Enabling/Enabled events) instead of being treated as already enabled. So this toggles booted off again around the enable() loop, then back on once it's done -- extenders are registered first, but enable()'s own internal ordering (dependency check, migrate, publish assets, persist enabled state, onEnable(), dispatch Enabled) is unchanged. Added a regression test: a fake extension shipping a locale file via Extend\Locales, asserting its translation key resolves to the real value instead of being echoed back.
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.
Fixes #4600.
OverrideExtensionManagerForTestswas callingenable()on each test extension before callingextend(). Any extender that registers aresolving()callback duringextend()—Extend\Localesdoes this forLocaleManager— never gets to fire, becauseenable()had already resolved that singleton via itsonEnable()hook. A singleton only resolves once, so the callback registered afterward is too late. In practice this meant an extension's own locale files never loaded into the translator during integration tests;$translator->trans('my-ext.some.key')just echoed the key back instead of resolving it.The obvious fix is to just swap the two calls, but that alone breaks something else:
ExtensionManagerIncludeCurrent::isEnabled()returnsfalseunconditionally whilebootedisfalse, specifically so thatenable()'s early-return guard (if ($this->isEnabled($name)) return;) doesn't skip its own body — migrations, asset publishing, theEnabling/Enabledevents,onEnable(). Flipbootedtotruebefore theenable()loop (which you'd need to do to callextend()first, sinceextend()only pulls extenders for already-enabled extensions) andenable()starts short-circuiting immediately, silently skipping all of that for every test extension.So this toggles
bootedoff again specifically around theenable()loop:booted = true→extend()(registers extenders, includingLocales'resolving()callback) →booted = false→ theenable()loop (now behaves exactly as it did before this change) →booted = trueagain.Added a regression test: a fake extension in the testing package's own fixtures that ships a locale file via
Extend\Locales, asserting its translation key resolves to the real string rather than being echoed back.Verified with a disposable
php:8.3-clicontainer (no persistent PHP/Composer in my normal setup): the new test fails against the old code exactly as described (flarum-testing-tests.testreturned literally), and passes after the fix — ran the fullTestCaseTestsuite (15 tests, 24 assertions), including the existing migration and enabled-state tests, to make sure thebootedtoggling doesn't regress anything else in that file. Four unrelated tests in the same container failed on missing theGDPHP extension, unrelated to this change. Didn't touch 1.x — this worktree only has the 2.x version of the file, and the fix needs verifying separately there if you want it backported.