Add $statement parameter to PDOConnection::query() #3770
Conversation
PHP 8 updates the signature to `PDO::query(string $statement)`, so change `PDOConnection` to be compatible with that (but also older versions). This doesn't seem relevant for version 3.x anymore (which does not extend PDO), but this should allow testing current Symfony and Laravel versions under PHP 8.
|
Thank you @nikic for the patch. The Given that the timelines of neither of Doctrine DBAL 3.0 and PHP 8.0 are clear, I'm not sure where exactly this patch should land. @doctrine/team-dbal, If we want to support PHP 8 in DBAL 2.x, we should create a new minor branch ( |
|
Additionally, it seems to be not the only issue that prevents testing DBAL 2.10 on PHP 8: Not sure how this one could be fixed to be compatible with both PHP 7 and PHP 8 without causing a BC break. On the contrary, the test results on the |
|
@morozov Thanks for pointing that out, I went ahead and reverted that particular change: php/php-src@4a5e17f I remember there's some trick to being compatible with both signatures, but in this particular case the change wasn't really intentional, just that nobody checked that it's compatible with earlier versions... |
|
This change is a BC-break for extending classes: https://3v4l.org/RQ20D That said, if 3.0 is too much of a BC-break for users that just want php 8, maybe we should release 3.0 or event part of it earlier? |
|
On a build from php/php-src@1d69bf1, this PR looks better: Although, the @greg0ire's concern still stands. |
|
Any ideas on how to move forward here? On the PHP side, we could avoid the compat-issue with Doctrine by removing the parameter, but that would break other code that (correctly) does specify the parameter. This method fell into a crack where the signature was not checked at all, so both variants are currently silently accepted and we need to pick one. |
|
A BC break for extending classes doesn't look like to much harm compared to the benefit of being able to use PHP 8. As far as I understand, after adding the optional parameter, the extending class be compatible with the older and newer DBAL 2.x versions. I'm personally not aware of any specific projects that would be impacted by this change. AFAIK @nikic has some setup for scanning most popular PHP packages for compatibility with the changes made to PHP. Would it be possible to do the same and find the classes that override In addition, we could have a twitter poll (link). |
|
Is there a plan for making classes |
|
There is a plan to make some classes |
|
Alternatively to moving towards |
|
@greg0ire I'd rather branch and later release |
|
Closed in favor of #3803. |
|
@nikic FWIW, the revert made in php/php-src@4a5e17f is no longer relevant for DBAL 3 since our classes will no longer extend the PDO's ones. Unless there are other widely used solutions that extend it, it might make sense to restore the variadic arguments. Even then it would be a good example of why developers should not tightly couple their code to the APIs they don't own in the form of class inheritance. |
Summary
PHP 8 updates the signature to
PDO::query(string $statement), so changePDOConnectionto be compatible with that (but also older versions).This doesn't seem relevant for version 3.x anymore (which does not extend PDO), but this should allow testing current Symfony and Laravel versions under PHP 8.