https://bugzilla.wikimedia.org/show_bug.cgi?id=22093


Aryeh Gregor <simetrical+wikib...@gmail.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |simetrical+wikib...@gmail.co
                   |                            |m




--- Comment #5 from Aryeh Gregor <simetrical+wikib...@gmail.com>  2010-01-13 
23:42:12 UTC ---
If there are only slight changes to a method like selectSQLText(), you should
probably adjust the parent method a little so that the differences are pulled
out into separate methods you can override in child classes.  Notice how
selectSQLText() already calls methods like limitResult() so child classes don't
have to duplicate code; it's not meant to be overridden by children.

Also, if your class has some methods identical to DatabaseMssql's, you might
want to inherit from it instead of DatabaseBase.  It can stay in a separate
file, the autoloader will take care of include order.  You should definitely
share code wherever possible -- don't feel too bound by current conventions as
far as file arrangement, since this is the first time we've got support for two
different drivers for the same DB.  If it makes more sense to use the same
schema, say, then do that.

You should consider requesting commit access here:

http://www.mediawiki.org/wiki/Commit_access_requests

Someone who wants to do general-purpose commits normally needs to get a bunch
of patches approved first, but if your only aim is to maintain support for a
database, you can probably get commit access for that purpose right away, or at
least that's been the case sometimes in the past.  But it can sometimes take a
while before new requests get looked at.


Comments:

diff -rupN
/home/chris/Desktop/RemoteDesktop/mediawiki-1.15.1.tar/mediawiki-1.15.1/mediawiki115/config/index.php
 

Patches should be submitted against SVN trunk so they apply cleanly.  See
<http://www.mediawiki.org/wiki/Subversion>.  On Windows, your best bet is
probably to use TortoiseSVN, check out
<http://svn.wikimedia.org/svnroot/mediawiki/trunk/phase3> someplace, make your
changes, right-click and select "Create Patch" (or whatever, haven't used it
for years).  Failing that, it would help if you at least made your patches
against nightly tarballs: <http://toolserver.org/~vvv/mw-nightly/>

-
-               if( $conf->DBtype == 'mysql' ) {
...
+               switch($conf->DBtype) {
+                       case 'mysql':

This is a good place to use a switch, you're right, but it's best if you don't
make unrelated changes like this in your patches.  They make it harder to
review.  Especially since you've changed indentation, so all the lines are
changed and it's hard to see which ones are actually different.

+       'SearchMssqlnative' => 'includes/SearchMSSQLNative.php',
...
+       'DatabaseMssqlnative' => 'includes/db/DatabaseMssqlnative.php',         

Probably SearchMssqlNative, DatabaseMssqlNative, DatabaseMssqlNative.php would
be more expected capitalizations.

+               # since MSSQL doesn't recognize the infinity keyword, set date
manually
+               # TO-DO: refactor for better DB portability and remove magic
date
+               $dbw = wfGetDB(DB_MASTER);
+               if($dbw instanceof DatabaseMssqlnative)
+               {
+                       return '3000-01-31 00:00:00.000';
+               }

I think the easiest way to handle this is how DatabaseOracle does it:

                if ( preg_match( '/^timestamp.*/i', $col_type ) == 1 &&
strtolower( $val ) == 'infinity' ) {
                        $val = '31-12-2030 12:00:00.000000';
                }

That just has that in DatabaseOracle::insertOneRow().  You might need it in a
couple other methods, but this is easier to do than changing all callers, and
should be reliable.

I notice you aren't consistently following
<http://www.mediawiki.org/wiki/Manual:Coding_conventions>.  In particular,
indentation should be one tab (no spaces), and all parentheses should have a
space inside (e.g., is_object( $v ) not is_object($v)).  You can run
stylize.php
<http://svn.wikimedia.org/viewvc/mediawiki/trunk/tools/code-utils/stylize.php?view=markup>
on your file to fix a lot of this stuff, or just leave it and someone else can
do it before committing.

+class DatabaseMssqlnative extends Database {

You should extend DatabaseBase.  Database is a legacy-compat class that's the
same as DatabaseMysql.

+               global $wgOut;
+               # Can't get a reference if it hasn't been set yet
+               if ( !isset( $wgOut ) ) {
+                       $wgOut = NULL;
+               }

Surely you mean to set $this->mOut here?  But you never seem to use
$this->mOut, so maybe you should just not set it at all.  I'm not sure why
you'd want it, anyway.

+        // lotsa components seem to think that all databases support limits
via LIMIT N after the WHERE clause
+        // well, MSSQL uses SELECT TOP N, so instead of going and rewriting
all of those extensions we attempt
+        // to do a more roboust mapping here
+        if (strpos($sql,'LIMIT ')  > 0) {
+            // massage LIMIT -> TopN
+            $sql = $this->LimitToTopN($sql) ;
+        }

This is wrong -- you should use limitResult() instead.  Callers that use LIMIT
directly should be changed to use limitResult().  LIMIT also breaks for Oracle
and non-native Mssql.

+        // MSSQL doesn't have EXTRACT(epoch FROM XXX) 
+        if (strpos($sql, "EXTRACT(epoch FROM ") > 0) {
+            // This is same as UNIX_TIMESTAMP, we need to calc # of seconds
from 1970
+            $sql = str_replace("EXTRACT(epoch FROM ",
"DATEDIFF(s,CONVERT(datetime,'1/1/1970'),", $sql);
+        }

This is currently only called in two places,
includes/specials/SpecialAncientpages.php and
includes/specials/SpecialUnusedimages.php.  Both of those use switch (
$wgDBType ) and feed totally different results to different DBs.  This is bad,
we should add a new method for this, but for now you should just update those
two callers (unless you want to add a method that abstracts this logic).

+            echo $message;

Why are you doing this?  Just throw the exception like all the other DBs.  That
will print out the error message along with a stack trace by default.

+       function select( $table, $vars, $conds='', $fname = 'Database::select',
$options = array(), $join_conds = array() )
...
+       function selectSQLText( $table, $vars, $conds='', $fname =
'Database::select', $options = array(), $join_conds = array() ) {

You shouldn't need to override either of these.  If there are differences,
please adjust the parent method to account for them, and add an extra method,
the way limitResult() is currently used.

+       function useIndexClause( $index ) {
+               return '';
+       }

This is the same as DatabaseBase, you don't need to override it.  It's
basically a MySQL-specific hack.

+       function lowPriorityOption() {
+               return '';
+       }

Ditto.

+                       } else {
+                               return
preg_replace('/SELECT(\s*DISTINCT)?/Dsi', 'SELECT$1 TOP '.$limit, $sql);  
+                       }               
+               } else {
+                       $sql = preg_replace('/SELECT(\s*DISTINCT)?/Dsi',
'SELECT$1 TOP(10000000) ', $sql);  

You probably want to only replace the first occurrence.  Otherwise this will
result in really weird bugs when you have a user whose name contains "select"
or something.

+    // this massages a LIMIT OFFSET,COUNT clause into a TOP N clause
+    function LimitToTopN($sql) {

Why is this needed?  Callers should not be concatenating LIMIT onto anything. 
If they are, the callers should be fixed, the database layer shouldn't be
trying to parse SQL queries using explode() -- that's unnecessarily difficult
and will surely cause bugs.

+       function limitResultForUpdate($sql, $num) {
+               return $sql;
+       }

I guess this isn't implemented?  Probably not a big deal.  You should note in a
comment if MSSQL doesn't support this.

+       function conditional( $cond, $trueVal, $falseVal ) {
+               return " (CASE WHEN $cond THEN $trueVal ELSE $falseVal END) ";
+       }

This is also identical to the parent class.

I'm not familiar with SearchEngine and didn't review SearchMssqlnative.

-               }
-
-               $res = $dbr->query( $sql, __METHOD__ );

-               if( $res->numRows() == 0 )
+                       $res = $dbr->query($sql, __METHOD__ );
+                                               
+                       if( $res->numRows() == 0 )
                        $this->mResultEmpty = true;
+                       
+               }

I don't understand what this is meant to do.


-- 
Configure bugmail: https://bugzilla.wikimedia.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.
You are on the CC list for the bug.

_______________________________________________
Wikibugs-l mailing list
Wikibugs-l@lists.wikimedia.org
https://lists.wikimedia.org/mailman/listinfo/wikibugs-l

Reply via email to