Native SQL Server Support mssqlnative.php

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/branches/phpBB-3_0_0/phpBB/docs/coding-guidelines.html#code

Thanks again!
Nils

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

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.

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.

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

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

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.

mssqlnative.php has the wrong permissions set, it should not be executable.

http://github.com/evil3/phpbb3/commit/58bdd91d616c7c9117c22722bbd93f4c58d5bacf