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

Implement our own error handler for db2_prepare #273

Merged
merged 10 commits into from Nov 23, 2017

Conversation

Projects
None yet
7 participants
@akrabat
Member

akrabat commented Oct 13, 2017

db2_prepare() may issue a warning in addition to returning false.
If you have an error handler set (e.g. in the Expressive pipeline) then
the message created is "db2_prepare(): Statement Prepare Failed" which
is not helpful.

Therefore, we create our own error handler and trap this ourselves so
that we can throw an ErrorException with more information from
db2_stmt_errormsg().

I've left the previous handling as I don't know PHP extension code well enough to be 100% sure that db2_prepare() always generates a warning when it returns false.

@Ocramius

This comment has been minimized.

Show comment
Hide comment
@Ocramius

Ocramius Oct 13, 2017

Member

@akrabat you know exactly what I'm gonna ask you :-P

Member

Ocramius commented Oct 13, 2017

@akrabat you know exactly what I'm gonna ask you :-P

@akrabat

This comment has been minimized.

Show comment
Hide comment
@akrabat

akrabat Oct 13, 2017

Member

I can't run the integration tests against this client's IBM i DB2 and don't have my own, so unclear how to run them…

Member

akrabat commented Oct 13, 2017

I can't run the integration tests against this client's IBM i DB2 and don't have my own, so unclear how to run them…

@Ocramius

This comment has been minimized.

Show comment
Hide comment
@Ocramius

Ocramius Oct 13, 2017

Member

Yeah, I know the scenario very well, but we're also not merging anything without verification, sorry...

Member

Ocramius commented Oct 13, 2017

Yeah, I know the scenario very well, but we're also not merging anything without verification, sorry...

@akrabat

This comment has been minimized.

Show comment
Hide comment
@akrabat

akrabat Oct 13, 2017

Member

Travis error is:

PHP Parse error:  syntax error, unexpected ':', expecting ';' or '{' in /home/travis/build/zendframework/zend-db/vendor/doctrine/instantiator/src/Doctrine/Instantiator/Instantiator.php on line 95

Thoughts on how to fix?

Member

akrabat commented Oct 13, 2017

Travis error is:

PHP Parse error:  syntax error, unexpected ':', expecting ';' or '{' in /home/travis/build/zendframework/zend-db/vendor/doctrine/instantiator/src/Doctrine/Instantiator/Instantiator.php on line 95

Thoughts on how to fix?

@Ocramius

This comment has been minimized.

Show comment
Hide comment
@Ocramius

Ocramius Oct 13, 2017

Member

@akrabat I think the wrong composer.lock was committed to this repository

Member

Ocramius commented Oct 13, 2017

@akrabat I think the wrong composer.lock was committed to this repository

@webimpress

This comment has been minimized.

Show comment
Hide comment
@webimpress

webimpress Oct 13, 2017

Contributor

@Ocramius @akrabat tests on travis should works despite of committed composer.lock, maybe #259 will help

BTW currently we don't have composer.lock in zend-db at all

Contributor

webimpress commented Oct 13, 2017

@Ocramius @akrabat tests on travis should works despite of committed composer.lock, maybe #259 will help

BTW currently we don't have composer.lock in zend-db at all

@Ocramius

This comment has been minimized.

Show comment
Hide comment
@Ocramius

Ocramius Oct 13, 2017

Member

Hmm, not sure how this was installed during the composer update step

Member

Ocramius commented Oct 13, 2017

Hmm, not sure how this was installed during the composer update step

@webimpress

This comment has been minimized.

Show comment
Hide comment
@webimpress
Contributor

webimpress commented Oct 13, 2017

@webimpress

This comment has been minimized.

Show comment
Hide comment
@webimpress

webimpress Oct 13, 2017

Contributor

@Ocramius and maybe composer.lock is cached on travis?

Contributor

webimpress commented Oct 13, 2017

@Ocramius and maybe composer.lock is cached on travis?

akrabat added some commits Oct 13, 2017

Implement our own error handler for db2_prepare
db2_prepare() may issue a [warning][1] in addition to returning false.
If you have an error handler set (e.g. in the Expressive pipeline) then
the message created is "db2_prepare(): Statement Prepare Failed" which
is not helpful.

Therefore, we create our own error handler and trap this ourselves so
that we can throw an ErrorException with more information from
db2_stmt_errormsg().

[1]: https://github.com/php/pecl-database-ibm_db2/blob/df913feb4dff56366ac4656b0bb6b39200794bde/ibm_db2.c#L4400
@akrabat

This comment has been minimized.

Show comment
Hide comment
@akrabat

akrabat Oct 13, 2017

Member

@Ocramius: Added an integration test. I can't prove it works on every IBM DB2 installation out there, but it does on one specific IBMI I:

$ vendor/phpunit/phpunit/phpunit test/Adapter/Driver/IbmDb2/StatementIntegrationTest.php
PHPUnit 4.8.36 by Sebastian Bergmann and contributors.

.....

Time: 1.58 seconds, Memory: 4.00MB

OK (5 tests, 8 assertions)
Member

akrabat commented Oct 13, 2017

@Ocramius: Added an integration test. I can't prove it works on every IBM DB2 installation out there, but it does on one specific IBMI I:

$ vendor/phpunit/phpunit/phpunit test/Adapter/Driver/IbmDb2/StatementIntegrationTest.php
PHPUnit 4.8.36 by Sebastian Bergmann and contributors.

.....

Time: 1.58 seconds, Memory: 4.00MB

OK (5 tests, 8 assertions)
@akrabat

This comment has been minimized.

Show comment
Hide comment
@akrabat

akrabat Oct 13, 2017

Member

@samsonasik Addressed your comments.

Member

akrabat commented Oct 13, 2017

@samsonasik Addressed your comments.

@akrabat

This comment has been minimized.

Show comment
Hide comment
@akrabat

akrabat Oct 16, 2017

Member

@webimpress Dunno what I did wrong last time, but it works this time!

$ vendor/phpunit/phpunit/phpunit  test/Adapter/Driver/IbmDb2/StatementIntegrationTest.php
PHPUnit 4.8.36 by Sebastian Bergmann and contributors.

.....

Time: 1.71 seconds, Memory: 4.00MB
Member

akrabat commented Oct 16, 2017

@webimpress Dunno what I did wrong last time, but it works this time!

$ vendor/phpunit/phpunit/phpunit  test/Adapter/Driver/IbmDb2/StatementIntegrationTest.php
PHPUnit 4.8.36 by Sebastian Bergmann and contributors.

.....

Time: 1.71 seconds, Memory: 4.00MB
@mwillbanks

Looks like everything is addressed 👍

@mwillbanks

This comment has been minimized.

Show comment
Hide comment
@mwillbanks

mwillbanks Oct 20, 2017

Contributor

@akrabat can you see if you can solve the coveralls although?

Contributor

mwillbanks commented Oct 20, 2017

@akrabat can you see if you can solve the coveralls although?

@akrabat

This comment has been minimized.

Show comment
Hide comment
@akrabat

akrabat Oct 20, 2017

Member

can you see if you can solve the coveralls although?

@mwillbanks The coveralls problem is that I've added new code that is covered by an integration test rather than a unit test.

Member

akrabat commented Oct 20, 2017

can you see if you can solve the coveralls although?

@mwillbanks The coveralls problem is that I've added new code that is covered by an integration test rather than a unit test.

akrabat added some commits Oct 20, 2017

Catch ErrorException
That's what the error handler throws.
Unit test Adapter\Driver\IbmDb2\Statement::prepare()
Mock the relevant db2_xxx functions by placing them into the
Zend\Db\Adapter\Driver\IbmDb2 namespace so that they are preferred
over the actual ones in the idm_db2 extension.
@akrabat

This comment has been minimized.

Show comment
Hide comment
@akrabat

akrabat Oct 20, 2017

Member

@mwillbanks Thought some more and then wrote some unit tests.

Member

akrabat commented Oct 20, 2017

@mwillbanks Thought some more and then wrote some unit tests.

@ezimuel ezimuel merged commit 984e355 into zendframework:develop Nov 23, 2017

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.2%) to 47.234%
Details
@ezimuel

This comment has been minimized.

Show comment
Hide comment
@ezimuel

ezimuel Nov 23, 2017

Member

@akrabat thanks for your contribution!

Member

ezimuel commented Nov 23, 2017

@akrabat thanks for your contribution!

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