Issue #2261 - OCI8 Driver PHP 7 - Fix bindValue overwriting other params.

[pdgcvs]

#2261

[Ocramius]

Requires a test case scenario that validates the fix.

[deeky666]

please do not introduce a protected member here, unless completely necessary. Otherwise we will have to keep BC for that afterwards. Also please do not use the old underscore prefix naming here. private $bindings is fine.

[deeky666]

Delayed for now as I'd like to wait for the PHP guys to come up with a fix before we start working around here.

[nickvergessen]

Considered not a bug by PHP:

[2016-02-22 04:41 UTC] sixd@php.net
Looks like an expected change with PHP 7.

OCI8 needs the zval of the bound variable to be available when OCIExecute is called.
Try using pass by reference in your bindValue() function, or don't bind in a function.
This is similar to example #3 in http://php.net/manual/en/function.oci-bind-by-name.php

From the dev who analyzed this:

"Reason for this new behavior in PHP 7 is that, unlike in PHP 5.6, zval (_zend_value) in PHP 7.0 is no more have reference count field. Only its values field (zend_value) has reference count field.

Since we store the placeholder (i.e., zval variable) [not its content (zend_value)] in php_oci_bind
structure, it gets overwritten when oci_bind_by_name() is called with same variable and different content in it."

[nickvergessen]

Or to translate it, and give more insight why this PR works:
$value is a local variable, in php7 it's always the same local variable (reference). So you always pass the same variable placeholder to bindParam (by reference).
So at the end all placeholders point to the same variable.

By using the array, you always pass a different reference and therefor the values are not overwritten.
As a test you can use #1237

[Ocramius]

So basically what is happening is that the values are GC'd before we can ever use them.
How the heck are people supposed to use oci_bind_by_name() if this is not a bug? :-\

[arcostasi]

Ok, it's a bug! how can we try to solve in the doctrine?
Apparently don't have solution in oci8 with php7. :(

Actually, we solved the problem after applied the patch that use solution in the issue #2261

[soyuka]

So, I've tried this pull-request and I ran the test suite on an oracle-12 instance (https://hub.docker.com/r/sath89/oracle-12c/):

• There is an issue with blobs, this should store a reference to the lob directly from the bindValue function. I've fixed this and I'm willing to do another pull-request if needed.
• I've one failing test and I'm wondering if it's related or if it's a misconfiguration:

1) Doctrine\Tests\DBAL\Functional\Schema\OracleSchemaManagerTest::testListDatabases
Doctrine\DBAL\Exception\DriverException: An exception occurred while executing 'CREATE USER c##test_create_database IDENTIFIED BY oracle':

ORA-65094: invalid local user or role name

My test configuration is available in this gist.

Btw, without this patch none of the OCI8 tests will pass because bindValue (which is used by executeUpdate) is broken when there are more than one binding (using php7 + oci 2.1.0). With only one binding, the value is still available, can be misleading when debugging.
For my information, what would be a valid test-case for this to be merged?

[eisberg]

We expect look !!!

[lynx-coding]

If someone got a solution for this problem without manipulating the doctrine sources locally i would be interesseted in this solution.

[soyuka]

No solution on my end, this also adds issues regarding CLOB/BLOB to me.

We just need to determine whether it's a php bug or a feature, in which case we have to fix the OCI8Statement library.

[lynx-coding]

As written before, its NOT a bug by PHP, its a "new behavior in PHP 7"

[soyuka]

@Lynx91 I agree, just saying this because it's still labeled as "Delayed".

How the heck are people supposed to use oci_bind_by_name() if this is not a bug? :-\

He has a point, so I'm not sure everyone agrees to this behavior...

[lynx-coding]

Hm..it seems to be a difficult decision...hope, that there will be any solution in the near future...thx for your response

[mmucklo]

@soyuka Have you done another PR with your CLOB/BLOB fix?

If your PR works, maybe the solution is just to clone off your fork for awhile until the Doctrine team stops playing ostrich on this issue -- since apparently the PHP team is not considering it a bug but a feature (as has been repeatedly pointed out)...

[soyuka]

@mmucklo I need time to check this further, I have no clean fix yet.

However, you can check this commit on my fork. It's almost there, I remember there was a small issue on top of it (maybe unrelated).
Don't use my fork as is because I replaced this with a crap hotfix where I comment every lob conditions!

[Garfield-fr]

It's possible to merge this commit, because Statement on Oracle doesn't work on PHP7 :(

[soyuka]

Nope this one should be closed in favor of #2434

[deeky666]

Closing in favour of #2434.

[Ocramius]

Handled in #2434