Steps to replicate the issue (include links if applicable):
From @Krinkle's review of our change that added the hook runner to the loader https://gerrit.wikimedia.org/r/c/mediawiki/core/+/851178/8#message-180f0802cf4776f04f6cc1551ba8d44e0f51b9e2
Add the following three lines to your LocalSettings.php file:
$wgHooks['PreferencesGetLayout'][] = function ( &$useMobileLayout, $skin ) { $skin->isResponsive(); };
What happens?:
fatal error.
BadMethodCallException: Sessions are disabled for load entry point Backtrace: #3 /w/includes/user/User.php(438): User->loadFromSession() … #10 /w/includes/skins/Skin.php(300): UserOptionsLookup->getBoolOption() #12 /w/LocalSettings.php(29): SkinVector->isResponsive() … #16 /w/resources/Resources.php(2302): HookRunner->onPreferencesGetLayout() … #30 /w/load.php(39): wfLoadMain()
What should have happened instead?:
mediawiki running without going kablooey
Solutions (notes here focus on loader code):
Our callback in the loader currently does this:
$skinName = $context->getSkin(); $skinFactory = MediaWikiServices::getInstance()->getSkinFactory(); $skin = $skinFactory->makeSkin( $skinName ); Hooks::runner()->onPreferencesGetLayout( $useMobileLayout, $skin );
@Krinkle's suggestion:
I suggest changing the (new) PreferencesGetLayout hook to pass a string instead. It hasn't made it into a stable release yet so this should be easy to do still and ensures callers can never call into unsafe methods in the future which would be a train blocker waiting to happen as it may work in testing under certain conditions.
Mod tools initial idea:
We could pass an array containing the skin name (required) and then optionally Skin->isResponsive() and Skin->getOptions() as available. @Scardenasmolinar and I talked about this and were thinking of something like the following in the loader:
Hooks::runner()->onPreferencesGetLayout( $useMobileLayout, [ 'name' => $context->getSkin(), 'responsive' => null, 'options' => null, ] );
or
Hooks::runner()->onPreferencesGetLayout( $useMobileLayout, [ 'name' => $context->getSkin() ] );
Having looked through the existing art, I also see that plenty of hooks have optional parameters that can have null passed in, eg.
@param string|null
I think the best practice would be for us to take this flat approach and simply pass in null values when the hook gets run in the loader. eg.
Hooks::runner()->onPreferencesGetLayout( $useMobileLayout, $context->getSkin(), null, null );