Page MenuHomePhabricator

Call LoadExtensionSchemaUpdates later
Closed, ResolvedPublic

Description

Handlers of the LoadExtensionSchemaUpdates hook are supposed to add callbacks to DatabaseUpdater. The callbacks are called during the relevant install stage. Checks for table existence, field existence, etc. are supposed to be done during the callback, not during LoadExtensionSchemaUpdates. Doing the checks late allows the callback to take into account the effect of prior schema updates.

Unfortunately this expectation was not properly documented and is not enforced. Many extensions call tableExists() during hook execution to decide whether or not to add an update. Wikibase actually ran the whole update from the hook, i.e. during object construction, with catastrophic results (T249565). T157651 described this as an issue with sql.php, but the problem is deeper than that.

Code review observations:

  • Many extensions call $updater->getDB()->getType()
  • Some extensions type-hint the $updater parameter
  • Some extensions call $updater->tableExists()
  • ContributionTracking optionally gets its own connection to a foreign DB and updates that immediately (not in a callback)
  • Echo calls $updater->getDB()->indexExists()
  • EmailCapture calls $updater->getDB()->tableExists()
  • QuizGame calls $updater->getDB()->fieldExists()

I think the simplest way to deal with this would be to run LoadExtensionSchemaUpdates later: from doUpdates() instead of __construct(). Then it will be safe, if not strictly correct, to access the database. Apparently nothing actually needs to inspect the update list after construction, so there's no need to call the hook so early.

Eventually, LoadExtensionSchemaUpdates should be deprecated. It should be replaced by either a completely declarative interface (e.g. a JSON file registered in extension.json) or a functional interface, in which the extension is only called when it is safe to directly perform conditional updates, or two distinct interfaces serving both styles.

Event Timeline

Restricted Application added a subscriber: Aklapper. · View Herald TranscriptApr 7 2020, 6:31 AM
Anomie added a subscriber: Anomie.Apr 7 2020, 2:41 PM

Eventually, LoadExtensionSchemaUpdates should be deprecated. It should be replaced by either a completely declarative interface (e.g. a JSON file registered in extension.json) or a functional interface, in which the extension is only called when it is safe to directly perform conditional updates, or two distinct interfaces serving both styles.

T191231: RFC: Abstract schemas and schema changes includes that completely declarative interface.

Change 587223 had a related patch set uploaded (by Daniel Kinzler; owner: Daniel Kinzler):
[mediawiki/core@master] DatabaseUpdater: don't call hook in constructor

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

Change 587386 had a related patch set uploaded (by Tim Starling; owner: Tim Starling):
[mediawiki/extensions/Cognate@master] Do not extend DatabaseUpdater, copy its functionality instead

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

Change 587223 merged by jenkins-bot:
[mediawiki/core@master] DatabaseUpdater: don't call hook in constructor

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

daniel closed this task as Resolved.Apr 8 2020, 9:04 AM
daniel claimed this task.
Reedy added a subscriber: Reedy.EditedApr 8 2020, 4:28 PM

Change 587223 merged by jenkins-bot:
[mediawiki/core@master] DatabaseUpdater: don't call hook in constructor

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

This seems to have caused T249732: Database Update broken on beta (based on merging time) and break beta-update-databases-eqiad

Is it fixed by merging Tim's patch above?

Change 587386 merged by jenkins-bot:
[mediawiki/extensions/Cognate@master] Do not extend DatabaseUpdater, copy its functionality instead

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