@fitzoh opened this Pull Request on March 21st 2018 Contributor

This might just handle the PDO adapter, does matomo/core/Tracker/Db/Mysqli.php also need to be modified?

refs https://github.com/matomo-org/matomo/pull/10866

@mattab commented on March 21st 2018 Member

Hi @Fitzoh
Thanks for the PR! Yes, Mysqli also needs to work with SSL as it makes it easier to document the feature when we have support across the board :+1:

@fitzoh commented on March 21st 2018 Contributor

I think that should do it

@fitzoh commented on March 21st 2018 Contributor

And just out of curiosity, is there a reason for having different connections for core and tracker?
Feels like there's a lot of copy paste between the two

@mattab commented on March 21st 2018 Member

Initially it was for performance reasons, but this is not a valid reason anymore, so now it is actually technical debt and ultimately we would like to refactor both to use the same code.

@diosmosis commented on March 22nd 2018 Member

hi @Fitzoh can you recreate this PR in a separate branch instead of using the 3.x-dev branch? I can't pull these changes down locally to test.

Also, I haven't tested (since I can't pull down the code), but I don't believe your changes to the MySQLi adapter will work. $mysqlOptions doesn't exist in the MySQLi , the array isn't used & PDO options don't have meaning to MySQLi. Can you fix this?

Also, I added another option for self-signed certs, ssl_no_verify, in https://github.com/matomo-org/matomo/pull/10866/commits/f2d5f49ef46f0ecef3a0e350ade56f04b2581d6e. Would you be able to replicate those changes here?

@fitzoh commented on March 22nd 2018 Contributor

Sure @diosmosis , might be able to do that tonight, probably some time this weekend (No idea what I'm doing in PHP land).

@diosmosis commented on March 22nd 2018 Member

No worries, happy to answer any questions (even over the weekend), the MySQLi changes might be a pain, took me a while to figure out some things while fixing up the other PR.

You'll want to replicate what's happening here: https://github.com/matomo-org/matomo/pull/10866/files
in the libs/Zend/Db/Adapter/Mysqli.php file based on the config options.

@fitzoh commented on March 24th 2018 Contributor

@diosmosis does that look about right?
Also, what exactly did you have in mind for branches?
Did you just want me to change the branch the PR is targeting?

@diosmosis commented on March 24th 2018 Member

@Fitzoh that looks pretty good, I’ll test it and let you know tomorrow.

Don’t worry about the branch, I should be able to pull down the changes without that, enjoy your weekend!

@diosmosis commented on March 24th 2018 Member

@Fitzoh :+1: for the changes, had to make a couple extra changes to get this to work locally, if you can apply them, then it should be good to go. Feel free to let me know if you're unable to make them (do to lack of time or something).

@fitzoh commented on March 24th 2018 Contributor

updated @diosmosis

@diosmosis commented on March 25th 2018 Member

Tests are failing since it's missing the commits from https://github.com/matomo-org/matomo/pull/10866, otherwise the PR looks good and works for me.

This Pull Request was closed on April 2nd 2018
Powered by GitHub Issue Mirror