[Bug 22093] Native Microsoft SQL Server Support
https://bugzilla.wikimedia.org/show_bug.cgi?id=22093 db duplicate...@googlemail.com changed: What|Removed |Added CC||duplicate...@googlemail.com --- Comment #36 from db duplicate...@googlemail.com --- installer and updater support was added with gerrit 105138 -- You are receiving this mail because: 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
[Bug 22093] Native Microsoft SQL Server Support
https://bugzilla.wikimedia.org/show_bug.cgi?id=22093 Andre Klapper aklap...@wikimedia.org changed: What|Removed |Added Priority|Normal |Lowest --- Comment #35 from Andre Klapper aklap...@wikimedia.org --- Again: WONTFIX means actively against fixing the issue. If you're not against MSSQL support, this is not a WONTFIX. I'd recommend to exclude lowest priority tickets in your search queries instead. -- You are receiving this mail because: 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
[Bug 22093] Native Microsoft SQL Server Support
https://bugzilla.wikimedia.org/show_bug.cgi?id=22093 --- Comment #32 from Max Semenik maxsem.w...@gmail.com --- Is there anyone willing to actually develop MSSQL support? Not just commit a patch and run away, but to be a caring maintainer that continuously keeps the thing up to date? So far there were several attempts, but all of them were former, not latter. Until a proof of long-term commitments can be demonstrated, I agree with Chad. -- You are receiving this mail because: 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
[Bug 22093] Native Microsoft SQL Server Support
https://bugzilla.wikimedia.org/show_bug.cgi?id=22093 --- Comment #33 from Mark A. Hershberger m...@everybody.org --- I agree that we need a maintainer. That we don't currently have one does not make this bug WONTFIX -- it makes it a valid, open bug. -- You are receiving this mail because: 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
[Bug 22093] Native Microsoft SQL Server Support
https://bugzilla.wikimedia.org/show_bug.cgi?id=22093 --- Comment #34 from Max Semenik maxsem.w...@gmail.com --- Meh, that's what LATER was. However, I seriously disagree with your revert of the removal - we shouldn't have deadbeef in our code base. -- You are receiving this mail because: 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
[Bug 22093] Native Microsoft SQL Server Support
https://bugzilla.wikimedia.org/show_bug.cgi?id=22093 Chad H. innocentkil...@gmail.com changed: What|Removed |Added Status|NEW |RESOLVED Resolution|--- |WONTFIX --- Comment #30 from Chad H. innocentkil...@gmail.com --- We've never had solid support and we've effectively been completely broken since the new installer landed and we didn't include Mssql support there. I'm removing the broken Mssql support from core in gerrit change 95651. Considering Mssql support for MediaWiki is a story of cookie licking and broken promises, I'm being bold and WONTFIXing this for good. -- You are receiving this mail because: 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
[Bug 22093] Native Microsoft SQL Server Support
https://bugzilla.wikimedia.org/show_bug.cgi?id=22093 Mark A. Hershberger m...@everybody.org changed: What|Removed |Added Status|RESOLVED|REOPENED CC||m...@everybody.org Resolution|WONTFIX |--- --- Comment #31 from Mark A. Hershberger m...@everybody.org --- Reverted and reopened. I did work this past summer on supporting SQL Server and MS has asked for this as well, so it isn't just cookie-licking. -- You are receiving this mail because: 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
[Bug 22093] Native Microsoft SQL Server Support
https://bugzilla.wikimedia.org/show_bug.cgi?id=22093 Sumana Harihareswara suma...@panix.com changed: What|Removed |Added Keywords||reviewed CC||suma...@panix.com --- Comment #29 from Sumana Harihareswara suma...@panix.com 2011-11-10 06:35:48 UTC --- Ryan Biesemeyer, thank you for your patch. Adding the reviewed keyword here because it looks like DJ and others have reviewed it and DJ will follow your approach in his work -- DJ, can you point to that so Ryan can follow and collaborate with you? -- Configure bugmail: https://bugzilla.wikimedia.org/userprefs.cgi?tab=email --- You are receiving this mail because: --- 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
[Bug 22093] Native Microsoft SQL Server Support
https://bugzilla.wikimedia.org/show_bug.cgi?id=22093 Brion Vibber br...@wikimedia.org changed: What|Removed |Added CC||dj.ba...@gmail.com --- Comment #26 from Brion Vibber br...@wikimedia.org 2011-10-31 22:33:57 UTC --- DJ, can you take a peek at this and see if the old patches here need keeping or are overruled by the other stuff you've been working on? -- Configure bugmail: https://bugzilla.wikimedia.org/userprefs.cgi?tab=email --- You are receiving this mail because: --- 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
[Bug 22093] Native Microsoft SQL Server Support
https://bugzilla.wikimedia.org/show_bug.cgi?id=22093 --- Comment #27 from D J Bauch dj.ba...@gmail.com 2011-10-31 23:16:14 UTC --- (In reply to comment #26) DJ, can you take a peek at this and see if the old patches here need keeping or are overruled by the other stuff you've been working on? Yes. I've been working with the PDO variation of the Microsoft native driver, but otherwise the goal is the same. I've had everything working up through version 1.16.5. Additionally, 1.17, 1.18, and 1.19alpha are all mostly working. Still some kind of problem with objectcache for some things I try to store and some issues with some non US English characters, but I will get that ironed out. My initial impression is that this patch makes heavier use of NVARCHAR vice VARCHAR than I have, and there are not a bunch of views -- so perhaps there is a better approach here to handling the conversion of date/time values that will be more in line with the way that you and Tim Starling have both taken up with me. Oh, and the new installer needs to be brute-forced into allowing additional databases other than the ones that are already there -- the _CompiledDBs setting is forgetful. Max has seen this problem demonstrated to him at the hackathon, so I think this will be addressed as well. -- Configure bugmail: https://bugzilla.wikimedia.org/userprefs.cgi?tab=email --- You are receiving this mail because: --- 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
[Bug 22093] Native Microsoft SQL Server Support
https://bugzilla.wikimedia.org/show_bug.cgi?id=22093 Max Semenik maxsem.w...@gmail.com changed: What|Removed |Added CC||maxsem.w...@gmail.com --- Comment #28 from Max Semenik maxsem.w...@gmail.com 2011-11-01 03:57:07 UTC --- (In reply to comment #27) Oh, and the new installer needs to be brute-forced into allowing additional databases other than the ones that are already there -- the _CompiledDBs setting is forgetful. Max has seen this problem demonstrated to him at the hackathon, so I think this will be addressed as well. Reverting r95023 locally on your machine seemed to fix this - just commit it with your other changes. -- Configure bugmail: https://bugzilla.wikimedia.org/userprefs.cgi?tab=email --- You are receiving this mail because: --- 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
[Bug 22093] Native Microsoft SQL Server Support
https://bugzilla.wikimedia.org/show_bug.cgi?id=22093 Chad H. innocentkil...@gmail.com changed: What|Removed |Added Version|1.15.1 |1.15.x -- Configure bugmail: https://bugzilla.wikimedia.org/userprefs.cgi?tab=email --- You are receiving this mail because: --- 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
[Bug 22093] Native Microsoft SQL Server Support
https://bugzilla.wikimedia.org/show_bug.cgi?id=22093 Eggertsen christ...@eggertsen.se changed: What|Removed |Added CC||christ...@eggertsen.se --- Comment #24 from Eggertsen christ...@eggertsen.se 2010-10-21 16:03:35 UTC --- Add myself to watch list. -- Configure bugmail: https://bugzilla.wikimedia.org/userprefs.cgi?tab=email --- You are receiving this mail because: --- 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
[Bug 22093] Native Microsoft SQL Server Support
https://bugzilla.wikimedia.org/show_bug.cgi?id=22093 --- Comment #23 from Aryeh Gregor simetrical+wikib...@gmail.com 2010-07-29 20:48:08 UTC --- (In reply to comment #14) RE: Aryeh Gregor why would any caller call open() with an empty $user ::open() is called when the class is instantiated. Is there anywhere I'm not finding that instantiates a database for any purpose without credentials? Wouldn't it be kind of pointless? The MySQL code doesn't seem to have this check. I don't see why it's needed. But it probably doesn't hurt anything. This LIMIT check is intentionally lax, since LimitToTopN() has a much more stringent parsing and stripping of the LIMIT/OFFSET clause and will return the SQL untouched if it doesn't match. Okay. On another note: Microsoft would like to see this mainlined (as would I :) ) At this point it seems like it should be okay to commit. The bar isn't so high for a new database driver, as long as it doesn't break anything else. It's easier to fix up the issues once it's committed anyway. Did you say you had commit access, or would you like someone else to commit it? and I realize that there will need to be some documentation on MediaWiki.org showing how to set up MS SQL Server, revealing the ntauth scenario etc., but don't want to cause confusion by people accidentally accessing these articles until it is released. How is documentation on future features typically handled? There are templates that say what version things are available from. Documentation is usually added well before releases, when it's added at all. -- Configure bugmail: https://bugzilla.wikimedia.org/userprefs.cgi?tab=email --- You are receiving this mail because: --- 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
[Bug 22093] Native Microsoft SQL Server Support
https://bugzilla.wikimedia.org/show_bug.cgi?id=22093 --- Comment #22 from Max Semenik maxsem.w...@gmail.com 2010-07-28 09:39:23 UTC --- (In reply to comment #21) I've gone through and reverted the whitespace changes caused by stylize.php Thanks, looks much clearer now. -- Configure bugmail: https://bugzilla.wikimedia.org/userprefs.cgi?tab=email --- You are receiving this mail because: --- 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
[Bug 22093] Native Microsoft SQL Server Support
https://bugzilla.wikimedia.org/show_bug.cgi?id=22093
Ryan Biesemeyer r...@yaauie.com changed:
What|Removed |Added
Attachment #7335|0 |1
is obsolete||
AssignedTo|wikibug...@lists.wikimedia. |r...@yaauie.com
|org |
--- Comment #14 from Ryan Biesemeyer r...@yaauie.com 2010-07-27 21:05:21 UTC
---
Created an attachment (id=7599)
-- (https://bugzilla.wikimedia.org/attachment.cgi?id=7599)
DatabaseMssqlNative
The suggestions have been implemented and the code has been tested on MS SQL
Server 2008 with Fulltext, as well as on MySQL 5.1 to ensure it doesn't break
anything. There are two small changes to DatabaseBase (suggested by Aryeh
Gregor), which cause ::deadlockLoop to call ::commit and ::rollback instead of
executing SQL directly.
The patch is based on today's r70030.
RE: Aryeh Gregor why would any caller call open() with an empty $user
::open() is called when the class is instantiated. Is there anywhere I'm not
finding that instantiates a database for any purpose without credentials?
RE: Aryeh Gregor This is evil, but I'm not sure there's a better solution.
+// 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 LIMIT check is intentionally lax, since LimitToTopN() has a much more
stringent parsing and stripping of the LIMIT/OFFSET clause and will return the
SQL untouched if it doesn't match.
RE: Aryeh Gregor Ouch. There's really no better way to do this? in
::insert()
Yes, there is absolutely a better way of doing this, and I believe I have
implemented it this time around, bu asking the database for the $table's
IDENTITY column and handling it appropriately. The old way was inherited and
worked, so I didn't mess with it until you brought it up, but yes, definitely
ugly.
On another note: Microsoft would like to see this mainlined (as would I :) ),
and I realize that there will need to be some documentation on MediaWiki.org
showing how to set up MS SQL Server, revealing the ntauth scenario etc., but
don't want to cause confusion by people accidentally accessing these articles
until it is released. How is documentation on future features typically
handled?
-yaauie
--
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
[Bug 22093] Native Microsoft SQL Server Support
https://bugzilla.wikimedia.org/show_bug.cgi?id=22093 --- Comment #15 from Max Semenik maxsem.w...@gmail.com 2010-07-27 21:12:49 UTC --- How is documentation on future features typically handled? We tag such pages, e.g. http://www.mediawiki.org/wiki/Manual:$wgAllowImageTag -- Configure bugmail: https://bugzilla.wikimedia.org/userprefs.cgi?tab=email --- You are receiving this mail because: --- 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
[Bug 22093] Native Microsoft SQL Server Support
https://bugzilla.wikimedia.org/show_bug.cgi?id=22093 --- Comment #16 from Reedy s...@reedyboy.net 2010-07-27 21:25:02 UTC --- You've also got a lot of whitespace changes in there, that make the diff a little cluttered.. As for mainline support.. As long as it's maintained, and it works, having it in core shouldn't be too much of an issue.. Though, the 3rd party drive might be a bit of an issue You will of course need to probably do some work for the new installer ;) -- Configure bugmail: https://bugzilla.wikimedia.org/userprefs.cgi?tab=email --- You are receiving this mail because: --- 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
[Bug 22093] Native Microsoft SQL Server Support
https://bugzilla.wikimedia.org/show_bug.cgi?id=22093
--- Comment #17 from Max Semenik maxsem.w...@gmail.com 2010-07-27 21:45:09
UTC ---
Note that we're going to ditch the current installer pretty soon, so it's worth
concentrating on new-installer instead to avoid a double effort.
-if( !defined( 'MEDIAWIKI_INSTALL' ) ) {
+if ( !defined( 'MEDIAWIKI_INSTALL' ) ) {
Your patch has lots of whitespace-only changes, this makes reviewing MUCH
harder. Committing changes of such scale combimed with reformatting is
completely out of the question.
-## DB2 specific:
+# # DB2 specific:
This is even weirder, did you use an automated source formatter? If so, it
should be fixed not to screw things up.
+else if ( $conf-DBtype == 'mssqlnative' ) {
+error_reporting( E_ALL );
Why do you clear E_STRICT here?
+# 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 ) {
1) Don't connect to master just to find out your DB type. DB_SLAVE is more than
sufficient.
2) Use getType() instead of instanceof
+$res = $this-doQuery( SELECT NAME AS idColumn FROM
SYS.IDENTITY_COLUMNS WHERE OBJECT_NAME(OBJECT_ID)='{$tableRaw}' );
Needs to be cached.
You don't need to reimplement stuff such as DatabaseBase::ping() and getLag().
--
Configure bugmail: https://bugzilla.wikimedia.org/userprefs.cgi?tab=email
--- You are receiving this mail because: ---
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
[Bug 22093] Native Microsoft SQL Server Support
https://bugzilla.wikimedia.org/show_bug.cgi?id=22093 --- Comment #18 from Ryan Biesemeyer r...@yaauie.com 2010-07-27 21:46:08 UTC --- RE: Whitespace Hm. It looks like those are artifacts of running it through stylize.php. I'll create a new diff. -- Configure bugmail: https://bugzilla.wikimedia.org/userprefs.cgi?tab=email --- You are receiving this mail because: --- 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
[Bug 22093] Native Microsoft SQL Server Support
https://bugzilla.wikimedia.org/show_bug.cgi?id=22093 --- Comment #19 from Bryan Tong Minh bryan.tongm...@gmail.com 2010-07-27 22:02:58 UTC --- I suggest that Ryan is given commit access, once all comments have been addressed. I would not want to have Yet Another Database Driver that is will be checked in and then be forgotten about and removed in some later release. -- Configure bugmail: https://bugzilla.wikimedia.org/userprefs.cgi?tab=email --- You are receiving this mail because: --- 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
[Bug 22093] Native Microsoft SQL Server Support
https://bugzilla.wikimedia.org/show_bug.cgi?id=22093 --- Comment #20 from Max Semenik maxsem.w...@gmail.com 2010-07-27 22:03:25 UTC --- (In reply to comment #17) This is even weirder, did you use an automated source formatter? If so, it should be fixed not to screw things up. stylize.php fixed in r70050. -- Configure bugmail: https://bugzilla.wikimedia.org/userprefs.cgi?tab=email --- You are receiving this mail because: --- 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
[Bug 22093] Native Microsoft SQL Server Support
https://bugzilla.wikimedia.org/show_bug.cgi?id=22093 Ryan Biesemeyer r...@yaauie.com changed: What|Removed |Added Attachment #7599|0 |1 is obsolete|| --- Comment #21 from Ryan Biesemeyer r...@yaauie.com 2010-07-27 23:45:58 UTC --- Created an attachment (id=7600) -- (https://bugzilla.wikimedia.org/attachment.cgi?id=7600) DatabaseMssqlNative I've gone through and reverted the whitespace changes caused by stylize.php -- Configure bugmail: https://bugzilla.wikimedia.org/userprefs.cgi?tab=email --- You are receiving this mail because: --- 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
[Bug 22093] Native Microsoft SQL Server Support
https://bugzilla.wikimedia.org/show_bug.cgi?id=22093 --- Comment #13 from Max Semenik maxsem.w...@gmail.com 2010-06-30 08:49:49 UTC --- Another issue: I haven't tried, but after reading MSDN I think that Article::incViewCount() wil not work on MSSQL, so this should be also eventually fixed. -- 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
[Bug 22093] Native Microsoft SQL Server Support
https://bugzilla.wikimedia.org/show_bug.cgi?id=22093
Roan Kattouw roan.katt...@gmail.com changed:
What|Removed |Added
CC||roan.katt...@gmail.com
--- Comment #11 from Roan Kattouw roan.katt...@gmail.com 2010-05-28 20:00:49
UTC ---
(In reply to comment #10)
+// 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).
Note that a number of QueryPage subclasses use EXTRACT(). In the
querypage-work2 branch, I've refactored this so it no longer uses EXTRACT(),
but the point is we have code using EXTRACT() in core right now.
--
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
[Bug 22093] Native Microsoft SQL Server Support
https://bugzilla.wikimedia.org/show_bug.cgi?id=22093 --- Comment #12 from Aryeh Gregor simetrical+wikib...@gmail.com 2010-05-28 20:02:51 UTC --- (In reply to comment #11) Note that a number of QueryPage subclasses use EXTRACT(). In the querypage-work2 branch, I've refactored this so it no longer uses EXTRACT(), but the point is we have code using EXTRACT() in core right now. Only two, AFAICT, Ancientpages and Unusedimages, and those are fixed in the patch. Are there more? I don't see them. -- 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
[Bug 22093] Native Microsoft SQL Server Support
https://bugzilla.wikimedia.org/show_bug.cgi?id=22093
--- Comment #10 from Aryeh Gregor simetrical+wikib...@gmail.com 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 occurredbr/\n .
+Query: $sqlbr/\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']
[Bug 22093] Native Microsoft SQL Server Support
https://bugzilla.wikimedia.org/show_bug.cgi?id=22093
--- Comment #9 from Max Semenik maxsem.w...@gmail.com 2010-04-27 06:04:10 UTC
---
Few comments on patch:
* Other Database*.php files list their main database classes first, you should
do it too.
$sql = str_replace(EXTRACT(epoch FROM ,
DATEDIFF(s,CONVERT(datetime,'1/1/1970'),, $sql);
This is unreliable, as it doesn't take into account possible variations in
spacing and casing. Use regex?
class result_mssqlnative {
This name doesn't follow our naming conventions.
wfDebug(SQL: [$sql]br/\n);
Debug log is in plaintext, tags make no sense here.
if ( preg_match( '/\bLIMIT\s*/i', $sql ) ) 0) {
Ideally, extensions not using limitResult() should be fixed instead.
$message = A database error has occurredbr/\n .
Query: $sqlbr/\n .
You're mixing tags with unescaped data - are you sure that there is no XSS or
tag breakage by overescaping?
if ((strpos( .$sql, SELECT) 0)
Use strict comparison (===) with FASLE instead, it will be more readable.
$bAllOk
We don't use Hungarian notation.
# Copyright (C) 2004 Brion Vibber br...@pobox.com
Not anymore :)
FROM $page,FREETEXTTABLE($searchindex , $match, LANGUAGE
'English') as ftindex .
Use $wgContLang to determine the correct language instead of assuming that it
is always English.
* Our coding conventions
(http://www.mediawiki.org/wiki/Manual:Coding_conventions) aren't strictly
followed, use stylize.php from trunk/tools/code-utils/ to at least fix spacing
problems.
--
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
[Bug 22093] Native Microsoft SQL Server Support
https://bugzilla.wikimedia.org/show_bug.cgi?id=22093 Ryan Biesemeyer r...@yaauie.com changed: What|Removed |Added Attachment #6957|0 |1 is obsolete|| --- Comment #8 from Ryan Biesemeyer r...@yaauie.com 2010-04-26 23:44:22 UTC --- Created an attachment (id=7335) -- (https://bugzilla.wikimedia.org/attachment.cgi?id=7335) DatabaseMssqlNative and supporting files I've taken the recommendations and implemented them as best I could against the current code base (r.65560). Some of the issues still exist, but the inline comments reflect why some portions are still not quite in line with the suggestions. A few notes: DatabaseMssqlNative::select() and DatabaseMssqlNative::selectSQLText() AryehGregor says that I shouldn't need to override either of these, and I agree that the old patch was horridly verbose and duplicated the logic several times. Instead of following the suggestion, which would have caused my patch to affect all of the Database* classes that are currently working, I took a cue from MaxSem's suggestion in b#23330 and re-wrote the methods from the original patch to inherit the logic from the parent and either adjust the setup beforehand or the output afterhand where appropriate. I think this helps isolate the change-set and uses inheritance much better than the patch did previously DatabaseMssqlNative::LimitToTopN() I agree that this should not be needed, and that its implementation was less than ideal, duplicating logic and doing a lot of extra work, but because we still can't depend on extensions to use the SQL-generation functions, I think this functionality is still needed as a last resort. I rewrote the function to parse a LIMIT clause, remove it, and then pass the resulting parameters to the existing DatabaseMssqlNative::limitResult() method, which I feel is a much cleaner and lighter-weight way of implementing a necessary evil. DatabaseMssqlNative::doQuery() - Epoch magic. I don't think this is the optimal solution, but since we can't rely on extensions to not use this, I'm leaving it in place. The two known caller functions have been updated to make the right calls. This patch is against TRUNK rev 65560 and for feedback only. I'll be spending the next few days going through and doing extensive bug-testing and will do my best to implement your suggestions before committing to trunk. I appreciate your feedback. -yaauie -- 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
[Bug 22093] Native Microsoft SQL Server Support
https://bugzilla.wikimedia.org/show_bug.cgi?id=22093 Reedy s...@reedyboy.net changed: What|Removed |Added CC||s...@reedyboy.net Blocks||9767 -- 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
[Bug 22093] Native Microsoft SQL Server Support
https://bugzilla.wikimedia.org/show_bug.cgi?id=22093
--- Comment #7 from Max Semenik maxsem.w...@gmail.com 2010-01-14 15:18:52
UTC ---
A couple comments on search:
+$namespaces = implode(',', $this-namespaces);
You should use DatabaseBase::makeList() for that.
+FROM $page,FREETEXTTABLE($searchindex , $match, LANGUAGE
'English') as ftindex .
This is pretty i10n-unfriendly. If MSSQL doesn't support universal Unicode
collation, you should use $wgContLang to determine the correct collation.
--
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
[Bug 22093] Native Microsoft SQL Server Support
https://bugzilla.wikimedia.org/show_bug.cgi?id=22093
Bryan Tong Minh bryan.tongm...@gmail.com changed:
What|Removed |Added
CC||bryan.tongm...@gmail.com
--- Comment #1 from Bryan Tong Minh bryan.tongm...@gmail.com 2010-01-13
21:33:23 UTC ---
I only took a quick look, but some general comments. Does it really need to
duplicate every single function from DatabaseBase? selectSQLText() looks
completely copied. I also think there may be unnecessary duplication between
the two mssql database classes. Things like SQL generation can be completely
shared, only things that call the underlying database interface ought to be
different.
What is exactly the advantage between using the two drivers?
Also some specific notes:
-...@ini_set( display_errors, true );
+...@ini_set( display_errors, false );
?
+ # 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';
+ }
This should be implemented as Database*::infinity()
- GROUP BY tl_namespace, tl_title
+ GROUP BY tl_title, tl_namespace
Why?
--
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
[Bug 22093] Native Microsoft SQL Server Support
https://bugzilla.wikimedia.org/show_bug.cgi?id=22093 --- Comment #2 from Chris Pucci a-cpu...@microsoft.com 2010-01-13 22:53:24 UTC --- Hi Bryan, Thanks for the comments. I've tried to address them below: Does it really need to duplicate every single function from DatabaseBase? selectSQLText() looks completely copied. Yeah, we definitely want to minimize or remove duplication if it exists. In the method you mentioned, there are actually some slight differences in regards to limits and offsets which is why it was extended in the mssqlnative.php database class. It is something to be aware of though and I will go back and ensure that we aren't needlessly duplicating existing code. I also think there may be unnecessary duplication between the two mssql database classes. Things like SQL generation can be completely shared... This is absolutely true with one example being the tables.sql file that is basically identicle for the mssql and the mssqlnative drivers. There were basically two reasons we chose to create new files for the native driver even if they were identicle instead of simply including or pointing to the existing mssql version. The first was to maintain the existing Mediawiki file structure in regards each individual driver and the associated files for said driver. The second reason was to maintain separate files in case future changes need to be made that may affect one driver and not the other. I'm open to changing that however if necessary though as part of our goal with submitting this patch is to develop in the light, so any community or project suggestions are appreciated. What is exactly the advantage between using the two drivers? There are actually quite a few differences and advantages to using the Native SQL Server driver. First, it is my understanding that community MSSQL driver is no longer officially maintained or supported whereas the Native driver is still in active development, is feature rich and is actively supported and maintained. There are also a few key features that the Native driver provides such as built-in UTF-8 support, (MSSQL driver requires a bridge for UTF-8), and MARS support which means it is compatible with SQL Azure which is a cloud based DB offering. There are other benefits as well and full information about the Native driver can be found here: http://www.microsoft.com/downloads/details.aspx?FamilyID=ccdf728b-1ea0-48a8-a84a-5052214caad9displaylang=en As for your other notes, I'll look into implementing the database::infinity solution. I've also updated the patch file and removed some unnecessary artifacts from testing/debugging. Regards, Chris -- 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
[Bug 22093] Native Microsoft SQL Server Support
https://bugzilla.wikimedia.org/show_bug.cgi?id=22093 Chris Pucci a-cpu...@microsoft.com changed: What|Removed |Added Attachment #6956 is|0 |1 obsolete|| --- Comment #3 from Chris Pucci a-cpu...@microsoft.com 2010-01-13 22:56:01 UTC --- Created an attachment (id=6957) -- (https://bugzilla.wikimedia.org/attachment.cgi?id=6957) Updated Native SQL Server Patch -- 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
[Bug 22093] Native Microsoft SQL Server Support
https://bugzilla.wikimedia.org/show_bug.cgi?id=22093 Chad H. innocentkil...@gmail.com changed: What|Removed |Added CC||innocentkil...@gmail.com --- Comment #4 from Chad H. innocentkil...@gmail.com 2010-01-13 23:35:18 UTC --- If the old driver has been discontinued, I would be in favor of dropping support for it in favor of this. IIRC, the current MSSQL class barely even works and has been practically abandoned for some time. Also, don't use @ for error suppression, use wfSuppressWarnings() and wfRestoreWarnings() before and after the error you wish to suppress. -- 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
[Bug 22093] Native Microsoft SQL Server Support
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.00';
}
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
+//
[Bug 22093] Native Microsoft SQL Server Support
https://bugzilla.wikimedia.org/show_bug.cgi?id=22093 --- Comment #6 from Chris Pucci a-cpu...@microsoft.com 2010-01-14 00:03:50 UTC --- Aryeh, Thanks for the comments and thorough review. I will work on your suggestions and update when complete. -- 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
