Does getMFConfig() allow changing global variables too?

In T146312#2660250, @bmansurov wrote:

Does getMFConfig() allow changing global variables too?

[We've configured the ConfigFactory to return instances of the GlobalVarConfig.](https://github.com/wikimedia/mediawiki-extensions-MobileFrontend/blob/master/extension.json#L24-L26), an implementation of [Config](https://github.com/wikimedia/mediawiki/blob/master/includes/config/Config.php), which only defines get and has.

We could change getMFConfig to return an instance of HashConfig, an implementation of [MutableConfig](https://github.com/wikimedia/mediawiki/blob/master/includes/config/MutableConfig.php). However, I'd recommend against it.

As an aside, GlobalVarConfig can be passed a prefix, which'll be prepended to all keys that we pass, e.g.

$config = new GlobalVarConfig( 'wgMF' );
$config->get( 'CollapseSectionsByDefault' ); // returns $_GLOBALS[ 'wgMFCollapseSectionsByDefault' ]

I've also expected getMFConfig to be MobileFrontend-only.

With this in mind, I'd suggest that we also audit the use of getMFConfig to access non-MobileFrontend global configuration.

In T146312#2660250, @bmansurov wrote:

Does getMFConfig() allow changing global variables too?

I think this is a special case and an exception to the rule.

I did a quite audit of the includes folder and this is what I've got:

includes/api/ApiMobileView.php
534:		global $wgMemc;

includes/devices/CustomHeaderDeviceDetector.php
23:	 * @param Config $config The global config. Currently this can be any instance
24:	 *  of `GlobalVarConfig`.
27:	 * instance. `GlobalVarConfig#__construct` accepts a custom prefix to avoid
31:	 * $config = new GlobalVarConfig();
32:	 * $mobileFrontendConfig = new GlobalVarConfig( 'wgMF' );

includes/devices/DeviceDetector.php
41:	 * `$_SERVER` superglobal within its API, it's expected to be passed as
45:	 * @param array $server Per the above, the `$_SERVER` superglobal

includes/diff/InlineDifferenceEngine.php
117:		global $wgContLang;

includes/MobileFrontend.body.php
194:		global $wgContLang;

includes/MobileFrontend.hooks.php
23:	 * Enables the global booleans $wgHTMLFormAllowTableFormat and $wgUseMediaWikiUIEverywhere
28:		global $wgHTMLFormAllowTableFormat, $wgUseMediaWikiUIEverywhere;
70:		// FIXME: This shouldn't be a global, it should be possible for other extensions
72:		global $wgULSPosition;
287:		// FIXME: Global core variable don't use it.
288:		global $wgResourceModules;
441:			$vars += CodeMirrorHooks::getGlobalVariables( MobileContext::singleton() );
1258:	 * Handler for MakeGlobalVariablesScript hook.
1261:	 * @see http://www.mediawiki.org/wiki/Manual:Hooks/MakeGlobalVariablesScript
1266:	public static function onMakeGlobalVariablesScript( array &$vars, OutputPage $out ) {
1343:		global $wgResourceLoaderLESSImportPaths, $wgDisableAuthManager;

includes/specials/SpecialUploads.php
107:		global $wgConf;

I can't remember what the state was when I created this task but it looks pretty clear that the only place we use globals is when overriding or adding to them which is probably not appropriate behaviour.

I'd like to codify this... @pmiazga as our resident phpcs wizard do you know if it's possible to disallow the use of the global keyword?
Where we are currently using it we'd want to override the sniff
e.g.

// @codingStandardsIgnoreEnd there is no way to access this directly without the global
global wgMemc;

(btw, $wgMemc should be considered deprecated. It may be accessed through ObjectCache::getLocalClusterInstance().

^ Seems to have been accidentially removed, at least it was missing but the sentence didn't make sense without.

When it comes to removing global state I'm yes, yes and once again yes!.

As someone said - there is no comment to disable GlobalKeywordSniff, anyway this sniff is bad as it says -> use $_GLOBALS which at the end is the same poison.

Possible solutions

We can do two things,
a) Mark every global with comments which at the end is still bad, we just create more code

We can use

// comment why it is still here
// @codingStandardsIgnoreStart
global $myLovelyGlobal
// @codingStandardsIgnoreLine

or even better:

// comment why it is still here
// @codingStandardsIgnoreLine
global $myLovelyGlobal

b) enable this Sniff with low severity, so it's hidden during an automated build, jenkins will still be happy and when developers run the phpcs it will complain. I think it's better approach as:

• we all know globals are bad, once you see a global keyword - try to remove it
• we cannot get rid of globals because of MWCore, documenting each global is difficult

c) create some kind of globals wrapper, for example: "MediaWikiCoreBridge" with a set of getters and setters to modify MWCore global state.

Last words

I prefer solution b) or c) [or even better both]. We cannot get rid of globals, as for example $wgContLang is not available via dependency injection yet and global is the only way to get it . We could use time more efficiently than enabling a sniff and disabling it via comments.

With regards to c) I don't think this would help us. Where we are using globals we tend to be trying to get round a lack of dependency injection (and we should create that wrapper in core not in MobileFrontend) or we are hijacking them (being naughty and changing their state). b) thus seems like a good approach.

@pmiazga does the description look good?
If so please unassign yourself and move to "triaged but future"

Once we go with low severity we should not suppress phpcs checks for global keyword usage. Instead, we have to setup CI in the way it doesn't complain about low severity warnings. Only command line phpcs execution will notify about all errors &warnings and it will up to the developer to fix it or leave it for now. There is no real value with setting low severity and ignoring those errors at the same time.

I'll remove

and disable the phpcs sniff using // @codingStandardsIgnoreLine

and move it to "triaged but future".

If those warnings will bother us a lot we can re-open this task and add // @codingStandardsIgnoreLine as it's pretty trivial task.

We estimated this and got a 3, two 2s and a ?
It sounds like this might be a spike.

moving to triaged but future as per backlog grooming

@Jdlrobson
I can help mentor this task. The codesniffer link is broken, and the github links seem messed up.

In T146312#3692171, @MtDu wrote:

The codesniffer link is broken, and the github links seem messed up.

I think I fixed both.

Imported as https://codein.withgoogle.com/tasks/5688942768685056/

I wish I had seen this earlier - I strongly think this is a bad idea. There are still plenty of legitimate reasons to use globals in MediaWiki code - requiring explicit coding standards suppression for each one is a bad idea. It's going to confuse newcomers who suddenly have PHPCS failures even though they did everything fine.

Also, doing this as a MobileFrontend-only sniff is going to be really annoying from a maintenance perspective. We work really hard to centralize the CodeSniffer configuration, and diverging from it makes it harder to maintain in the long-run.

Instead of doing this, please expand the list at https://phabricator.wikimedia.org/diffusion/MCSN/browse/master/MediaWiki/Sniffs/VariableAnalysis/ForbiddenGlobalVariablesSniff.php;f494036e8a7a3892f458442289da289c4966c3ab$34 of globals that have non-global replacements, e.g. $wgMemc that was mentioned earlier.

@Legoktm you're correct. Lots of MediaWiki codebase depends on globals, because of that it's a common practice to use it. We should not only maintain the highest standards but promote the best practices. Currently, we have MediaWikiServices - which uses Service Locator patter, we should encourage people to use it instead of sticking to old things are currently considered as a bad practice. If something is missing (ex: $wgMemc) we should implement those missing bits in the MediaWikiServices.

Thanks for the insight @Legoktm! When we set this task up our concern was that we were relying heavily on code review to stop developers using the global keyword even when a config was available and the value could be sought from it. In fact, the only place the global keyword should be used in MobileFrontend is if the original value of the global is being modified (yes hacky, but this happens).

I didn't know you could list certain globals as forbidden so that does change things although I wonder if a whitelist would be more appropriate? Surely using globals is still relatively rare.. especially in an extension?

Should this be declined? Still a bit lost about this one..