Page MenuHomePhabricator
Create Task
Maniphest T206949

ExtensionProcessor::storeToArray() should respect merge strategies instead of using array_merge_recursive unconditionally
Open, LowestPublic
Actions

Description

I don't know if this is something to write a task about, as it's just a question, but it seems like the most practical approach.

Where do we merge non-config globals? We merge the extension modifications to core globals in exportExtractedData, but where do we merge the data of different extensions? ExtensionProcessor::$mergeStrategies isn't used to merge into $this->globals, but only later on. Instead, we seemingly use ExtensionProcessor->storeToArray, which ignores merge strategies and uses array_merge_recursive all the time.

This is probably just a phantom I'm changing and something I overlook, as I think it works in production. However, if I'm correct here, this might be a problem for the settings in the intersection of ExtensionProcessor::§mergeStrategies and ExtensionProcessor::$globalSettings:

'wgAuthManagerAutoConfig' => 'array_plus_2d',
'wgGrantPermissions' => 'array_plus_2d',
'wgGroupPermissions' => 'array_plus_2d',
'wgPasswordPolicy' => 'array_merge_recursive',
'wgRateLimits' => 'array_plus_2d',
'wgRevokePermissions' => 'array_plus_2d',

Thank you and sorry for the inconvenience.

Event Timeline

MGChecker created this task.Oct 14 2018, 5:03 PM
Reedy updated the task description. (Show Details)Oct 14 2018, 9:01 PM
Legoktm added a comment.Oct 15 2018, 10:03 PM

Extensions shouldn't be modifying other extensions' config settings. If extensions need to configure something in another extension, they should use attributes instead, which use array_merge_recursive. In practice this works out fine since most attributes are keyed by the registering extension name.

Does that answer your question?

MGChecker added a comment.Oct 16 2018, 12:19 AM
In T206949#4668683, @Legoktm wrote:

Extensions shouldn't be modifying other extensions' config settings. If extensions need to configure something in another extension, they should use attributes instead, which use array_merge_recursive. In practice this works out fine since most attributes are keyed by the registering extension name.

Does that answer your question?

It doesn't really answer my question, as this problem wouldn't appear with extension defined config settings, but with core config settings like $wgGroupPermissions that can be modified via extension registration. Extension registration (seemingly?) uses the wrong merging algorithm here, namely array_merge_recursive all the time instead of the specified one in ExtensionProcessor::$mergeStrategies.

Legoktm triaged this task as Lowest priority.Dec 12 2019, 8:58 AM

So I think there's technically a problem, but it's not a problem in practice (though its a nasty trap we should fix).

If Extension A has GroupPermissions['sysop']['foo'] = true; and Extension B has GroupPermissions['sysop']['foo'] = false; (this will never happen in practice because the two extensions should never define the same userright), then storeToArray/array_merge_recursive will do the wrong thing.

Legoktm renamed this task from Where do we merge non-config globals between extensions? to ExtensionProcessor::storeToArray() should respect merge strategies instead of using array_merge_recursive unconditionally.Dec 12 2019, 8:58 AM
Content licensed under Creative Commons Attribution-ShareAlike (CC BY-SA) 4.0 unless otherwise noted; code licensed under GNU General Public License (GPL) 2.0 or later and other open source licenses. By using this site, you agree to the Terms of Use, Privacy Policy, and Code of Conduct. · Wikimedia Foundation · Privacy Policy · Code of Conduct · Terms of Use · Disclaimer · CC-BY-SA · GPL