ZF-5918: connect errors are supressed when dbname is provided

Description

It seems a bit inconsistent that oci_connect() errors are suppressed in _connect() when dbname is provided in config.ini, but are not supressed when dbname isn't present.

It caused me a bit of head scratching during a recent connection issue.

Comments

In this case, Zend_Db_Adapter_Abstract will throw an exception with _checkRequiredOptions():


Zend_Db_Adapter_Exception("Configuration array must have a key for 'dbname' that names the database instance")

Since SVN5014, this part of code is unused.

sorry, I wasn't very clear in the description.

inside the _connect() function there is the following code:


        if (isset($this->_config['dbname'])) {
            $this->_connection = @oci_connect(
                $this->_config['username'],
                $this->_config['password'],
                $this->_config['dbname']);
        } else {
            $this->_connection = oci_connect(
                $this->_config['username'],
                $this->_config['password']);
        }

at the moment a check is made to see if dbname has been declared somewhere, and if it has oci_connect has it's errors suppressed (@). There is no error suppression when dbname has not been set.

This is an inconsistency, and caused me some issues while trying to diagnose a connection issue (The exception thrown contained no useful information)

I hope this is clearer.

You were clear, no problem ;).

Based on actual trunk, I think this code is unreachable. So we could replace:


if (isset($this->_config['dbname'])) {
    $this->_connection = @oci_connect(
        $this->_config['username'],
        $this->_config['password'],
        $this->_config['dbname']);
} else {
    $this->_connection = oci_connect(
        $this->_config['username'],
        $this->_config['password']);
}

by:


$this->_connection = @oci_connect(
    $this->_config['username'],
    $this->_config['password'],
    $this->_config['dbname']);

Because $this->_config['dbname'] is mandatory, if it misses, then you have an exception thrown by Zend_Db_Adapter_Abstract::_checkRequiredOptions() before entering in Zend_Db_Adapter_Oracle::_connect()

I see what your saying. It sounds like a good idea to remove code which will never be run.

But the problem which I was having (with the empty Exception messages) will still occur in your replacement code, because you still have the @ suppressing error messages.

If you look at the next code block in the file:


// check the connection
if (!$this->_connection) {
            /**
             * @see Zend_Db_Adapter_Oracle_Exception
             */
            require_once 'Zend/Db/Adapter/Oracle/Exception.php';
            throw new Zend_Db_Adapter_Oracle_Exception(oci_error());
        }

sorry, I hit "Add" too soon.

the call to oci_error() will never return anything while the oci_connect() errors are suppressed.

So my suggestion is to remove the @ before oci_connect.

{quote} the call to oci_error() will never return anything while the oci_connect() errors are suppressed. {quote}

For me, it's false, see these examples:


<?php
oci_connect('user', 'good_password', 'database');
print_r(oci_error());

=> return nothing


<?php
oci_connect('user', 'bad_password', 'database');
print_r(oci_error());

=>



Warning: oci_connect() [function.oci-connect]: ORA-01017: invalid username/password; logon denied in D:\web\www\test_oracle.php on line 2
Array ( [code] => 1017 [message] => ORA-01017: invalid username/password; logon denied [offset] => 0 [sqltext] => )


<?php
@oci_connect('user', 'bad_password', 'database');
print_r(oci_error());

=>


Array
(
    [code] => 1017
    [message] => ORA-01017: invalid username/password; logon denied
    [offset] => 0
    [sqltext] => 
)

The goal of @ is simply to suppress the warning but oci_error() is always populated.

No?

It looks like I have misunderstood the purpose of @

In which case the exception thrown needs to contain at least the code and message, something like:


// check the connection
if (!$this->_connection) {
            /**
             * @see Zend_Db_Adapter_Oracle_Exception
             */
            require_once 'Zend/Db/Adapter/Oracle/Exception.php';
            $errorMessageArray = oci_error();
            throw new Zend_Db_Adapter_Oracle_Exception($errorMessageArray[code] . ': ' . $errorMessageArray[message]);
        }

?

Open Zend/Db/Adapter/Oracle/Exception.php, you will see how this is managed

I see. That makes sense now.

So in my case the lack of an error message is down to oci_error not returning one, rather than the framework's processing.

Thanks for pointing me in the right direction.

The unreachable code was removed with SVN 14324 to close this issue.