Bug tracker

This ticket has been moved to our new tracker. Open Ticket PHPBB3-9039 now.

Native SQL Server Support mssqlnative.php (fix completed in vcs)

Currently phpBB only supports the community mssql driver for php.

Our team has created a phpBB 3.0.x driver package that uses the Native SQL Server driver for PHP which was recently released by Microsoft (currently in v1.1).

More information regarding the Native SQL Server driver for PHP can be found
here:

http://www.microsoft.com/downloads/deta ... laylang=en

I've attached the full patch file that adds Native SQL Server support to phpBB 3.0.x to this request and would love to work with the developers/maintainers in order to have this package officially supported.

Comments / History

Assigned ticket to group "Development Team"

Action performed by bantu (3.0 Release Manager) on Jan 25th 2010, 20:23

Posted by naderman (Development Team Leader) on Jan 25th 2010, 21:44

Hey, thank you for submitting this patch. Overall I'd say it looks like a really good starting point. I'm sure other developers will add a few more detailed comments soon. I'd just like to mention a few things I noticed on a first glance:

The schema file appears to be identical to the mssql_schema.sql file already present, there is no need to specify a seperate one in that case. Just reuse the existing one. You can do so by setting 'SCHEMA' to 'mssql' rather than 'mssqlnative' in includes/functions_install.php

phpBB does not use a numrows function of any sort because some DBMSs do not support it. So it seems rather odd that you buffer all results in a PHP array just to get the amount of rows even though the API does not have any way of using that information?

I have a few problems with the style/formatting of the code. The indentation seems to be a mix of spaces and tabs. The use of spaces within the code seems rather arbitrary. Some names correctly only consist of lower case characters and underscores, others mix the names with camel case or only use camel case. In order to commit this into the phpBB repository it will have to meet the guidelines explained in http://code.phpbb.com/svn/phpbb/branche ... .html#code

Thanks again!
Nils

Edited post #209625

Action performed by naderman (Development Team Leader) on Jan 25th 2010, 21:44

Posted by cpucci on Jan 26th 2010, 18:12

Thanks for the reply Nils,

In regards to the code duplication between mssql_schema.sql and mssqlnative_schema.sql, that was a purposeful decision. The idea that we had when addressing this issue is that since the MSSQL community php driver is no longer officially supported or maintained, we wanted to provide completely stand alone files for the SQL Server Native driver so that if the decision was made to remove any MSSQL components, our driver would not be affected. That of course is a community decision, but that was the purpose behind the decision to include a separate file rather than reference the existing file.

As for the code formatting, I will take another look and update as necessary.

Regards,

Chris

Edited post #209975

Action performed by cpucci on Jan 26th 2010, 18:13

Posted by cpucci on Jan 26th 2010, 20:51

Updated patch file:

- Removed a few unecessary methods
- General clean up
- Addressed code formating.

I think I got the includes/db/mssqlnative.php file correctly formated according to the phpBB docs, but fresh eyes may prove me wrong. :P

Edited post #210045

Action performed by cpucci on Jan 26th 2010, 20:54

Posted by naderman (Development Team Leader) on Feb 9th 2010, 15:57

Hey, I'm really sorry this is taking so long, too much on my plate right now. I've tried an install myself and it seemed to work so far, I'll do some more testing soon.

I would still really prefer to have just one schema file. If we ever decide to make changes/delete any of the other dbals we can still create two distinct ones. But I don't want to have two identical files in the package.

Posted by cpucci on Feb 9th 2010, 18:58

Thanks Naderman,

I can definitely understand where you're coming from for the schema file. I'll go ahead and put up a new patch that references the existing mssql schema file rather than duplicate.

-Chris

Posted by cpucci on Feb 10th 2010, 21:25

Updated patch to remove duplicate schema file and reference existing mssql schema file when using the native driver.

Linked ticket with changeset: r10490

Action performed by naderman (Development Team Leader) on Feb 11th 2010, 00:05

Changed ticket status from "New" to "Fix completed in SVN"

Action performed by naderman (Development Team Leader) on Feb 11th 2010, 00:11

Posted by naderman (Development Team Leader) on Feb 11th 2010, 00:11

Alright, I've commited the patch with a few last formatting changes to the 3_0_0 branch, this means it will be released with 3.0.8. It should certainly go through more testing before we release. But since we are in 3.0.7 RC phase right now, that would be too late and it should give us plenty of testing time.

I've attached the changes I have made compared to your last diff. I also fixed a typo and removed the hardcoded error output (print_r & die) from the connect method.

Linked ticket with changeset: r10489

Action performed by naderman (Development Team Leader) on Feb 11th 2010, 00:13

Linked ticket with changeset: r10548

Action performed by naderman (Development Team Leader) on Feb 27th 2010, 18:04

Ticket details

Related SVN changesets