r69229 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:r69228‎ | r69229 | r69230 >
Date:10:15, 10 July 2010
Author:maxsem
Status:resolved (Comments)
Tags:
Comment:
Fixed mysqlisms in Database.php comments, abstracted getSearchEngine()
Modified paths:
  • /trunk/phase3/includes/db/Database.php (modified) (history)
  • /trunk/phase3/includes/db/DatabaseMysql.php (modified) (history)

Diff [purge]

Index: trunk/phase3/includes/db/DatabaseMysql.php
@@ -383,6 +383,9 @@
384384 }
385385 }
386386
 387+ /**
 388+ * FROM MYSQL DOCS: http://dev.mysql.com/doc/refman/5.0/en/miscellaneous-functions.html#function_release-lock
 389+ */
387390 public function unlock( $lockName, $method ) {
388391 $lockName = $this->addQuotes( $lockName );
389392 $result = $this->query( "SELECT RELEASE_LOCK($lockName) as lockstatus", $method );
@@ -410,6 +413,16 @@
411414 $this->query( "UNLOCK TABLES", $method );
412415 }
413416
 417+ /**
 418+ * Get search engine class. All subclasses of this
 419+ * need to implement this if they wish to use searching.
 420+ *
 421+ * @return String
 422+ */
 423+ public function getSearchEngine() {
 424+ return 'SearchMySQL';
 425+ }
 426+
414427 public function setBigSelects( $value = true ) {
415428 if ( $value === 'default' ) {
416429 if ( $this->mDefaultBigSelects === null ) {
Index: trunk/phase3/includes/db/Database.php
@@ -4,7 +4,7 @@
55 *
66 * @file
77 * @ingroup Database
8 - * This file deals with MySQL interface functions
 8+ * This file deals with database interface functions
99 * and query specifics/optimisations
1010 */
1111
@@ -2324,7 +2324,6 @@
23252325 * @param $lockName String: Name of lock to release
23262326 * @param $method String: Name of method calling us
23272327 *
2328 - * FROM MYSQL DOCS: http://dev.mysql.com/doc/refman/5.0/en/miscellaneous-functions.html#function_release-lock
23292328 * @return Returns 1 if the lock was released, 0 if the lock was not established
23302329 * by this thread (in which case the lock is not released), and NULL if the named
23312330 * lock did not exist
@@ -2355,14 +2354,12 @@
23562355 }
23572356
23582357 /**
2359 - * Get search engine class. All subclasses of this
2360 - * need to implement this if they wish to use searching.
 2358+ * Get search engine class. Subclasses that don't support a search engine
 2359+ * should return 'SearchEngineDummy'.
23612360 *
23622361 * @return String
23632362 */
2364 - public function getSearchEngine() {
2365 - return "SearchMySQL";
2366 - }
 2363+ public abstract function getSearchEngine();
23672364
23682365 /**
23692366 * Allow or deny "big selects" for this session only. This is done by setting

Follow-up revisions

RevisionCommit summaryAuthorDate
r69230Update tests for r69229maxsem10:26, 10 July 2010
r69727Fix r69229. This should not be abstract, return dummy in parentdemon13:04, 22 July 2010

Comments

#Comment by 😂 (talk | contribs)   10:43, 10 July 2010

"Subclasses that don't support a search engine should return 'SearchEngineDummy'."

Wouldn't it make more sense to have DatabaseBase return SearchEngineDummy, and then child classes can return an appropriate class once it's implemented?

#Comment by MaxSem (talk | contribs)   11:59, 10 July 2010

Such approach is good when there are classes both implementing the function and not implementing. However, everything we currently have implements its own search and in this situation it's better not to have a function that is rendered unused because it's always overridden.

#Comment by Simetrical (talk | contribs)   21:16, 12 July 2010

I agree with ^demon. Abstract methods should only be used if something absolutely must be implemented for the class to be functional. Otherwise, you force anyone making a new subclass to copy-paste lots of boilerplate dummy functions until they've implemented all the features. Where possible, DatabaseBase should implement dummy functions, so that child classes don't have to override them if they have nothing better to substitute.

Status & tagging log