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

PdoDriver: check for misconfigured PDO connections resource (target 3.3.x and 4.0.x) #294

Merged
merged 11 commits into from
Sep 17, 2018

Conversation

jkuchar
Copy link
Contributor

@jkuchar jkuchar commented Jun 11, 2018

  • bug fix
  • BC break? no (as original behaviour should be considered as broken)

see #292

@jkuchar jkuchar changed the title PdoDriver: check for misconfigured PDO connections resource PdoDriver: check for misconfigured PDO connections resource Jun 11, 2018
@jkuchar jkuchar changed the title PdoDriver: check for misconfigured PDO connections resource PdoDriver: check for misconfigured PDO connections resource (target 3.3.x and 4.0.x) Jun 11, 2018
@jkuchar jkuchar force-pushed the check-for-misiconfigured-pdo-resources branch from 838b540 to 303d46f Compare June 11, 2018 13:13
@dg dg force-pushed the master branch 4 times, most recently from 1ac208e to 9840c31 Compare June 22, 2018 09:47
@jkuchar
Copy link
Contributor Author

jkuchar commented Jul 6, 2018

I do not know where to add tests for this. I dot see any PdoDriver unit test. Should I make one?

@dg
Copy link
Owner

dg commented Jul 8, 2018

If you want, create a test :)

@jkuchar
Copy link
Contributor Author

jkuchar commented Jul 19, 2018

I do not feel need to test this, however coveralls compalains in this PR. :)

One is never sure enough.

@jkuchar
Copy link
Contributor Author

jkuchar commented Jul 19, 2018

Any elegant way how to create PDO connection from database.ini? Maybe create PdoConnection and use reflection to access connection object? (do not want to replicate dibi connection factory)

require __DIR__ . '/bootstrap.php';

if($config['driver'] !== 'pdo') {
\Tester\Environment::skip('Only relevant for PDO driver.');
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Wanted to use something like:

 /**
  * @dataProvider ../databases.ini *, pdo
  */

However it looks like tester does not support anything like that.

Copy link
Owner

Choose a reason for hiding this comment

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

Maybe use workaround like != foo, pdo

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good!


// PDO error mode: warning
Assert::exception(function() use ($config) {
($pdoConnection = new PDO($config['dsn']))->setAttribute(\PDO::ATTR_ERRMODE, \PDO::ERRMODE_EXCEPTION);
Copy link
Owner

Choose a reason for hiding this comment

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

It should be PDO::ERRMODE_WARNING

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated

@dg
Copy link
Owner

dg commented Jul 25, 2018

Great, thanks!

…rom PDO connection using Dibi internal factories)
->getProperty('connection');
$connectionProperty->setAccessible(TRUE);
$pdo = $connectionProperty->getValue($dibiDriver);
\assert($pdo instanceof PDO);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have tried to rethink a little creation process of PDO connection. As it is quite complex I have delegated it back to \Dibi\Connection.

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