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

Adding mysqlAttributes to default connection #103

Closed
wants to merge 5 commits into from

Conversation

lacalan
Copy link

@lacalan lacalan commented Jul 21, 2015

Here is a fix that allows ssl_cert, ssl_ca and ssl_key parameters to be added to the mysql connection string. A developer could add additional parameters as needed.

This is pull request to issue #99


foreach ($mysqlAttributesArray as $value) {
if (isset($config[$value])) {
$configArray['environments']['default']['mysql_attr'.$value] = $config[$value];
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Concatenate is missing spaces e.g : 'mysql_attr' . $value
It is missing an underscore for this to work :

$configArray['environments']['default']['mysql_attr_' . $value] = $config[$value];

(See ref in phinx)

@HavokInspiration
Copy link
Member

Good job.

I however think this is not "general enough" and maybe too much "MySQL centric".
Phinx is able to support all "MYSQL_ATTR" constants, so why not allowing the rest of them to be configured ? (http://php.net/manual/en/ref.pdo-mysql.php#pdo-mysql.constants)

Phinx also support this kind of configuration with SqlServer using the "sqlsrv_attr_" prefix.

I was hoping we could find a more general way, implement it and be done with connection config argument missing (kind of like the flags argument Cake Connection config has)

Since I do not have not a better alternative to submit right now, I guess this can be merged after corrections to support SSL for MySQL for the time being.

@@ -92,10 +92,19 @@ public function getConfig()
'port' => isset($config['port']) ? $config['port'] : null,
'name' => $config['database'],
'charset' => isset($config['encoding']) ? $config['encoding'] : null,
'unix_socket' => isset($config['unix_socket']) ? $config['unix_socket'] : null,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should not be removed.

@HavokInspiration HavokInspiration modified the milestones: 1.2.1, 1.1.6 Aug 2, 2015
@lorenzo
Copy link
Member

lorenzo commented Aug 11, 2015

@HavokInspiration Can this be merged?

@HavokInspiration
Copy link
Member

@lorenzo Nope, there are errors that would make this not work.

@HavokInspiration
Copy link
Member

Closing in favor of #122

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

Successfully merging this pull request may close these issues.

None yet

3 participants