r112598 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:< r112597‎ | r112598 | r112599 >
Date:14:42, 28 February 2012
Author:maxsem
Status:ok (Comments)
Tags:
Comment:
Follow-up r112565: fix code duplication
Modified paths:
  • /trunk/phase3/includes/db/Database.php (modified) (history)
  • /trunk/phase3/includes/db/DatabaseIbm_db2.php (modified) (history)
  • /trunk/phase3/includes/db/DatabaseMssql.php (modified) (history)
  • /trunk/phase3/includes/db/DatabaseMysql.php (modified) (history)
  • /trunk/phase3/includes/db/DatabaseOracle.php (modified) (history)
  • /trunk/phase3/includes/db/DatabasePostgres.php (modified) (history)
  • /trunk/phase3/includes/db/DatabaseSqlite.php (modified) (history)

Diff [purge]

Index: trunk/phase3/includes/db/DatabaseMysql.php
——@@ -154,18 +154,8 @@
155155 /**
156156 * @return bool
157157 */
158 - function close() {
159 - $this->mOpened = false;
160 - if ( $this->mConn ) {
161 - if ( $this->trxLevel() ) {
162 - $this->commit( __METHOD__ );
163 - }
164 - $ret = mysql_close( $this->mConn );
165 - $this->mConn = false;
166 - return $ret;
167 - } else {
168 - return true;
169 - }
 158+ protected function closeConnection() {
 159+ return mysql_close( $this->mConn );
170160 }
171161
172162 /**
Index: trunk/phase3/includes/db/DatabaseOracle.php
——@@ -288,18 +288,8 @@
289289 * Returns success, true if already closed
290290 * @return bool
291291 */
292 - function close() {
293 - $this->mOpened = false;
294 - if ( $this->mConn ) {
295 - if ( $this->mTrxLevel ) {
296 - $this->commit( __METHOD__ );
297 - }
298 - $ret = oci_close( $this->mConn );
299 - $this->mConn = null;
300 - return null;
301 - } else {
302 - return true;
303 - }
 292+ protected function closeConnection() {
 293+ return oci_close( $this->mConn );
304294 }
305295
306296 function execFlags() {
Index: trunk/phase3/includes/db/DatabasePostgres.php
——@@ -240,15 +240,8 @@
241241 * Returns success, true if already closed
242242 * @return bool
243243 */
244 - function close() {
245 - $this->mOpened = false;
246 - if ( $this->mConn ) {
247 - $ret = pg_close( $this->mConn );
248 - $this->mConn = null;
249 - return $ret;
250 - } else {
251 - return true;
252 - }
 244+ protected function closeConnection() {
 245+ return pg_close( $this->mConn );
253246 }
254247
255248 protected function doQuery( $sql ) {
Index: trunk/phase3/includes/db/DatabaseIbm_db2.php
——@@ -557,18 +557,8 @@
558558 * Returns success, true if already closed
559559 * @return bool
560560 */
561 - public function close() {
562 - $this->mOpened = false;
563 - if ( $this->mConn ) {
564 - if ( $this->trxLevel() > 0 ) {
565 - $this->commit( __METHOD__ );
566 - }
567 - $ret = db2_close( $this->mConn );
568 - $this->mConn = null;
569 - return $ret;
570 - } else {
571 - return true;
572 - }
 561+ protected function closeConnection() {
 562+ return db2_close( $this->mConn );
573563 }
574564
575565 /**
Index: trunk/phase3/includes/db/Database.php
——@@ -733,12 +733,28 @@
734734 *
735735 * @return Bool operation success. true if already closed.
736736 */
737 - function close() {
738 - # Stub, should probably be overridden
739 - return true;
 737+ public function close() {
 738+ $this->mOpened = false;
 739+ if ( $this->mConn ) {
 740+ if ( $this->trxLevel() ) {
 741+ $this->commit( __METHOD__ );
 742+ }
 743+ $ret = $this->closeConnection();
 744+ $this->mConn = false;
 745+ return $ret;
 746+ } else {
 747+ return true;
 748+ }
740749 }
741750
742751 /**
 752+ * Closes underlying database connection
 753+ * @since 1.20
 754+ * @return bool: Whether connection was closed successfully
 755+ */
 756+ protected abstract function closeConnection();
 757+
 758+ /**
743759 * @param $error String: fallback error message, used if none is given by DB
744760 */
745761 function reportConnectionError( $error = 'Unknown error' ) {
Index: trunk/phase3/includes/db/DatabaseMssql.php
——@@ -110,15 +110,8 @@
111111 * Returns success, true if already closed
112112 * @return bool
113113 */
114 - function close() {
115 - $this->mOpened = false;
116 - if ( $this->mConn ) {
117 - $ret = sqlsrv_close( $this->mConn );
118 - $this->mConn = null;
119 - return $ret;
120 - } else {
121 - return true;
122 - }
 114+ protected function closeConnection() {
 115+ return sqlsrv_close( $this->mConn );
123116 }
124117
125118 protected function doQuery( $sql ) {
Index: trunk/phase3/includes/db/DatabaseSqlite.php
——@@ -115,16 +115,11 @@
116116 }
117117
118118 /**
119 - * Close an SQLite database
120 - *
 119+ * Does not actually close the connection, just destroys the reference for GC to do its work
121120 * @return bool
122121 */
123 - function close() {
124 - $this->mOpened = false;
125 - if ( is_object( $this->mConn ) ) {
126 - if ( $this->trxLevel() ) $this->commit( __METHOD__ );
127 - $this->mConn = null;
128 - }
 122+ protected function closeConnection() {
 123+ $this->mConn = null;
129124 return true;
130125 }
131126

Follow-up revisions

RevisionCommit summaryAuthorDate
r113602Update code to match changes in r112598reedy00:05, 12 March 2012

Past revisions this follows-up on

RevisionCommit summaryAuthorDate
r112565(bug 34762) Calling close() on a DatabaseBase object now clears the connectio...demon02:18, 28 February 2012

Comments

#Comment by QChris (WMF) (talk | contribs)   21:52, 29 February 2012

This refactoring looks somewhat like an oversimplification.

It sets $this->mConn to false, for all backends upon closure. However, most backends used null for “no connection set” instead of false.

#Comment by Krinkle (talk | contribs)   05:16, 1 March 2012

Hm.. yeah, maybe null does make more sense

#Comment by QChris (WMF) (talk | contribs)   10:34, 1 March 2012

Besides, this refactoring factors out logic into the abstract base class -- i.e. handling/meaning of mConn -- that is inherently bound to backend implementation and is different from backend implementation to backend implementation.


Before this refactoring, it appeared to me, that mConn is under the sole discretion of the backend implementation. The refactoring silently breaks this clear separation without adding documentation about it.

#Comment by MaxSem (talk | contribs)   14:21, 1 March 2012

This class is not code-free, it is for code common for every inherited class, unless they override it - exactly what I'm doing here.

#Comment by QChris (WMF) (talk | contribs)   16:01, 1 March 2012

Yes, the class is not code free. But, the other fields being used within DatabaseBase are either self-explaining (e.g.: mOpened) or come with documentation (e.g.: trxLevel). The refactored way of using mConn is neither of the two.

But mConn now has undocumented restrictions. So the sum of restrictions on mConn are undocumented, and spread across the abstract class, and the class implementing the backend. If either of the two classes changes, those undocumented restrictions on mConn are likely to run apart.

But ignoring those documentation and clean code issues, the code you refactored was neither common for all implementing subclasses, nor did you override it where needed. E.g.: DatabaseOracle changed from mConn === null, for closed connections to mConn === false.

Are the current unit tests on the Oracle backend strong enough to detect eventual breakage arising from this change?

Same for the Postgres,... backend.

For the SQLite backend, the situation is even worse, the refactoring introduces a hidden, logical error. The backend uses $this->mConn = null; within closeConnection but this null is overridden to false immediately afterwards in DatabaseBase's close function.

Sorry, for sounding like a grumpy, old man. I am only old, not grumpy ;) The refactoring seems to work. So let's not burn hours on it, let's keep it :D

Status & tagging log