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

Fix breaks named parameters in Oracle #3738

Merged
merged 1 commit into from
Dec 21, 2019

Conversation

eisberg
Copy link
Contributor

@eisberg eisberg commented Nov 19, 2019

Q A
Type bug
BC Break yes

Fixes #3722
Closes #3779

Summary

Fix breaks named parameters in Oracle ( see issue #3722 )

@morozov
Copy link
Member

morozov commented Nov 20, 2019

@eisberg could you also add a test?

@eisberg eisberg force-pushed the hotfix/oci8_named_paramters branch 2 times, most recently from c7a6fd7 to cefd2dd Compare November 21, 2019 18:34
@eisberg eisberg force-pushed the hotfix/oci8_named_paramters branch from 1e6f514 to 8312053 Compare November 22, 2019 05:14
Copy link
Member

@morozov morozov left a comment

Choose a reason for hiding this comment

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

The testQueryConversion test does not cover all use cases because it calls the executeQuery method on the connection object. In this case, the named parameters are transformed into positional ones.

Thanks for bringing this up. We probably need to get rid of this behavior since, on the one hand, it doesn't make any sense for the drivers that support named parameters but don't support the positional ones. On the other hand, it's a driver-specific logic implemented on the wrapper level.

We'll try to take care of that in a different issue. Besides a few nitpicks, looks good.

lib/Doctrine/DBAL/Driver/OCI8/OCI8Statement.php Outdated Show resolved Hide resolved
$param = $this->_paramMap[$param];
if (is_int($param)) {
if (! isset($this->_paramMap[$param])) {
throw new OCI8Exception(sprintf('Could not find variable mapping with index %d, in the SQL statement', $param));
Copy link
Member

Choose a reason for hiding this comment

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

The idea behind throwing an exception is that in this case, it wouldn't have worked previously anyways, so we're just making the error more explicit. Need to double-check that before merging.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

any news?

Copy link
Member

@morozov morozov Dec 20, 2019

Choose a reason for hiding this comment

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

@eisberg I forgot about it. Thank you for the reminder.

This is a valid code example that works on 2.9 and a build from this PR:

$stmt = $conn->prepare('SELECT :p FROM DUAL');
$stmt->bindValue('p', 'foo');
$stmt->execute();
echo $stmt->fetchColumn(), PHP_EOL;
// foo

This is an example of invalid code that is not supposed to work:

$stmt = $conn->prepare('SELECT :p FROM DUAL');
$stmt->bindValue(1, 'foo');
$stmt->execute();
echo $stmt->fetchColumn(), PHP_EOL;

On 2.9 it produces:

Warning: oci_bind_by_name(): ORA-01036: illegal variable name/number in lib/Doctrine/DBAL/Driver/OCI8/OCI8Statement.php on line 287
Uncaught Doctrine\DBAL\Driver\OCI8\OCI8Exception: ORA-01008: not all variables bound

On a build from this patch it produces:

Uncaught Doctrine\DBAL\Driver\OCI8\OCI8Exception: Could not find variable mapping with index 1, in the SQL statement

Which is perfectly fine. DBAL prevents a knowingly invalid operation on the underlying driver.

/cc @beberlei (#3779).

@morozov morozov changed the base branch from master to 2.10 December 20, 2019 17:25
@morozov
Copy link
Member

morozov commented Dec 20, 2019

@eisberg please rebase your patch onto 2.10. It needs to go there since it's a bugfix. Later, the entire 2.10 branch will be merged into master.

@eisberg eisberg force-pushed the hotfix/oci8_named_paramters branch from 71b852d to 9f4d9a5 Compare December 21, 2019 07:53
@morozov morozov merged commit 400f604 into doctrine:2.10 Dec 21, 2019
@morozov
Copy link
Member

morozov commented Dec 21, 2019

Thank you @eisberg!

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 5, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants