Uploaded image for project: 'phpBB3'
  1. phpBB3
  2. PHPBB3-9039

Native SQL Server Support mssqlnative.php

    Details

    • Type: New Feature
    • Status: Closed
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: 3.0.x
    • Fix Version/s: 3.0.8-RC1
    • Labels:
      None
    • Environment:
      PHP Environment:
      Database:

      Description

      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/details.aspx?FamilyId=61BF87E0-D031-466B-B09A-6597C21A2E2A&displaylang=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.

      1. phpbb3-head-r10445-sqlsrv.patch
        83 kB
        cpucci
      2. phpbb3-head-r10460-sqlsrv.patch
        81 kB
        cpucci
      3. phpbb3-head-r10488-sqlsrv.patch
        32 kB
        cpucci
      4. phpbb3-head-r10488-sqlsrv-rev2.patch
        33 kB
        Nils Adermann
      5. sqlsrv-patch-modifications.diff
        12 kB
        Nils Adermann

        Issue Links

          Activity

          Hide
          naderman Nils Adermann added a comment -

          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

          Show
          naderman Nils Adermann added a comment - 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
          Hide
          cpucci cpucci added a comment -

          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

          Show
          cpucci cpucci added a comment - 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
          Hide
          cpucci cpucci added a comment -

          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.

          Show
          cpucci cpucci added a comment - 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.
          Hide
          naderman Nils Adermann added a comment -

          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.

          Show
          naderman Nils Adermann added a comment - 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.
          Hide
          cpucci cpucci added a comment -

          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

          Show
          cpucci cpucci added a comment - 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
          Hide
          cpucci cpucci added a comment -

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

          Show
          cpucci cpucci added a comment - Updated patch to remove duplicate schema file and reference existing mssql schema file when using the native driver.
          Hide
          naderman Nils Adermann added a comment -

          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.

          Show
          naderman Nils Adermann added a comment - 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.
          Hide
          igorw Igor Wiedler [X] (Inactive) added a comment -

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

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

          Show
          igorw Igor Wiedler [X] (Inactive) added a comment - mssqlnative.php has the wrong permissions set, it should not be executable. http://github.com/evil3/phpbb3/commit/58bdd91d616c7c9117c22722bbd93f4c58d5bacf

            People

            • Assignee:
              naderman Nils Adermann
              Reporter:
              cpucci cpucci
            • Votes:
              0 Vote for this issue
              Watchers:
              3 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development