Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

PHP 8.3 support added #293

Merged
merged 10 commits into from
Feb 1, 2024
Merged

PHP 8.3 support added #293

merged 10 commits into from
Feb 1, 2024

Conversation

glo71317
Copy link
Contributor

Q A
Documentation yes/no
Bugfix yes/no
BC Break yes/no
New Feature yes/no
RFC yes/no
QA yes/no

Description

@glo71317
Copy link
Contributor Author

Hi @Ocramius

Few tests are failing due to some certification issue
Error - [Microsoft][ODBC Driver 18 for SQL Server]SSL Provider: [error:0A000086:SSL routines::certificate verify failed:self-signed certificate]

It seems some issue with CI/CD Any plan to fix this issue OR anything can we do?

@Ocramius
Copy link
Member

Yep, the usual MSSQL mess.

@weierophinney can we perhaps drop testing MSSQL? It's such a massive waste of time and oxygen for other better RDBMS...

@glo17680
Copy link

glo17680 commented Oct 26, 2023

@Ocramius / @weierophinney is there any update on this. Can anyone please advise on the same ?

@Ocramius
Copy link
Member

Either we rip out MSSQL testing (and mark it as untested somewhere in the docs), or we wait for Microsoft to release MSSQL support for PHP 8.3, which usually happens few weeks/months after release.

@weierophinney
Copy link
Member

My feelings on the matter are:

  • Not testing against MS SQL Server means we don't support it
  • Not supporting it would be a BC break, requiring a new major
  • We've marked this component as security-only, so a new major feels like a break with that

As such, my vote is we wait until MS has released a version of the extension for 8.3. If tests still fail at that time, we can regroup and figure out what to do from there; my guess is everything will "just work" as it has the last several releases.

@glo71317 glo71317 force-pushed the php8.3_support branch 2 times, most recently from 365ceed to 9413f5f Compare November 14, 2023 07:39
@glo71317
Copy link
Contributor Author

@Ocramius @weierophinney Any update on this?

@Ocramius
Copy link
Member

No.

@driehle
Copy link

driehle commented Dec 5, 2023

This is a bit unfortunate, as this blocks laminas-log, which is quite a central component in many applications.

How about extracting the namespace Laminas\Db\Adapter\Driver\Sqlsrv into a separate package? This would, obviously, require a new major release.

@weierophinney
Copy link
Member

@driehle Considering this repo is security-only, we're not going to do a new major. It would likely make more sense to split the lamians-db related functionality in laminas-log into a new package.

I recognize folks are excited about the new PHP release but (a) it's been not even two weeks, and (b) not every organization is tracking PHP updates in order to release as soon as it drops. It's far better for us to wait and ensure that the component works as expected (which means running tests!) than it is to rush a release. Please be patient.

@glo71317
Copy link
Contributor Author

glo71317 commented Jan 3, 2024

@Ocramius Any update on this when we can expect laminas-db available with php 8.3 support

@Ocramius
Copy link
Member

Ocramius commented Jan 3, 2024

#293 (comment)

@rchrdkn
Copy link

rchrdkn commented Jan 4, 2024

Looks like this is the msphpsql issue that we're waiting on - microsoft/msphpsql#1477

Let's hope they decide to release soon 🤞🏻

@glo71317
Copy link
Contributor Author

glo71317 commented Feb 1, 2024

@Ocramius @weierophinney https://github.com/microsoft/msphpsql/releases/tag/v5.12.0 this released Can we prioritize this?

@glo71317
Copy link
Contributor Author

glo71317 commented Feb 1, 2024

I have fixed all the test failures and other deprecated dynamic properties issues, Only following error is there

Unable to connect laminasdb_test. Array

(
    [0] => Array
        (
            [0] => 08001
            [SQLSTATE] => 08001
            [1] => -1
            [code] => -1
            [2] => [Microsoft][ODBC Driver 18 for SQL Server]SSL Provider: [error:0A000086:SSL routines::certificate verify failed:self-signed certificate]
            [message] => [Microsoft][ODBC Driver 18 for SQL Server]SSL Provider: [error:0A000086:SSL routines::certificate verify failed:self-signed certificate]
        )
    [1] => Array
        (
            [0] => 08001
            [SQLSTATE] => 08001
            [1] => -1
            [code] => -1
            [2] => [Microsoft][ODBC Driver 18 for SQL Server]Client unable to establish connection. For solutions related to encryption errors, see https://go.microsoft.com/fwlink/?linkid=2226722
            [message] => [Microsoft][ODBC Driver 18 for SQL Server]Client unable to establish connection. For solutions related to encryption errors, see https://go.microsoft.com/fwlink/?linkid=2226722
        )
)

it seems it should be fixed from your end or suggest to us we can do anything for this.

@Ocramius
Copy link
Member

Ocramius commented Feb 1, 2024

SSounds like a new requirement by the MSSQL adapter? Can you try disabling SSL validation for MSSQL in the test suite?

@Xerkus
Copy link
Member

Xerkus commented Feb 1, 2024

This has to be done via connection options https://learn.microsoft.com/en-us/sql/connect/php/connection-options by setting TrustServerCertificate to 1

This is introduced by ODBC Driver 18

@glo71317
Copy link
Contributor Author

glo71317 commented Feb 1, 2024

SSounds like a new requirement by the MSSQL adapter? Can you try disabling SSL validation for MSSQL in the test suite?

No idea how to disable ssl validation for MSSQL in the suite. so any idea?

@driehle
Copy link

driehle commented Feb 1, 2024

[
'UID' => getenv('TESTS_LAMINAS_DB_ADAPTER_DRIVER_SQLSRV_USERNAME'),
'PWD' => getenv('TESTS_LAMINAS_DB_ADAPTER_DRIVER_SQLSRV_PASSWORD'),
'Database' => $database,
]

As Xerkus said, try adding 'TrustServerCertificate' => 1 there.

@glo71317
Copy link
Contributor Author

glo71317 commented Feb 1, 2024

[
'UID' => getenv('TESTS_LAMINAS_DB_ADAPTER_DRIVER_SQLSRV_USERNAME'),
'PWD' => getenv('TESTS_LAMINAS_DB_ADAPTER_DRIVER_SQLSRV_PASSWORD'),
'Database' => $database,
]

As Xerkus said, try adding 'TrustServerCertificate' => 1 there.

I have made changes - 2abd29d but still having same issue

@Xerkus
Copy link
Member

Xerkus commented Feb 1, 2024

There is more than one place where sqlsrv_connect() is used. You need to find which of those need the option

@glo71317
Copy link
Contributor Author

glo71317 commented Feb 1, 2024

There is more than one place where sqlsrv_connect() is used. You need to find which of those need the option

I have updated all the places and fixed that issues, now lot tests are failing, Can you check these need to fixed still having some issue?

@Xerkus
Copy link
Member

Xerkus commented Feb 1, 2024

There is still one certificate validation error:

 1) LaminasIntegrationTest\Db\Adapter\Platform\SqlServerTest::testQuoteValueWithSqlServer
PDOException: SQLSTATE[08001]: [Microsoft][ODBC Driver 18 for SQL Server]SSL Provider: [error:0A000086:SSL routines::certificate verify failed:self-signed certificate]

/github/workspace/test/integration/Adapter/Platform/SqlServerTest.php:54

@glo71317
Copy link
Contributor Author

glo71317 commented Feb 1, 2024

@Xerkus it seems tests are not running due to .laminas-ci/install-sqlsrv-extension-via-pecl.sh as you told to removed

@Xerkus
Copy link
Member

Xerkus commented Feb 1, 2024

@Xerkus it seems tests are not running due to .laminas-ci/install-sqlsrv-extension-via-pecl.sh as you told to removed

Yes, it is referenced here:

${WORKING_DIRECTORY}/.laminas-ci/install-sqlsrv-extension-via-pecl.sh "${PHP_VERSION}" || exit 1

Please also remove that line.

README.md Outdated Show resolved Hide resolved
@Xerkus
Copy link
Member

Xerkus commented Feb 1, 2024

I asked you to revert changes in adapter previously. The correct way is to pass connection options as parameters for Connection instance: new Connection([..., 'options' => ['TrustServerCertificate' => '1']]);

I looked at the tests code and best place seems to be here:

--- a/test/unit/Adapter/Driver/Sqlsrv/AbstractIntegrationTest.php
+++ b/test/unit/Adapter/Driver/Sqlsrv/AbstractIntegrationTest.php
@@ -38,6 +38,8 @@ abstract class AbstractIntegrationTest extends TestCase
             $this->variables[$name] = getenv($value);
         }
 
+        $this->variables['options'] = ['TrustServerCertificate' => '1'];
+
         if (! extension_loaded('sqlsrv')) {
             $this->fail('The phpunit group integration-sqlsrv was enabled, but the extension is not loaded.');
         }

@Xerkus
Copy link
Member

Xerkus commented Feb 1, 2024

Also you seem to have missed re-adding changes for some sqlsrv_connect() calls in tests.

@glo71317
Copy link
Contributor Author

glo71317 commented Feb 1, 2024

sqlsrv_connect

I don't think as i can see here i removed src/Adapter/Driver/Sqlsrv/Connection.php as u suggested

@Xerkus
Copy link
Member

Xerkus commented Feb 1, 2024

sqlsrv_connect

I don't think as i can see here i removed src/Adapter/Driver/Sqlsrv/Connection.php as u suggested

Nevermind, looking at a wrong commit.

@glo71317
Copy link
Contributor Author

glo71317 commented Feb 1, 2024

All issues resolved from my end now and pr is green now thank you so much for your quick support and review

@Xerkus
Copy link
Member

Xerkus commented Feb 1, 2024

You are missing signoff for your commits. See DCO check for details

@glo71317
Copy link
Contributor Author

glo71317 commented Feb 1, 2024

You are missing signoff for your commits. See DCO check for details

Shall i use following command
git rebase HEAD~8 --signoff
git push --force-with-lease origin php8.3_support

Signed-off-by: Rajesh Kumar <[email protected]>
Signed-off-by: Rajesh Kumar <[email protected]>
Signed-off-by: Rajesh Kumar <[email protected]>
Signed-off-by: Rajesh Kumar <[email protected]>
Signed-off-by: Rajesh Kumar <[email protected]>
Signed-off-by: Rajesh Kumar <[email protected]>
Signed-off-by: Rajesh Kumar <[email protected]>
Signed-off-by: Rajesh Kumar <[email protected]>
@Ocramius Ocramius added this to the 2.19.0 milestone Feb 1, 2024
@Xerkus Xerkus merged commit 58fd2df into laminas:2.19.x Feb 1, 2024
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

7 participants