"QChris" posted a comment on MediaWiki.r112598. URL: http://www.mediawiki.org/wiki/Special:Code/MediaWiki/112598#c31687
Commit summary for MediaWiki.r112598: Follow-up r112565: fix code duplication QChris's comment: 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 <code>mConn</code> is neither of the two. But <code>mConn</code> now has undocumented restrictions. So the sum of restrictions on <code>mConn</code> 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 <code>mConn === null</code>, for closed connections to <code>mConn === false</code>. 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 <code>$this->mConn = null;</code> within <code>closeConnection</code> but this <code>null</code> is overridden to <code>false</code> immediately afterwards in <code>DatabaseBase</code>'s <code>close</code> 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 _______________________________________________ MediaWiki-CodeReview mailing list [email protected] https://lists.wikimedia.org/mailman/listinfo/mediawiki-codereview
