Page MenuHomePhabricator

DatabaseUpdater runMaintenance() ignores the $script parameter
Closed, ResolvedPublic

Description

The 'runMaintenance' method in DatabaseUpdater is used like this (example from AbuseFilter):

		$updater->addExtensionUpdate( [
			'runMaintenance',
			MigrateActorsAF::class,
			__DIR__ . '/../../../maintenance/MigrateActorsAF.php',
		] );

However, the last parameter ($script) is mostly ignored – it's just printed in the output of update.php, but the script is not actually loaded from the given path.

This is bad for 3 reasons:

  • Developers waste their time inputting and maintaining these file paths
  • Different extensions specify them differently (CodeSearch) – as absolute path, relative to MW root, relative to extension root, or relative to the current file
  • It hides the fact that you need to set up the script class to be loaded via AutoLoader, providing the path here won't work
  • We're trying to move away from specifying maintenance scripts by file name (see T99268)

We should mark this parameter as optional and unused, and then remove it from the code and any examples.

Details

SubjectRepoBranchLines +/-
mediawiki/coreREL1_39+18 -16
mediawiki/extensions/WikiForummaster+1 -7
mediawiki/extensions/AJAXPollmaster+1 -6
mediawiki/extensions/VoteNYmaster+1 -2
mediawiki/extensions/QuizGamemaster+1 -16
mediawiki/extensions/Commentsmaster+1 -4
mediawiki/extensions/SocialProfilemaster+0 -13
mediawiki/extensions/PollNYmaster+1 -6
mediawiki/extensions/Videomaster+1 -6
mediawiki/extensions/FanBoxesmaster+1 -6
mediawiki/extensions/OATHAuthmaster+1 -3
mediawiki/extensions/AbuseFiltermaster+0 -1
mediawiki/extensions/CentralAuthmaster+1 -2
mediawiki/extensions/CheckUsermaster+0 -4
mediawiki/coremaster+27 -25
Show related patches Customize query in gerrit

Event Timeline

Change #1047171 had a related patch set uploaded (by Bartosz Dziewoński; author: Bartosz Dziewoński):

[mediawiki/core@master] DatabaseUpdater: Don't require script path in 'runMaintenance'

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

Change #1047171 merged by jenkins-bot:

[mediawiki/core@master] DatabaseUpdater: Don't require script path in 'runMaintenance'

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

I'll make patches for all the extensions too.

Change #1047574 had a related patch set uploaded (by Bartosz Dziewoński; author: Bartosz Dziewoński):

[mediawiki/extensions/AbuseFilter@master] LoadExtensionSchemaUpdates: Remove unused path from 'runMaintenance' action

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

Change #1047575 had a related patch set uploaded (by Bartosz Dziewoński; author: Bartosz Dziewoński):

[mediawiki/extensions/AJAXPoll@master] LoadExtensionSchemaUpdates: Remove unused path from 'runMaintenance' action

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

Change #1047576 had a related patch set uploaded (by Bartosz Dziewoński; author: Bartosz Dziewoński):

[mediawiki/extensions/CentralAuth@master] LoadExtensionSchemaUpdates: Remove unused path from 'runMaintenance' action

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

Change #1047577 had a related patch set uploaded (by Bartosz Dziewoński; author: Bartosz Dziewoński):

[mediawiki/extensions/CheckUser@master] LoadExtensionSchemaUpdates: Remove unused path from 'runMaintenance' action

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

Change #1047578 had a related patch set uploaded (by Bartosz Dziewoński; author: Bartosz Dziewoński):

[mediawiki/extensions/Comments@master] LoadExtensionSchemaUpdates: Remove unused path from 'runMaintenance' action

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

Change #1047579 had a related patch set uploaded (by Bartosz Dziewoński; author: Bartosz Dziewoński):

[mediawiki/extensions/FanBoxes@master] LoadExtensionSchemaUpdates: Remove unused path from 'runMaintenance' action

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

Change #1047580 had a related patch set uploaded (by Bartosz Dziewoński; author: Bartosz Dziewoński):

[mediawiki/extensions/OATHAuth@master] LoadExtensionSchemaUpdates: Remove unused path from 'runMaintenance' action

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

Change #1047581 had a related patch set uploaded (by Bartosz Dziewoński; author: Bartosz Dziewoński):

[mediawiki/extensions/PollNY@master] LoadExtensionSchemaUpdates: Remove unused path from 'runMaintenance' action

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

Change #1047582 had a related patch set uploaded (by Bartosz Dziewoński; author: Bartosz Dziewoński):

[mediawiki/extensions/QuizGame@master] LoadExtensionSchemaUpdates: Remove unused path from 'runMaintenance' action

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

Change #1047583 had a related patch set uploaded (by Bartosz Dziewoński; author: Bartosz Dziewoński):

[mediawiki/extensions/SocialProfile@master] LoadExtensionSchemaUpdates: Remove unused path from 'runMaintenance' action

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

Change #1047584 had a related patch set uploaded (by Bartosz Dziewoński; author: Bartosz Dziewoński):

[mediawiki/extensions/Video@master] LoadExtensionSchemaUpdates: Remove unused path from 'runMaintenance' action

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

Change #1047585 had a related patch set uploaded (by Bartosz Dziewoński; author: Bartosz Dziewoński):

[mediawiki/extensions/VoteNY@master] LoadExtensionSchemaUpdates: Remove unused path from 'runMaintenance' action

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

Change #1047586 had a related patch set uploaded (by Bartosz Dziewoński; author: Bartosz Dziewoński):

[mediawiki/extensions/WikiForum@master] LoadExtensionSchemaUpdates: Remove unused path from 'runMaintenance' action

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

Change #1047577 merged by jenkins-bot:

[mediawiki/extensions/CheckUser@master] LoadExtensionSchemaUpdates: Remove unused path from 'runMaintenance' action

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

Change #1047576 merged by jenkins-bot:

[mediawiki/extensions/CentralAuth@master] LoadExtensionSchemaUpdates: Remove unused path from 'runMaintenance' action

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

Change #1047574 merged by jenkins-bot:

[mediawiki/extensions/AbuseFilter@master] LoadExtensionSchemaUpdates: Remove unused path from 'runMaintenance' action

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

Change #1047575 merged by jenkins-bot:

[mediawiki/extensions/AJAXPoll@master] LoadExtensionSchemaUpdates: Remove unused path from 'runMaintenance' action

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

Change #1047578 merged by jenkins-bot:

[mediawiki/extensions/Comments@master] LoadExtensionSchemaUpdates: Remove unused path from 'runMaintenance' action

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

Change #1047585 merged by jenkins-bot:

[mediawiki/extensions/VoteNY@master] LoadExtensionSchemaUpdates: Remove unused path from 'runMaintenance' action

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

Change #1047586 merged by jenkins-bot:

[mediawiki/extensions/WikiForum@master] LoadExtensionSchemaUpdates: Remove unused path from 'runMaintenance' action

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

Change #1047580 merged by jenkins-bot:

[mediawiki/extensions/OATHAuth@master] LoadExtensionSchemaUpdates: Remove unused path from 'runMaintenance' action

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

Change #1047582 merged by jenkins-bot:

[mediawiki/extensions/QuizGame@master] LoadExtensionSchemaUpdates: Remove unused path from 'runMaintenance' action

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

Can this be backported to 1.39 (and whatever newer, non-LTS versions are currently supported)?

For context: quite a few social tools are impacted by this, but as per mw:Social tools/MediaWiki compatibility, Social-Tools target the latest LTS release, 1.39 currently. The updated MW requirement in several of the patches breaks this promise to our end-users, but the core patch itself is both rather small and also sensible, which is why I think backporting oughta be doable.

I'll make patches for all the extensions too.

I genuinely wish more people did this. Thank you for taking the time to do so, even if I literally am complaining here. (But my complaints are not about the code or the patches, but rather about longstanding procedural issues with code review, really...)

I'm not sure if it's backportable, but if you want to keep compatibility with 1.39, then just don't merge the extension patches (I can abandon them or mark them as WIP if you'd like), or revert them if any have been already merged. MW 1.43 and later will ignore the parameter if it is provided, just like earlier versions did, so keeping it doesn't hurt.

Change #1051157 had a related patch set uploaded (by Jack Phoenix; author: Jack Phoenix):

[mediawiki/core@REL1_39] DatabaseUpdater: Don't require script path in 'runMaintenance'

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

Change #1047579 abandoned by Bartosz Dziewoński:

[mediawiki/extensions/FanBoxes@master] LoadExtensionSchemaUpdates: Remove unused path from 'runMaintenance' action

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

Change #1047584 abandoned by Bartosz Dziewoński:

[mediawiki/extensions/Video@master] LoadExtensionSchemaUpdates: Remove unused path from 'runMaintenance' action

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

Change #1047581 abandoned by Bartosz Dziewoński:

[mediawiki/extensions/PollNY@master] LoadExtensionSchemaUpdates: Remove unused path from 'runMaintenance' action

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

Change #1047583 abandoned by Bartosz Dziewoński:

[mediawiki/extensions/SocialProfile@master] LoadExtensionSchemaUpdates: Remove unused path from 'runMaintenance' action

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

Change #1051157 merged by jenkins-bot:

[mediawiki/core@REL1_39] DatabaseUpdater: Don't require script path in 'runMaintenance'

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