Skip to content
This repository has been archived by the owner on Jan 29, 2020. It is now read-only.

Add mysqli connection flags to mysqli_real_connect #314

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

kris-sum
Copy link

The Mysqli driver doesn't allow setting of flags passed into mysqli_real_connect.

  • Are you fixing a bug?
    • Detail how the bug is invoked currently.

In some instances (like MySQL and PHP on Azure) MYSQLI_SSL_CLIENT needs to be set.

'params' => [
    'host' => 'xxxx.mysql.database.azure.com',
    'port' => 3306,
    'user' => 'xxxx@xxxxx',
    'password' => 'xxxxx',
    'dbname' => 'xxxx',
    'charset' => 'utf8',
    'use_ssl' => true,
    'ssl_ca' => 'xxxxx/certs/BaltimoreCyberTrustRoot.crt.pem',
    'driver_options' => [ ]
]
  • Detail the original, incorrect behavior.

This calls $this->resource->ssl_set($clientKey, $clientCert, $caCert, $caPath, $cipher); to configure the SSL options, but no flag is set on mysqli_real_connect. Connection to Azure subsequently fails with 'SSL connection is required'

  • Detail the new, expected behavior.

Driver should allow connecting as illustrated in azure documentation at https://docs.microsoft.com/en-us/azure/mysql/howto-configure-ssl .

Added 'flags' config key to driver_options to support the setting of flags into mysqli_real_connect. This lets users supply MYSQLI_CLIENT_SSL and other connection flags as indicated on http://www.php.net/manual/en/mysqli.real-connect.php

'params' => [
    'host' => 'xxxx.mysql.database.azure.com',
    'port' => 3306,
    'user' => 'xxxx@xxxxx',
    'password' => 'xxxxx',
    'dbname' => 'xxxx',
    'charset' => 'utf8',
    'use_ssl' => true,
    'ssl_ca' => 'xxxxx/certs/BaltimoreCyberTrustRoot.crt.pem',
    'driver_options' => [ 
        'flags' => MYSQLI_CLIENT_SSL 
    ]
]

@kris-sum
Copy link
Author

kris-sum commented May 8, 2018

Is anyone available to review this? @ezimuel @weierophinney ?

@michalbundyra
Copy link
Member

@kris-sum We need test case for this change.

@kris-sum
Copy link
Author

kris-sum commented May 8, 2018

@webimpress Any test case would require setting up SSL on the MySQL end - I can't see a way I can do that with the current integration test setup?

@michalbundyra
Copy link
Member

@kris-sum mock resource, pass it into the constructor and check if calls are with proper values/flags. Seems to be doable.

@ezimuel
Copy link
Contributor

ezimuel commented May 29, 2018

@kris-sum can you provide the unit test as suggested by @webimpress ? Thanks.

@weierophinney
Copy link
Member

This repository has been moved to laminas/laminas-db. If you feel that this patch is still relevant, please re-open against that repository, and reference this issue. To re-open, we suggest the following workflow:

  • Squash all commits in your branch (git rebase -i origin/{branch})
  • Make a note of all changed files (`git diff --name-only origin/{branch}...HEAD
  • Run the laminas/laminas-migration tool on the code.
  • Clone laminas/laminas-db to another directory.
  • Copy the files from the second bullet point to the clone of laminas/laminas-db.
  • In your clone of laminas/laminas-db, commit the files, push to your fork, and open the new PR.
    We will be providing tooling via laminas/laminas-migration soon to help automate the process.

@michalbundyra
Copy link
Member

This repository has been closed and moved to laminas/laminas-db; a new issue has been opened at laminas/laminas-db#53.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants