https://bugzilla.wikimedia.org/show_bug.cgi?id=22093
--- Comment #10 from Aryeh Gregor <[email protected]> 2010-05-25 23:57:41 UTC --- Sorry for the long delay, I got caught up in finals and then procrastinated. :( I should be able to review any further revisions quickly, now that the summer has started. I agree with most of what Max said. No comments on the Installer, search-related stuff, or the schema. As for the rest, generally there are lots of places where you don't follow MediaWiki style. Most of these should be fixable by running stylize.php. I've noted some places that will have to be fixed manually, but omitted a bunch of inconsequential stylistic issues that can be easily fixed after commit, since the patch is already so large. + # BEGIN DatabaseMssqlNative hack + # 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'; + } + # End DatabaseMssqlNative hack This is indented one tab too much. +class result_mssqlnative { + + public function result_mssqlnative($queryresult=false) { + $this->mCursor = 0; + $this->mRows = array(); + $this->mNumFields = sqlsrv_num_fields( $queryresult); The name of this class and most of its methods don't follow MediaWiki naming conventions. I'd use Result_MssqlNative here, and arrayToObj() etc. for the methods. Also, indentation here is weird. Should be: class result_mssqlnative { public function result_mssqlnative($queryresult=false) { $this->mCursor = 0; $this->mRows = array(); $this->mNumFields = sqlsrv_num_fields( $queryresult); Indentation is weird in a bunch of other places too, and stylize.php can't fix all of this. Try switching your tab stops to 8 briefly so you can spot it easily, or configure your development environment to flag indentation with spaces. Your new patch is really hard to read at my browser's tab settings, and the last patch wasn't. + if (!strlen($user)) { ## e.g. the class is being loaded + return; + } More explanation here, please -- why would any caller call open() with an empty $user, particularly when it will just return immediately? + ($ntAuthPassTest == 'ntauth' && $ntAuthUserTest == 'ntauth') ? $this->mConn = @sqlsrv_connect($server, array('Database'=>$dbName)) : $this->mConn = @sqlsrv_connect($server, array('Database'=>$dbName,'UID'=>$user,'PWD'=>$password)); Use if/else here, please, on separate lines. + // several extensions seem to think that all databases support limits via LIMIT N after the WHERE clause + // well, MSSQL uses SELECT TOP N, so to catch any of those extensions we'll do a quick check for a LIMIT + // clause and pass $sql through $this->LimitToTopN() which parses the limit clause and passes the result to + // $this->limitResult(); + if ( preg_match( '/\bLIMIT\s*/i', $sql ) ) > 0) { This is evil, but I'm not sure there's a better solution. :( Maybe make your check a little more careful, though -- this might cause all sorts of crazy breakage if you do SELECT * FROM user WHERE user_name='LIMIT' or something. + // 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); + } I think you can drop this code. I wouldn't worry about extensions using EXTRACT() unless you know of some that actually do (and which can't be easily changed). First of all, it's not a common need. And second of all, EXTRACT() doesn't work for anything other than PostgreSQL anyway AFAICT, so it's unlikely that anything would be hardcoding it, except for those two cases in core (which probably started by splitting MySQL to its own case then pgsql to the default case, and other DBs added to their own cases). + $message = "A database error has occurred<br/>\n" . + "Query: $sql<br/>\n" . + "Function: ".__FUNCTION__."<br/>\n"; + // process each error (our driver will give us an array of errors unlike other providers) + foreach( sqlsrv_errors() as $error) { + $message .= $message."ERROR[".$error['code']."] ".$error['message']."<br/>"; + } + + throw new DBUnexpectedError($this, $message); Does this actually work as expected? It looks like DBUnexpectedError takes a plaintext string for $message, not HTML, so you shouldn't be using <br /> and such. nl2br( htmlspecialchars() ) will be run for HTML output. Also, if this did accept HTML output, you'd want to htmlspecialchars() on $error['code'] and $error['message'] before outputting. + if ((strpos(" ".$sql, "SELECT") > 0) || + ((strpos(" ".$sql, "INSERT") > 0) && (strpos(" ".$sql, 'OUTPUT INSERTED') > 0))) Use strpos( $haystack, $needle ) !== false instead of strpos( " $haystack", $needle ) > 0. + if ( !(isset( $arrToInsert[0] ) && is_array( $arrToInsert[0] )) ) {//Not multi row + $arrToInsert = array(0=>$arrToInsert);//make everything multi row compatible + } Maybe this would work: $arrToInsert = (array)$arrToInsert; + $bAllOk = true; As Max says, use $allOk here. + $pregIgnoreIdentity = '/recentchanges|webauth_user|site_stats|watchlist|user_groups|externallinks|image|pagelinks|categorylinks|templatelinks/i'; ... // we want to figure out the identity column, so we examine each column name in the array + foreach($a as $k => $v) { + // all mediawiki columns (except for cl_from) are in format of XX_id and XX_XXX_id Ouch. There's really no better way to do this? + $sql = preg_replace('/\bSELECT(\s*DISTINCT)?\b/Dsi', 'SELECT$1 TOP(10000000) ', $sql,1); I don't get what this line does. + function deadlockLoop() { This appears to be copied line-by-line in its entirety from DatabaseBase, with only these lines changed: + $this->rollback($myFname); ... + $this->commit($myFname); Just change those two lines in DatabaseBase, please, instead of copying the function. Your new versions should work there too. + function addQuotes( $s ) { + if ( is_null( $s ) ) { + return 'NULL'; + } else if ($s instanceof Blob) { + return "'".$s->fetch($s)."'"; + } + return "'" . $this->strencode( $s ) . "'"; + } Better for clarity to do: function addQuotes( $s ) { if ($s instanceof Blob) { return "'".$s->fetch($s)."'"; } else { return parent::addQuotes( $s ); } } + function getDBname() { + return $this->mDBname; + } + + function getServer() { + return $this->mServer; + } These seem to be duplicates of the parent methods and could be dropped. -- 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 [email protected] https://lists.wikimedia.org/mailman/listinfo/wikibugs-l
