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

Already on GitHub? Sign in to your account

Enhancement: add support for $driverOptions #394

Merged
merged 9 commits into from Nov 8, 2013

Conversation

Projects
None yet
6 participants
Contributor

till commented Oct 28, 2013

  • introduced $_supportedDriverOptions to validate what is passed into the object
  • added setDrivrOptions() which sets the array and throws exceptions

Hopefully sucessfully building, and awaiting feedback. ;)

@till till Enhancement: add support for $driverOptions
 * introduced $_supportedDriverOptions to validate what is passed into the object
 * added `setDrivrOptions()` which sets the array and throws exceptions
da6892e

Hello,

thank you for creating this pull request. I have automatically opened an issue
on our Jira Bug Tracker for you. See the issue link:

http://www.doctrine-project.org/jira/browse/DBAL-643

We use Jira to track the state of pull requests and the versions they got
included in.

@staabm staabm and 1 other commented on an outdated diff Oct 28, 2013

lib/Doctrine/DBAL/Driver/Mysqli/MysqliConnection.php
+ {
+ $this->_driverOptions = $driverOptions;
+
+ static $exceptionMsg = "%s option '%s' with value '%s'";
+
+ foreach ($driverOptions as $option => $value) {
+
+ if (!in_array($option, $this->_supportedDriverOpions)) {
+ throw new MysqliException(
+ sprintf($exceptionMsg, 'Unsupported', $option, $value)
+ );
+ }
+
+ if (!mysqli_options($this->_conn, $option, $value)) {
+ throw new MysqliException(
+ sprintf($exceptionMsg, 'Failed to set', $option, $value)
@staabm

staabm Oct 28, 2013

any chance to get more details for this error (from mysql client or server side)?

@till

till Oct 28, 2013

Contributor

@staabm That's a good question, I just see true or false. It may or may not emit a warning, not entirely sure though if I should go through the process of trapping that and returning it? Do you know if there are other places in the code I can use for inspiration?

@staabm

staabm Oct 28, 2013

maybe http://php.net/mysqli_error will hold some more details in such a information?

@till

till Oct 28, 2013

Contributor

@staabm Still having trouble to get it to actually return false.

E.g.:

mysqli_options($connection, \MYSQLI_OPT_CONNECT_TIMEOUT, new \stdClass());

That emits a notice (unable to parse object into int), but still returns true.

Any thoughts?

@staabm

staabm Oct 28, 2013

never used this method, but sounds like a bug. maybe it only returns false in case the connection is not valid?

@till

till Oct 28, 2013

Contributor

Yeah, it does sound like a bug. I'll investigate this on another channel. :)

@beberlei beberlei commented on an outdated diff Oct 28, 2013

lib/Doctrine/DBAL/Driver/Mysqli/MysqliConnection.php
@@ -32,6 +32,20 @@ class MysqliConnection implements Connection
private $_conn;
/**
+ * @var array
+ */
+ private $_driverOptions;
+
+ private $_supportedDriverOpions = array(
@beberlei

beberlei Oct 28, 2013

Owner

typo here Opions

@beberlei beberlei commented on an outdated diff Oct 28, 2013

lib/Doctrine/DBAL/Driver/Mysqli/MysqliConnection.php
+ foreach ($driverOptions as $option => $value) {
+
+ if (!in_array($option, $this->_supportedDriverOpions)) {
+ throw new MysqliException(
+ sprintf($exceptionMsg, 'Unsupported', $option, $value)
+ );
+ }
+
+ if (!mysqli_options($this->_conn, $option, $value)) {
+ throw new MysqliException(
+ sprintf($exceptionMsg, 'Failed to set', $option, $value)
+ );
+ }
+ }
+
+ return $this;
@beberlei

beberlei Oct 28, 2013

Owner

We don't use fluent interfaces for configuration setters, please remove

@till till Update: incorporate feedback from @beberlei
 * fix typo
 * remove fluent interface
8010ed8

@stof stof and 1 other commented on an outdated diff Oct 28, 2013

lib/Doctrine/DBAL/Driver/Mysqli/MysqliConnection.php
@@ -32,6 +32,20 @@ class MysqliConnection implements Connection
private $_conn;
/**
+ * @var array
+ */
+ private $_driverOptions;
@stof

stof Oct 28, 2013

Member

Please remove the leading underscore from the property (the Doctrine CS have been changed at some point to stop using them, so all new code should avoid it)

@till

till Oct 28, 2013

Contributor

@stof About to push this too along with a bunch of improvements. Does Doctrine use psr2? Or what's the "standard"?

@stof

stof Oct 28, 2013

Member

We are using PSR-2 now for new code (plus some other rules coming from object calisthenics)

@till

till Oct 28, 2013

Contributor

@stof Is there a ruleset.xml I can use in the future?

@stof stof and 2 others commented on an outdated diff Oct 28, 2013

lib/Doctrine/DBAL/Driver/Mysqli/MysqliConnection.php
@@ -151,4 +167,34 @@ public function errorInfo()
{
return $this->_conn->error;
}
+
+ /**
+ * Apply the driver options to the connection.
+ *
+ * @param array $driverOptions
+ *
+ * @throws MysqliException When one of of the options is not supported.
+ * @throws MysqliException When applying doesn't work - e.g. due to incorrect value.
+ */
+ public function setDriverOptions(array $driverOptions = array())
@stof

stof Oct 28, 2013

Member

should it really be public ?

@till

till Oct 28, 2013

Contributor

@stof IMO, yes. Why wouldn't it be nice to set this later?

@stof

stof Oct 28, 2013

Member

because the driver options are immutable in the main Connection class. So allowing to change them here is inconsistent (and may cause some issues by having them out of sync with the main config)

@beberlei

beberlei Oct 28, 2013

Owner

i agree this should be private.

@till

till Oct 28, 2013

Contributor

@beberlei @stof

So e.g. in my test, I am doing $connection->setDriverOptions(array(...)); and then that's not possible? What's the advantage of now allowing people to re-configure this at runtime?

@stof

stof Oct 28, 2013

Member

@till first, users should never interact with this implementation directly. They will be using Doctrine\DBAL\Connection (or one of its subclasses to handle master/slave or sharding) and this MySQLi connection will be used inside it.
And second, it would mean that the options used by the connection would be out of sync with $mainConnection->getParams(), which is the getter to access the params used by the connection.
And third, if we start to make it mutable instead of immutable, it should be done for all implementations, and using a common interface, not only for the MySQLiConnection.

and I'm -1 on introducing mutability here.
On a side note, I think there is already too much mutable dependencies in the Doctrine object graph (for instance, the connection is mutable in the EntityManager, but if you actually replace it, you will break some parts of the ORM, and parts which are not broken are depending on the high-level EntityManager to implement the lower level API, thus breaking abstraction entirely)

@till

till Oct 28, 2013

Contributor

OK, that makes sense.

@stof stof and 2 others commented on an outdated diff Oct 28, 2013

lib/Doctrine/DBAL/Driver/Mysqli/MysqliConnection.php
@@ -151,4 +167,34 @@ public function errorInfo()
{
return $this->_conn->error;
}
+
+ /**
+ * Apply the driver options to the connection.
+ *
+ * @param array $driverOptions
+ *
+ * @throws MysqliException When one of of the options is not supported.
+ * @throws MysqliException When applying doesn't work - e.g. due to incorrect value.
+ */
+ public function setDriverOptions(array $driverOptions = array())
+ {
+ $this->_driverOptions = $driverOptions;
+
+ static $exceptionMsg = "%s option '%s' with value '%s'";
@stof

stof Oct 28, 2013

Member

I don't think you need to make it static

@till

till Oct 28, 2013

Contributor

@stof I think that's fine.

@beberlei

beberlei Oct 28, 2013

Owner

i never understand what that does, can we remove it? :)

@stof

stof Oct 28, 2013

Member

@beberlei it shares the same content for all calls of this method (for all instances). This can be interesting when you have a big array in it to save memory in a method called often. But here, it does not have any benefit:

  • the method will generally be called only once, so you loose all benefits
  • this string does not consume much memory so there is no clear win. Thus, it would be garbage collected at the end of the method anyway as you don't keep references to it elsewhere
  • making it static forces PHP to keep it in memory until the end of the process (like for static properties of a class) as it is shared between all calls, so it actually hurts the garbage collector instead of helping here.

This is a case where you are actually hurting the GC while trying to be smarter than PHP and failing.

@till

till Oct 28, 2013

Contributor

@stof @beberlei Addressed this too locally. It doesn't make any sense if this is private to begin with.

@stof stof and 1 other commented on an outdated diff Oct 28, 2013

lib/Doctrine/DBAL/Driver/Mysqli/MysqliConnection.php
@@ -32,6 +32,20 @@ class MysqliConnection implements Connection
private $_conn;
/**
+ * @var array
+ */
+ private $_driverOptions;
+
+ private $_supportedDriverOptions = array(
+ \MYSQLI_OPT_CONNECT_TIMEOUT,
+ \MYSQLI_OPT_LOCAL_INFILE,
+ \MYSQLI_INIT_COMMAND,
+ \MYSQLI_READ_DEFAULT_FILE,
+ \MYSQLI_READ_DEFAULT_GROUP,
+ //\MYSQLI_SERVER_PUBLIC_KEY,
@stof

stof Oct 28, 2013

Member

why is it commented ?

@till

till Oct 28, 2013

Contributor

@stof It's a PHP 5.5 feature.

@stof

stof Oct 28, 2013

Member

Well, we should allow using it for people using PHP 5.5 IMO.

And this array could be kept inside setDriverOptions, allowing it to be garbage collected as soon as the object is constructed instead of keeping it for the whole life of the connection while it is not used

@till

till Oct 28, 2013

Contributor

OK, doing that now.

Member

stof commented Oct 28, 2013

This is missing tests

@stof stof commented on an outdated diff Oct 28, 2013

lib/Doctrine/DBAL/Driver/Mysqli/MysqliConnection.php
@@ -151,4 +167,34 @@ public function errorInfo()
{
return $this->_conn->error;
}
+
+ /**
+ * Apply the driver options to the connection.
+ *
+ * @param array $driverOptions
+ *
+ * @throws MysqliException When one of of the options is not supported.
+ * @throws MysqliException When applying doesn't work - e.g. due to incorrect value.
+ */
+ public function setDriverOptions(array $driverOptions = array())
+ {
+ $this->_driverOptions = $driverOptions;
@stof

stof Oct 28, 2013

Member

you don't need this property as it is never used

Contributor

till commented Oct 28, 2013

@stof How would you like this tested? I am looking at the travis-ci setup for this code. It seems like you guys set username to travis (vs. root with an empty password). Do you know of an example?

Member

kimhemsoe commented Oct 28, 2013

@till Look at the functional tests, they can get a database connecion you can test on.

Contributor

till commented Oct 28, 2013

@kimhemsoe Cheers!

So I am gonna do

<?php
// tests/Doctrine/Tests/DBAL/Functional/Mysql/DriverTest.php
namespace Doctrine\Tests\DBAL\Functional\Mysqli;
class DriverTest extends \Doctrine\Tests\DbalFunctionalTestCase
{
}

👍 or 👎 ?

Member

kimhemsoe commented Oct 28, 2013

@till yeah.. i dont see how else you can test this.

till added some commits Oct 28, 2013

@till till Update: feedback from @stof and @beberlei
 * move supportedDriverOptions into setDriverOptions
 * make setDriverOptions private
 * support \MYSQLI_SERVER_PUBLIC_KEY on 5.5.0+
398dc4b
@till till Update: remove static (method is private anyway and will be called once) 2515da3
@till till Enhancement: restructured code - to loop 'earlier'
 * push error handling down to when it occurs
 * WIP, as I am unable to trigger false currently
ae97199
@till till Enhancement: small test case
 * test case to assert the connection timeout is set
 * test case to assert on exceptions thrown
   * wip, as it seems impossible to trigger 'false' from mysqli_options
b12db24
Contributor

till commented Oct 28, 2013

@beberlei A hint for the test suite? It seems like my test is executed with e.g. pgsql as well.

@stof stof and 1 other commented on an outdated diff Oct 28, 2013

...trine/Tests/DBAL/Functional/Mysqli/ConnectionTest.php
@@ -0,0 +1,54 @@
+<?php
+namespace Doctrine\Tests\DBAL\Functional\Mysqli;
+
+require_once __DIR__ . '/../../../TestInit.php';
@stof

stof Oct 28, 2013

Member

This is not needed as it is the phpunit bootstrap file

@till

till Oct 28, 2013

Contributor

@stof Other test cases had that. I removed it locally. Investigating the matrix thing, then I'll push.

@stof

stof Oct 28, 2013

Member

legacy test cases from before the cleanup of the PHPUnit configuration have it, indeed (the project was not using the XML config file for PHPUnit at first but the legacy AllTests.php files)

Member

stof commented Oct 28, 2013

@till If you want your test to run only in case the functional tests are using MySQLi, you need to mark it as skipped for others. If you don't mark it as skipped, it will run

Contributor

till commented Oct 28, 2013

@stof Is there something from $ENV I can grab? Haven't really used this matrix thing on travis-ci.

Contributor

till commented Oct 28, 2013

@stof @beberlei Any more feedback how I can unbreak the pgsql tests? (I have no idea how I broke them in the first place.)

Otherwise, I think I've included/everything that was brought up in the comments.

@beberlei beberlei added a commit that referenced this pull request Nov 8, 2013

@beberlei beberlei Merge pull request #394 from easybiblabs/topics/mysqli-driver-params
Enhancement: add support for $driverOptions
a57e05a

@beberlei beberlei merged commit a57e05a into doctrine:master Nov 8, 2013

1 check failed

default The Travis CI build failed
Details
Contributor

till commented Nov 8, 2013

👯

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