-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Conversation
Moved to draft. Let's get the deprecation merged up first (#5561). |
4288aca
to
e17632d
Compare
c33a57c
to
7974767
Compare
* | ||
* @dataProvider queryConversionProvider | ||
*/ | ||
public function testStatementBindParameters(string $query, array $params, array $expected): void |
There was a problem hiding this comment.
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.
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
## BC BREAK: removed support using NULL as prepared statement parameter type. | |
## BC BREAK: removed support for using NULL as prepared statement parameter type. |
7974767
to
0eef042
Compare
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' | ||
); | ||
} |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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-levelbind*()
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.
There was a problem hiding this comment.
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()
?
Lines 1317 to 1320 in 0cc7adc
if (isset($types[$key])) { | |
$type = $types[$key]; | |
[$value, $bindingType] = $this->getBindingInfo($value, $type); | |
} else { |
There was a problem hiding this comment.
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.
0eef042
to
2beb082
Compare
See the following pull requests for reference: