Page MenuHomePhabricator

ExtensionRegistry / wgExtensionFunctions not treated as array?
Closed, InvalidPublic

Description

This is a general inquiry.

I just had a quick look at https://phabricator.wikimedia.org/T104951 where DynamicSideba/extension.json is defined in part of:

},
"ExtensionFunctions": [
    "DynamicSidebar::setup"
],

[0] doesn't suggest to make an array_merge_recursive on $GLOBALS['wgExtensionFunctions'] with other extensions using $GLOBALS['wgExtensionFunctions'] as well and being ultimately executed in [1].

[0] https://github.com/wikimedia/mediawiki/blob/cf08f986af0a1cfa092f46f5c772c610fb52dce4/includes/registration/ExtensionRegistry.php#L165-L183

[1] https://github.com/wikimedia/mediawiki/blob/8338476b8e0c57132a5b150f42ed47da95f370b9/includes/Setup.php#L688-L705

Event Timeline

mwjames created this task.Jul 7 2015, 8:42 AM
mwjames updated the task description. (Show Details)
mwjames raised the priority of this task from to Needs Triage.
mwjames assigned this task to Legoktm.
mwjames added a subscriber: mwjames.
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptJul 7 2015, 8:42 AM

Sorry, I'm not sure what the question is..? Can you provide an example of what you think should work? Or how you expect it to?

vagrant@mediawiki-vagrant:/vagrant/mediawiki$ cat fakeextension.json 
{
	"name":"fake",
	"ExtensionFunctions": [
		"SomeCallback::foo"
	]
}

vagrant@mediawiki-vagrant:/vagrant/mediawiki$ hhvmsh
Welcome to HipHop Debugger!
Type "help" or "?" for a complete list of commands.

Connecting to ::1:8089...
Attaching to vagrant's default sandbox and pre-loading, please wait...
::1> $wgExtensionFunctions = array();
$wgExtensionFunctions = array();
::1> $r = new ExtensionRegistry;
$r = new ExtensionRegistry;
::1> $r->load('/vagrant/mediawiki/fakeextension.json');
$r->load('/vagrant/mediawiki/fakeextension.json');
::1> =$wgExtensionFunctions;
=$wgExtensionFunctions;
Array
(
    [0] => "SomeCallback::foo"
)

I just made a test installation with the mentioned T104951/Extension:DynamicSidebar and SMW and one could see that wgExtensionFunctions isn't merged recursively as stated at the beginning of this issue.

Changed ExtensionRegistry which resolves T104951 where an extension is loaded via extension.json but another extension is not (SMW).

	} elseif ( $key === 'wgHooks' || $key === 'wgExtensionCredits' || $key === 'wgExtensionFunctions' ) {
		// Special case $wgHooks and $wgExtensionCredits, which require a recursive merge.
		// Ideally it would have been taken care of in the first if block though.
		$GLOBALS[$key] = array_merge_recursive( $GLOBALS[$key], $val );
	} elseif ( $key === 'wgGroupPermissions' ) {

Thanks James,
that change made it work for me. In essence it's this diff (I changed the comment as well):

--- includes/registration/ExtensionRegistry.php.orig    2015-07-24 11:09:34.151972884 +0200
+++ includes/registration/ExtensionRegistry.php 2015-07-24 11:10:27.784141079 +0200
@@ -145,8 +145,8 @@
                foreach ( $info['globals'] as $key => $val ) {
                        if ( !isset( $GLOBALS[$key] ) || ( is_array( $GLOBALS[$key] ) && !$GLOBALS[$key] ) ) {
                                $GLOBALS[$key] = $val;
-                       } elseif ( $key === 'wgHooks' || $key === 'wgExtensionCredits' ) {
-                               // Special case $wgHooks and $wgExtensionCredits, which require a recursive merge.
+                       } elseif ( $key === 'wgHooks' || $key === 'wgExtensionCredits' || $key === 'wgExtensionFunctions' ) {
+                               // Special case $wgHooks, $wgExtensionCredits and $wgExtensionFunctions, which require a recursive merge.
                                // Ideally it would have been taken care of in the first if block though.
                                $GLOBALS[$key] = array_merge_recursive( $GLOBALS[$key], $val );
                        } elseif ( $key === 'wgGroupPermissions' ) {
Aklapper triaged this task as High priority.Jul 24 2015, 11:58 AM

Sorry, I'm still missing something here. Why does $wgExtensionFunctions need to be recursively merged? It's a flat array.

::1> var_dump($wgExtensionFunctions);
var_dump($wgExtensionFunctions);
array(6) {
  [0]=>
  string(30) "SyntaxHighlight_GeSHi::onSetup"
  [1]=>
  string(38) "GlobalCssJsHooks::onExtensionFunctions"
  [2]=>
  string(26) "EventLoggingHooks::onSetup"
  [3]=>
  string(33) "JsonSchemaHooks::registerHandlers"
  [4]=>
  string(28) "EchoHooks::initEchoExtension"
  [5]=>
  string(28) "FlowHooks::initFlowExtension"
}

::1> ExtensionRegistry::getInstance()->load('/vagrant/mediawiki/extensions/DynamicSidebar/extension.json');
ExtensionRegistry::getInstance()->load('/vagrant/mediawiki/extensions/DynamicSidebar/extension.json');
::1> var_dump($wgExtensionFunctions);
var_dump($wgExtensionFunctions);
array(7) {
  [0]=>
  string(21) "DynamicSidebar::setup"
  [1]=>
  string(30) "SyntaxHighlight_GeSHi::onSetup"
  [2]=>
  string(38) "GlobalCssJsHooks::onExtensionFunctions"
  [3]=>
  string(26) "EventLoggingHooks::onSetup"
  [4]=>
  string(33) "JsonSchemaHooks::registerHandlers"
  [5]=>
  string(28) "EchoHooks::initEchoExtension"
  [6]=>
  string(28) "FlowHooks::initFlowExtension"
}

Or, if I add wfLoadExtension( 'DynamicSidebar' ); to my LocalSettings.php:

::1> var_dump($wgExtensionFunctions);                                          
var_dump($wgExtensionFunctions);
array(7) {
  [0]=>
  string(30) "SyntaxHighlight_GeSHi::onSetup"
  [1]=>
  string(38) "GlobalCssJsHooks::onExtensionFunctions"
  [2]=>
  string(21) "DynamicSidebar::setup"
  [3]=>
  string(26) "EventLoggingHooks::onSetup"
  [4]=>
  string(33) "JsonSchemaHooks::registerHandlers"
  [5]=>
  string(28) "EchoHooks::initEchoExtension"
  [6]=>
  string(28) "FlowHooks::initFlowExtension"
}

and by adding debug logging, I verified that the DynamicSidebar's extension function was called properly.

Sorry, I'm still missing something here. Why does $wgExtensionFunctions need to be recursively merged? It's a flat array.

Well, I tried my best to explain of what is happening (with an example) when you have an extension loaded via your ExtensionRegistry and an extension that doesn't use the ExtensionRegistry but both access (or expect) $wgExtensionFunctions to be executed equally during the Setup.

If you think nothing is wrong and my analysis is false then I will excuse myself from this issue and leave users like @HermannSchwaerzler to make the appropriate changes themselves.

It's a flat array.

It is not.

Sorry, I'm still missing something here. Why does $wgExtensionFunctions need to be recursively merged? It's a flat array.

Well, I tried my best to explain of what is happening (with an example) when you have an extension loaded via your ExtensionRegistry and an extension that doesn't use the ExtensionRegistry but both access (or expect) $wgExtensionFunctions to be executed equally during the Setup.

If you think nothing is wrong and my analysis is false then I will excuse myself from this issue and leave users like @HermannSchwaerzler to make the appropriate changes themselves.

It's a flat array.

It is not.

Huh? Its a flat array of callables. What non-flat structure do you think it has?

I also find this bug confusing in terms of what the problem actually is. An example with expected behaviour vs actual behaviour would probably be helpful.

A high-level example of expected vs actual behaviour is given here https://phabricator.wikimedia.org/T104951

I am not an expert here (so this explanation might be wrong) but as far as I can tell:

  1. $info['globals']['wgExtensionFunctions'] might for sure be a flat array.
  2. $GLOBALS['wgExtensionFunctions'] is not (always) flat and thus has to be merged recursively.

Huh? Its a flat array of callables.

Well, if you call $wgExtensionFunctions[] = array( $classInstance, 'functionName' ); [0] a single dimensional array then our definition of what constitutes a flat array is different.

[0] https://www.mediawiki.org/wiki/Manual:$wgExtensionFunctions

I also find this bug confusing in terms of what the problem actually is.

@HermannSchwaerzler Sorry that I couldn't be of more help but apparently this is not bug, so please be aware of the issue in future releases. In case of SMW (and SMW is not going to use extension.jon in near future) and other non extension.jon extension that will use $GLOBALS['wgExtensionFunctions'] to a certain extend the describe issue may surface again.

mwjames closed this task as Resolved.Jul 27 2015, 12:34 PM
mwjames set Security to None.
Legoktm reopened this task as Open.Jul 27 2015, 7:21 PM

Huh? Its a flat array of callables.

Well, if you call $wgExtensionFunctions[] = array( $classInstance, 'functionName' ); [0] a single dimensional array then our definition of what constitutes a flat array is different.

Right. What bawolff said is also technically correct, since array( $classInstance, 'functionName' ) is a callable, making it a flat array composed of callables (which may be strings, closures, arrays, etc). But technicalities aside, are any of the extensions that aren't working using this format? AFAIS SMW uses a closure and DynamicSidebar has a string

@HermannSchwaerzler Sorry that I couldn't be of more help but apparently this is not bug, so please be aware of the issue in future releases. In case of SMW (and SMW is not going to use extension.jon in near future) and other non extension.jon extension that will use $GLOBALS['wgExtensionFunctions'] to a certain extend the describe issue may surface again.

Please don't close bugs as Resolved if they're not actually resolved.

@HermannSchwaerzler, can you provide the configuration you're using and any other extensions that might be installed? I tried earlier today with MediaWiki core master / SMW master / DynamicSidebar master and SMW's namespaces were registered properly. Will try later today with the versions you posted on T104951.

Relatedly, registering namespaces in a $wgExtensionFunction isn't recommended and may not work depending on what else is installed, see T47031: Some extensions register their namespaces too late for an example.

This is my configuration:
MediaWiki 1.25.1 (fd1124b)
PHP 5.3.3 (apache2handler)
MySQL 5.1.73

  • MediaWiki is installed from git but "REL1_25" not "master" (as I could not get SMW working with "master") and set up via Web-Interface.
  • SMW is installed using composer:
composer.phar require mediawiki/semantic-media-wiki "~2.2" --update-no-dev
php maintenance/update.php
  • DynamicSidebar is installed from git:
cd extensions
git clone https://git.wikimedia.org/git/mediawiki/extensions/DynamicSidebar.git
cd ..
echo "wfLoadExtension( 'DynamicSidebar' );" >> LocalSettings.php

After installing SMW the page "Special:Properties" looks like it should (listing the 20 properties that ship with SMW).
As soon as DynamicSidebar gets enabled that page looks like this:

List of properties

    NO_VALID_VALUE (0)
    NO_VALID_VALUE (0)
    NO_VALID_VALUE of type Date (0 uses)
    NO_VALID_VALUE (0)
    NO_VALID_VALUE (0)
...

What I see in ExtensionRegistry::exportExtractedData() is this:
When DynamicSidebar is disabled (and SMW fully works):

$GLOBALS['wgExtensionFunctions'] = Array( [0] => Closure Object  ( ) )
$info['globals']['wgExtensionFunctions']  *does not exit*.

When DynamicSidebar is enabled (and SMW only partly works):

$GLOBALS['wgExtensionFunctions'] = Array( [0] => Closure Object  ( ) )
$info['globals']['wgExtensionFunctions'] = Array( [0] => DynamicSidebar::setup )

Okay, this was fun to debug. Here's what's going on:

First, DynamicSidebar's extension function runs, calling $wgUser->isLoggedIn(). I had identified this as problematic in rEDSB81658b6b4c05: Add extension.json and FIXME as initializing the user object early on can easily break stuff. In this case it did.

So what happens when User::isLoggedIn() is called? If the user is logged in, it goes down a stack of: User::getIdUser::loadUser::loadFromSessionUser::loadFromUserObjectUser::loadOptionsUser::getDefaultOptions. The search preferences are a bit special, so at this point we need to invoke SearchEngine::searchableNamespacesLanguage::getNamespacesMWNamespace::getCanonicalNamespaces. MWNamespace has an internal cache of namespace names, so if anything adds namespaces after this point, it is too late. User loading continues, and we move onto the next extension function:

Next, SemanticMediaWiki's extension function runs, adding namespaces to globals, and other setup. At this point, it's too late to add any namespaces (as documented at T47031: Some extensions register their namespaces too late).

What gets interesting here is how extension registration broke this. When it goes through array_merge_recursive as @mwjames and @HermannSchwaerzler pointed out as working, DynamicSidebar's extension function is placed after SemanticMediaWiki's. So MWNamespace::getCanonicalNamespaces has not been called, so the namespaces are registered just in time. If it goes through the normal array_merge route, DynamicSidebar's extension function is placed in front of SemanticMediaWiki's, breaking the namespace registration.

This wasn't an issue before the extension registration conversion because SemanticMediaWiki would register its extension function in vendor/autoload.php, which is before LocalSettings.php, so it would always be before DynamicSidebar.

Conclusion: No bug in extension registration, just a race between DynamicSidebar doing things too early, and SemanticMediaWiki doing things too late. Closing as invalid.

Actionables:

  • DynamicSidebar should not use $wgUser in an extension function (we can track this as T104951 and I'll work on a patch)
  • SemanticMediaWiki should not register namespaces in an extension function. It should instead use the CanonicalNamespaces hook as recommended in T47031.
Legoktm closed this task as Invalid.Jul 31 2015, 4:33 AM

SemanticMediaWiki should not register namespaces in an extension function. It should instead use the CanonicalNamespaces hook as recommended in

Just for clarification, this hook is used and active since at least version 2.0 (which was released in Aug 4, 2014), so this no actionable.

[0] https://github.com/SemanticMediaWiki/SemanticMediaWiki/blob/2.0.x/includes/Setup.php#L470-L480

SemanticMediaWiki should not register namespaces in an extension function. It should instead use the CanonicalNamespaces hook as recommended in

Just for clarification, this hook is used and active since at least version 2.0 (which was released in Aug 4, 2014), so this no actionable.

Er, I quickly skimmed through https://github.com/SemanticMediaWiki/SemanticMediaWiki/blob/2.0.x/SemanticMediaWiki.php#L122 and it looks like the function you linked to still is only registered inside an extension function, meaning it's still too late.

looks like the function you linked to still is only registered inside an extension function, meaning it's still too late.

It has to be late since we compute the NS based on a possible customer offset (an offset to the regular NS and has been a feature since 0.7 ergo ~2005 [0]). By the time of extension function we are sure that a possible offset has been set in the LocalSettings.

[0] https://www.semantic-mediawiki.org/wiki/Help:$smwgNamespaceIndex

looks like the function you linked to still is only registered inside an extension function, meaning it's still too late.

It has to be late since we compute the NS based on a possible customer offset (an offset to the regular NS and has been a feature since 0.7 ergo ~2005 [0]). By the time of extension function we are sure that a possible offset has been set in the LocalSettings.

[0] https://www.semantic-mediawiki.org/wiki/Help:$smwgNamespaceIndex

Unless I'm misunderstanding how that feature works, I think it should still be possible. Something like:

// In SemanticMediaWiki.php
$wgHooks['CanonicalNamespaces'][] = function( &$namespaces ) {
	global $smwgNamespaceIndex;
	$namespaces[$smwgNamespaceIndex] = 'First_namespace';
	$namespaces[$smwgNamespaceIndex+1] = 'First_namespace_talk';
	$namespaces[$smwgNamespaceIndex+2] = 'Second_namespace';
	$namespaces[$smwgNamespaceIndex+3] = 'Second_namespace_talk;
	return true;
};

// In LocalSettings.php
$smwgNamespaceIndex = 120;
enableSemantics( 'example.org' );

Basically, you register the hook right away, but the namespaces the hook ends up registering is determined later on when the hook is invoked.

And the CanonicalNamespaces hook shouldn't run until after all user configuration is loaded and all extensions have been loaded (if it runs earlier, that's probably some other extension breaking stuff), so this should work.