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

Remove the APIs deprecated in DBAL 3.x #5560

Merged
merged 3 commits into from Aug 3, 2022

Conversation

@morozov morozov added this to the 4.0.0 milestone Aug 1, 2022
@morozov morozov marked this pull request as ready for review August 1, 2022 21:25
@morozov morozov marked this pull request as draft August 1, 2022 22:44
@morozov
Copy link
Member Author

morozov commented Aug 1, 2022

Moved to draft. Let's get the deprecation merged up first (#5561).

@morozov morozov force-pushed the parameter-type-non-nullable branch from 4288aca to e17632d Compare August 2, 2022 18:23
@morozov morozov changed the title Remove support for using NULL as prepared statement parameter type Remove the APIs deprecated in DBAL 3.x Aug 2, 2022
@morozov morozov force-pushed the parameter-type-non-nullable branch 3 times, most recently from c33a57c to 7974767 Compare August 2, 2022 19:30
*
* @dataProvider queryConversionProvider
*/
public function testStatementBindParameters(string $query, array $params, array $expected): void
Copy link
Member Author

@morozov morozov Aug 2, 2022

Choose a reason for hiding this comment

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

This test was implemented in #3738 and used to reuse the existing data provider of the testQueryConversion() method.

The data provider provides positional statement parameters as a list which is still supported by the "shortcut" methods like Connection::executeQuery() but is no longer supported by the Statement class being tested. At the same time, this test doesn't and didn't need to reuse the data provider which focuses on testing the SQL parser while the test itself is meant to cover binding positional parameters directly to the statement.

The test has been renamed, reworked, and moved to the end of the suite in order to not be located between the other test and its data provider.

@morozov morozov marked this pull request as ready for review August 2, 2022 19:44
UPGRADE.md Outdated

The `$type` parameter of the driver-level `Statement::bindParam()` and `::bindValue()` has been made required.

## BC BREAK: removed support using NULL as prepared statement parameter type.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
## BC BREAK: removed support using NULL as prepared statement parameter type.
## BC BREAK: removed support for using NULL as prepared statement parameter type.

@morozov morozov force-pushed the parameter-type-non-nullable branch from 7974767 to 0eef042 Compare August 2, 2022 20:07
Comment on lines -1321 to -1328
if (array_key_exists($key, $types)) {
Deprecation::trigger(
'doctrine/dbal',
'https://github.com/doctrine/dbal/pull/5550',
'Using NULL as prepared statement parameter type is deprecated.'
. 'Omit or use Parameter::STRING instead'
);
}
Copy link
Member

Choose a reason for hiding this comment

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

This basically means that the deprecation had no effect: It is still possible to pass null as type.

Copy link
Member Author

@morozov morozov Aug 2, 2022

Choose a reason for hiding this comment

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

This is not enforced at runtime because connection methods accept types as an array. If I misunderstand your point, could you illustrate it with a code snippet?

Passing NULL to the wrapper will likely result in getBindingInfo() returning NULL for $bindingType which in turn will cause an exception in the driver because the driver-level bind*() methods don't accept NULL.

Copy link
Member

Choose a reason for hiding this comment

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

This is not enforced at runtime because connection methods accept types as an array.

This is okay, but then it did not make sense that we triggered that deprecation. The caller is warned about a call that continues to work.

If I misunderstand your point, could you illustrate it with a code snippet?

If I understood the deprecation correctly, it is triggered on calls like this:

$connection->executeQuery(
    sql: $sql,
    params: ['myParam' => 'foo'],
    types: ['myParam' => null]
);

The change you are suggesting is to remove the deprecation trigger without introducing a breaking change. The call above remains intact.

Passing NULL to the wrapper will likely result in getBindingInfo() returning NULL for $bindingType which in turn will cause an exception in the driver because the driver-level bind*() methods don't accept NULL.

No, passing null has the same effect as omitting that particular parameter in the $types array: We fall back to string. This is literally what the line after this block does.

My suggesting would be to either:

  • Revert the deprecation on 3.4.x or
  • Throw if we encounter null in this array.

Copy link
Member Author

Choose a reason for hiding this comment

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

I see. You're right. Should we just replace theisset() in the below condition with array_key_exists()?

dbal/src/Connection.php

Lines 1317 to 1320 in 0cc7adc

if (isset($types[$key])) {
$type = $types[$key];
[$value, $bindingType] = $this->getBindingInfo($value, $type);
} else {

Copy link
Member

Choose a reason for hiding this comment

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

We could do that. That would trigger a TypeError on the call to getBindingInfo() because you've removed null from the union there.

@morozov morozov force-pushed the parameter-type-non-nullable branch from 0eef042 to 2beb082 Compare August 2, 2022 23:47
@morozov morozov merged commit 60f014a into doctrine:4.0.x Aug 3, 2022
@morozov morozov deleted the parameter-type-non-nullable branch August 3, 2022 05:27
@morozov morozov linked an issue Aug 4, 2022 that may be closed by this pull request
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 4, 2023
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.

Inconsistent parameter indexes passed to the logging middleware
3 participants