search

zendtech / IbmiToolkit

PHP frontend to XMLSERVICE for IBM i development.
BSD 3-Clause "New" or "Revised" License
46 stars 34 forks source link

False positive error message on disconnect #129

Closed alanseiden closed 3 years ago

alanseiden commented 3 years ago

Disconnecting a stateful connection causes the error message Error: SQLSTATE='' and Message: '', indicating that this PHP job's associated database job is no longer running. We verified this behavior with a PDO_ODBC connection.

Reason: a disconnect returns an empty $outputXML string. The error handling code in Toolkit.php, according to the comment, is testing for false (error condition, actually, tests for !$outputXml. The "!" symbol, however, also matches an empty string, such as that returned by the disconnect operation, thus flagging the empty string as an error. We recommend testing for false === $outputXml explicitly. In our test, this eliminates the error message.
https://github.com/zendtech/IbmiToolkit/blob/464bbddc3114e2f8485400220c5ec814ab3e5ddd/ToolkitApi/Toolkit.php#L909

Would an explicit check for false cause any problems? i.e. is an empty string or null value coming from execXMLStoredProcedure ever commonly an error condition?

alanseiden commented 3 years ago

Some friends came up with this alternative for the same file and line. This fix would eliminate the error for the disconnection case. It is a less general solution but would be very safe (only affects disconnects): image

What do you think?

chukShirley commented 3 years ago

So your first suggestion would be

if (false === $outputXml) {

?

alanseiden commented 3 years ago

@chukShirley Yes.

Gavdsc commented 3 years ago

Just an idea, but could you consider removing the 'die()' in that if statement for errors which gracefully fail?

alanseiden commented 3 years ago

Just an idea, but could you consider removing the 'die()' in that if statement for errors which gracefully fail?

Thank you for the suggestion. It would probably be better to throw an exception there or similar way to allow developers to catch and handle the error. An error there indicates something wrong with the database or other transport connection. It "shouldn't" happen, but it can, such as when CCSID is not set.

alanseiden commented 3 years ago

I am thinking we should use the if (false === $outputXml) and consider throwing an exception for true errors rather than the die().