r89263 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:r89262‎ | r89263 | r89264 >
Date:15:15, 1 June 2011
Author:freakolowsky
Status:reverted (Comments)
Tags:
Comment:
* suggestion for ExtensionUpdater that would handle DB install/update of extension from DatabaseUpdater
* could also generate default code for LocalSettingsGenerator
* revert if silly :D
Modified paths:
  • /trunk/phase3/includes/installer/DatabaseUpdater.php (modified) (history)
  • /trunk/phase3/includes/installer/ExtensionUpdater.php (added) (history)

Diff [purge]

Index: trunk/phase3/includes/installer/DatabaseUpdater.php
@@ -31,6 +31,12 @@
3232 protected $extensionUpdates = array();
3333
3434 /**
 35+ * List of extension-provided database updaters
 36+ * @var array
 37+ */
 38+ protected $extensionUpdaters = array();
 39+
 40+ /**
3541 * Used to hold schema files during installation process
3642 * @var array
3743 */
@@ -170,6 +176,17 @@
171177 }
172178
173179 /**
 180+ * Add a updater class that will handle extensions DB install/update.
 181+ * This should be called by extensions while executing the
 182+ * LoadExtensionSchemaUpdates hook.
 183+ *
 184+ * @param $updater (string) classname (extends DatabaseUpdater).
 185+ */
 186+ public function addExtensionUpdater( $updater ) {
 187+ $this->extensionUpdaters[] = $updater;
 188+ }
 189+
 190+ /**
174191 * Convenience wrapper for addExtensionUpdate() when adding a new table (which
175192 * is the most common usage of updaters in an extension)
176193 * @param $tableName String Name of table to create
@@ -205,6 +222,15 @@
206223 return $this->extensionUpdates;
207224 }
208225
 226+ /**
 227+ * Get the list of extension-defined updaters
 228+ *
 229+ * @return Array
 230+ */
 231+ protected function getExtensionUpdaters() {
 232+ return $this->extensionUpdaters;
 233+ }
 234+
209235 public function getPostDatabaseUpdateMaintenance() {
210236 return $this->postDatabaseUpdateMaintenance;
211237 }
@@ -224,6 +250,10 @@
225251 if ( isset( $what['extensions'] ) ) {
226252 $this->runUpdates( $this->getOldGlobalUpdates(), false );
227253 $this->runUpdates( $this->getExtensionUpdates(), true );
 254+ foreach ( $this->getExtensionUpdaters() as $updaterClass ) {
 255+ $eupdater = new $updaterClass(this);
 256+ $eupdater->doUpdates();
 257+ }
228258 }
229259
230260 $this->setAppliedUpdates( $wgVersion, $this->updates );
Index: trunk/phase3/includes/installer/ExtensionUpdater.php
@@ -0,0 +1,16 @@
 2+<?php
 3+
 4+class ExtensionUpdater extends DatabaseUpdater {
 5+
 6+ // overload the constructor, so the init and hooks don't get called
 7+ // while still have all the patching code without duplicating
 8+ public __construct( DatabaseBase &$db, $shared, Maintenance $maintenance = null ) {
 9+ $this->db = $db;
 10+ $this->shared = $shared;
 11+ $this->maintenance = $maintenance;
 12+ }
 13+
 14+ public function doUpdates() {
 15+ $this->runUpdates( $this->getCoreUpdateList(), false );
 16+ }
 17+}
Property changes on: trunk/phase3/includes/installer/ExtensionUpdater.php
___________________________________________________________________
Added: svn:eol-style
118 + native

Follow-up revisions

RevisionCommit summaryAuthorDate
r89297Not silly, but parser error (r89263)... fixing...platonides22:04, 1 June 2011
r89347Fix for r89263: this to $thisplatonides16:10, 2 June 2011
r89586Revert r89263, r89297, r89347: unclear whether this is needed; DatabaseUpdate...brion18:35, 6 June 2011

Comments

#Comment by 😂 (talk | contribs)   15:24, 1 June 2011

Should probably make doUpdates() abstract in the base class.

#Comment by Freakolowsky (talk | contribs)   15:27, 1 June 2011

but the general idea seem sound?

#Comment by Brion VIBBER (talk | contribs)   21:02, 3 June 2011

Can't extensions already add themselves to the DatabaseUpdater list through a hook? Why do we need a subclass?

#Comment by Freakolowsky (talk | contribs)   23:35, 3 June 2011

Currently it only works for the installer part (net even sure if that works correctly).

You can specify a updater function trough the hook but DatabaseUpdater passes itself to that function as a parameter. That class has all the addXXX and applyPatch functions as protected, so you actually can't do nothing useful with it other that using the DB directly and doing DDL stuff. So instead of opening up the updater functions i think it would be better to have an updater specific to the extension inside.

This way you can besides taking care of the installation process also handle the potential DB upgrade of extension specific parts. You can also use this updater to pass some defaults to LocalSettingsGenerator.

The current code works, but it's very limited.

Generally i think this would be a useful feature.

#Comment by MaxSem (talk | contribs)   07:49, 7 June 2011

To elaborate on Brion's revert of this revision: LoadExtensionSchemaUpdates hook works both for installation and upgrades. If it does not work, fix the extension and/or the core updater, but not introduce an alternative update infrastructure. As of adding stuff to LocalSettings, just use sane defaults.

#Comment by Freakolowsky (talk | contribs)   08:11, 7 June 2011

No need for elaboration i totally understand your (and Brion's) logic. If it works why change it.

What i fail to understand is your resistance to a simple "let's make this ugly thing nicer" idea. It doesn't break anything ...

But hey ... i'll let it be ... got other things to do :D

Status & tagging log