Skip to content

Conversation

@remicollet
Copy link
Contributor

@remicollet remicollet commented Aug 1, 2016

Mostly a proof of concept (tests needed)

- move query text in header (defined only once)
- json for socket driver
- mysqlnd for mysqlnd driver
@remicollet remicollet mentioned this pull request Aug 1, 2016
@remicollet remicollet changed the title add support of mysqlnd driver (instead of using libmysqlclient) add support of mysqlnd driver (alternative to libmysqlclient) Aug 1, 2016
Copy link
Owner

@patrickallaert patrickallaert left a comment

Choose a reason for hiding this comment

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

I am very much in favor of supporting mysqlnd! I also think it should be the default as of APM 2.2 (new version), what is your opinion on that?

However, I think this should not be implemented as another driver, but rather defining macros that would abstract the mysql or mysqlnd API that would be used under the hood.

That way, it also prevents having both drivers at the same time, which is something I would prefer not to support.

Opinions?

@patrickallaert
Copy link
Owner

By supporting mysqlnd using macros around mysql API, this would dramatically reduce the number of lines of duplicated code.

@remicollet
Copy link
Contributor Author

remicollet commented Feb 14, 2017

I am very much in favor of supporting mysqlnd! I also think it should be the default as of APM 2.2 (new version), what is your opinion on that?

+1 (having dependency on libmysqlclient is not friendly, especially when PHP don't have it anymore)

That way, it also prevents having both drivers at the same time, which is something I would prefer not to support.

This is already not alllowed (if ... elseif ...)

By supporting mysqlnd using macros around mysql API, this would dramatically reduce the number of lines of duplicated code.

This was only a prrof of concept ;) btw this is about lot of conditional vs duplicated code.

I will try to see if a can update this PR (time...)

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants