-
-
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
Backport PDO-related changes from master to 3.0.x #3803
Conversation
… avoid having to replicate the \PDOStatement interface in ResultStatement
…ce to composition
Use a wrapped PDO Statement instead of a custom class for `pdo_sqlsrv` since `PDOSqlsrv\Statement` doesn't extend `PDOStatement` anymore.
@doctrine/doctrinecore what do you think about branching /cc @nikic, @nicolas-grekas, @Nyholm. |
I think that's a good direction. W should ensure that all possible deprecation-free are reported in 2.10. And I'm not sure about return types for v3. They might be for v4, see how we're doing on the topic on Symfony or on the FIG |
Yeah. Dragging them (as well as other scalar/strict type changes) into v3 would be too much. I do want to remove the deprecated APIs though but not necessarily all of them. If something appears to require non-trivial handling on the consumer's side, we can agree to postpone some of them by v4. |
|
||
## BC BREAK: the PDO symbols are no longer part of the DBAL API | ||
|
||
1. The support of `PDO::PARAM_*`, `PDO::FETCH_*`, `PDO::CASE_*` and `PDO::PARAM_INPUT_OUTPUT` constants in the DBAL API is removed. |
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.
Can we word this differently? First you could mention that ParameterType
is the alternative, second when users continnue using PDO
constants, then they will still work, because we use the same values.
This is something we could a.) document on ParameterType to be true b.) add a test for.
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.
Can we word this differently? First you could mention that ParameterType is the alternative […]
Will do. Should we do it here or we can file a separate ticket? I'm not sure how detailed it should be described in the upgrade notes. A blog post or a migration guide looks like a more proper place for such information.
This is something we could a.) document on ParameterType to be true b.) add a test for.
I'd be careful here. The values of the DBAL constants are left as the current values of the PDO constants to avoid unnecessary disruption but it doesn't mean they have to be equal. The PDO constants are not supported which is clearly stated in this very sentence.
second when users continue using
PDO
constants, then they will still work, because we use the same values.
No, this is no longer supported. If users do so, they are on their own. If PHP decides to change the values of PDO constants or introduces a new constant whose value collides with the value of ours, we won't be able to rule it out.
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 looks good.
I tested the BC break potential with ORM 2.7 and it is extremely minimal. Only some of the manual mocks need to be changed in the tests, the actual lib code just keeps on working. Patch for testing: https://gist.github.com/beberlei/28f1bdd3da76d825d2f9832193beaa3d
This is why I advocate for using PHPUnit-managed mocks instead of the hard-coded ones. We didn't have to rework any of our own mocks as you can see from the patch. |
I'm going to merge this without the suggested changes to UPGRADE.md in order to facilitate testing against PHP 8 on Travis (#3814). |
Closes #3770.
Closes #3798.
This is a PoC that represents the minimal required changes to support PHP 8 by the DBAL 2 codebase.
TODO:
dbal:import
CLI command is removed since it relies onPDO::nextRowset()
.Doctrine\DBAL\Driver\PDOStatement
no longer extendsPDOStatement
.Doctrine\DBAL\Driver\PDOConnection
no longer extendsPDO
.pdo_sqlsrv
driver.