Page MenuHomePhabricator

Don’t use array_merge_recursive() to merge Wikibase definitions arrays
Closed, ResolvedPublic5 Estimated Story Points

Description

Several Wikibase extensions use array_merge_recursive() to combine multiple “definitions” arrays (mappings from entity or data type names to various definitions – formatter factory callback, RDF builder factory callback, etc.). However, this is dangerous – when more than one input array has a non-array value for the same (nested) key, array_merge_recursive() will merge the values into an array:

>>> array_merge_recursive( [ 'a' => 1 ], [ 'a' => 2 ] )
=> [ "a" => [ 1, 2 ] ]

But an array is not a valid value for most definitions, and depending the key where this happens, it can bring down the whole site (as it did in T287100). We don’t really want a fully recursive merge of these arrays – just one level deeper than a standard array_merge() (the arrays for each type should be added non-recursively).

Acceptance Criteria: 🏕️🌟

  • All "definitions" are fixed EntityTypeDefinitions & DataTypeDefinitions
  • All extensions are fixed that use these (Probably Wikibase, WikibaseLexeme, WikibaseMediaInfo, WikibaseCirrusSearch, WikibaseLexemeCirrusSearch but others should be checked too)

Event Timeline

Change 705968 had a related patch set uploaded (by Lucas Werkmeister (WMDE); author: Lucas Werkmeister (WMDE)):

[mediawiki/extensions/WikibaseLexeme@master] Don\u2019t use array_merge_recursive() to merge entity types

https://gerrit.wikimedia.org/r/705968

I’ve attached the change I uploaded yesterday to this task, but that’s not the only place where we use array_merge_recursive() for definitions arrays, so if we pick this task up, more work still needs to be done. (Though it shouldn’t be too bad either.)

toan added a subscriber: dang.

Change 714336 had a related patch set uploaded (by Lucas Werkmeister (WMDE); author: Lucas Werkmeister (WMDE)):

[mediawiki/extensions/Wikibase@master] Use wfArrayPlus2d() to merge definitions arrays

https://gerrit.wikimedia.org/r/714336

Change 714360 had a related patch set uploaded (by Lucas Werkmeister (WMDE); author: Lucas Werkmeister (WMDE)):

[mediawiki/extensions/WikibaseLexemeCirrusSearch@master] Use wfArrayPlus2d() to merge definitions arrays

https://gerrit.wikimedia.org/r/714360

I think I’m done, it doesn’t seem to be used in that many extensions after all. (I even checked codesearch for array_merge_recursive( in case I missed any extensions.)

Change 714360 merged by jenkins-bot:

[mediawiki/extensions/WikibaseLexemeCirrusSearch@master] Use wfArrayPlus2d() to merge definitions arrays

https://gerrit.wikimedia.org/r/714360

Change 714336 merged by jenkins-bot:

[mediawiki/extensions/Wikibase@master] Use wfArrayPlus2d() to merge definitions arrays

https://gerrit.wikimedia.org/r/714336

Change 705968 merged by jenkins-bot:

[mediawiki/extensions/WikibaseLexeme@master] Don\u2019t use array_merge_recursive() to merge entity types

https://gerrit.wikimedia.org/r/705968

Change 719083 had a related patch set uploaded (by Addshore; author: Lucas Werkmeister (WMDE)):

[mediawiki/extensions/WikibaseLexeme@REL1_36] Don\u2019t use array_merge_recursive() to merge entity types

https://gerrit.wikimedia.org/r/719083

Change 719084 had a related patch set uploaded (by Addshore; author: Lucas Werkmeister (WMDE)):

[mediawiki/extensions/WikibaseLexemeCirrusSearch@REL1_36] Use wfArrayPlus2d() to merge definitions arrays

https://gerrit.wikimedia.org/r/719084

Change 719084 merged by jenkins-bot:

[mediawiki/extensions/WikibaseLexemeCirrusSearch@REL1_36] Use wfArrayPlus2d() to merge definitions arrays

https://gerrit.wikimedia.org/r/719084

Change 719083 abandoned by Addshore:

[mediawiki/extensions/WikibaseLexeme@REL1_36] Don\u2019t use array_merge_recursive() to merge entity types

Reason:

This was only a maybe, and CI says no, so not going to invest time in this

https://gerrit.wikimedia.org/r/719083