r60757 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:< r60756‎ | r60757 | r60758 >
Date:00:22, 7 January 2010
Author:tstarling
Status:ok (Comments)
Tags:
Comment:
Add a Database::getType() function so that we can get rid of $wgDBtype in favour of OOP goodness.
Modified paths:
  • /trunk/phase3/includes/db/Database.php (modified) (history)

Diff [purge]

Index: trunk/phase3/includes/db/Database.php
——@@ -284,6 +284,17 @@
285285 }
286286 }
287287
 288+ /**
 289+ * Get the type of the DBMS, as it appears in $wgDBtype.
 290+ */
 291+ function getType() {
 292+ if ( preg_match( '/^Database(\w+)$/', get_class( $this ), $m ) ) {
 293+ return strtolower( $m[1] );
 294+ } else {
 295+ throw new MWException( get_class( $this ) .'::getType: unable to determine type.' );
 296+ }
 297+ }
 298+
288299 #------------------------------------------------------------------------------
289300 # Other functions
290301 #------------------------------------------------------------------------------

Follow-up revisions

RevisionCommit summaryAuthorDate
r60816For r60757: implement Database::getType() explicitly in each subclass, to mak...tstarling00:31, 8 January 2010

Comments

#Comment by Catrope (talk | contribs)   09:13, 7 January 2010

Why not just make getType() abstract and have subclasses implement it, a la QueryPage::getName()? That'd be cleaner than regexing the class name.

Also, I have my doubts as to whether $wgDBtype == 'foo' should be changed to $db->getType() == 'foo' ; we might as well use $db instanceof DatabaseFoo as long as we're just checking for a specific DB type and don't need to grab the DB type as a string.

#Comment by Tim Starling (talk | contribs)   12:41, 7 January 2010
Why not just make getType() abstract and have subclasses implement it, a la QueryPage::getName()? That'd be cleaner than regexing the class name.

Because the regex matches the way class names are constructed in LoadBalancer. I did start writing it as an abstract function, but I changed it because this way is simpler. It's the interface I'm interested in, the implementation of getType() can be changed at any time.

Also, I have my doubts as to whether $wgDBtype == 'foo' should be changed to $db->getType() == 'foo' ; we might as well use $db instanceof DatabaseFoo as long as we're just checking for a specific DB type and don't need to grab the DB type as a string.

The reasons I didn't use instanceof are:

  1. The database class name may at some point need to be divorced from the type name
  2. Having a function to get the type allows you to feed that type back into the LoadBalancer configuration to create more database objects of the same type. In the future, there will be a factory function that accepts that type as a parameter, it won't accept the database class name.
  3. instanceof breaks if you try to create a proxy object with __call(), like StubObject
  4. instanceof would not necessarily work if you had an extension which implemented the same type as a core class, overriding the core class. When you use an abstract type name like that from getType() instead of the class name, you can add a hook to the factory function allowing the class name to be overridden by extensions. instanceof would work in this case if the extension class was a subclass of the core class, but not otherwise.


Status & tagging log