[2.12] PHP 8 compatibility #4347

derrabus · 2020-10-15T18:48:53Z

Q	A
Type	improvement
BC Break	kinda
Fixed issues	N/A

Summary

This PR makes DBAL 2.12 compatible with PHP 8.

The main problem with PHP 8 is that certain PDO classes have changed their method signatures in PHP 8. Since DBAL extends those classes, those changed signatures have to be applied to those child classes as well. That is of course a BC break if the calling code itself extends DBAL's classes.

With this PR, I suggest to move the affected methods into traits that exist in a php 7 and a php 8 version. This way, the BC break introduced with php 8 is handed down if and only if we're actually on php 8.

I am aware that DBAL 3.0 is supposed to become the php 8 compatible version. So why am I so eager to fix the 2.x series?

I'm really looking forward to 3.0 because it moves away from the tight coupling to PDO which makes perfect sense – especially if we look at the problem this PR aims to fix. But DBAL 3 has no stable release yet and ORM has not been migrated to DBAL 3 yet. PHP 8 on the other hand has just reached RC2 and plans the first stable release in six weeks. I really doubt that the ecosystem around Doctrine will be ready for php 8 if we make DBAL 3 a precondition for php 8.

Finally, I'd really like to test the applications I am working on with php 8 and this small change I'm proposing here unblocks me for the moment.

greg0ire

Nice! Please add some CI jobs to make sure the PHP 8 code is covered

derrabus · 2020-10-15T19:51:41Z

Do you already have CI jobs for php 8 for higher branches that I could backport?

greg0ire · 2020-10-15T19:53:01Z

Do you already have CI jobs for php 8 for higher branches that I could backport?

We have some, but for Travis, and as you may know I just merged #4310 . I still need to merge it into 3.0.x. Until then, no GA jobs to backport, sorry!

beberlei

This is a good idea, i was under the impression its impossible, tried my share of things but didn't come up with this workaroud :)

It would reduce a lot of the pressure to have PHP 8 support in DBAL 2.x already, we started on work for DBAL 3 in ORM 2.8, but its probably a long stretch to finish that in 6 weeks, including all the other parts of the ecosystem (fixtures, migrations, DoctrineBundle, Laminas module etc...)

As for the phpcs violations, a few have to be ignored here to disable our strict standard.

lib/Doctrine/DBAL/Driver/PDOConnectionTrait.php

lib/Doctrine/DBAL/Driver/PDOStatementTrait.php

lib/Doctrine/DBAL/Driver/PDOConnectionTrait.php

phpcs.xml.dist

derrabus · 2020-10-15T20:45:13Z

PHP CodeSniffer is happy now, but I need some help with silencing Psalm. I'm unfamiliar with the configuration of that tool. 😓

derrabus · 2020-10-15T21:15:51Z

CI jobs on php 8 with SQLite, MySQL and Postgres were successful. I've tried to enable DB2 and MSSQL as well, but apparently the corresponding php extensions are not ready yet.

composer.json

morozov

Is there a need to have an individual implementation for each version? As far as I understand, the underlying logic doesn't need to change, only the method signatures do. In this case, it should be possible to have the logic implemented in one trait and extended by two others which defined the expected signature.

lib/Doctrine/DBAL/Driver/PDOQueryImplementation.php

lib/Doctrine/DBAL/Driver/PDOStatementImplementations.php

.github/workflows/continuous-integration.yml

morozov · 2020-10-16T00:55:23Z

I see that PHP 8 has been added to some build jobs but not to all of them (oci8, pdo_oci, sqlsrv, pdo_sqlsrv, MariaDB are missing it). Is there a plan to update those? Testing with SQL Server will require bumping the extension version to 5.9.0-preview1.

lib/Doctrine/DBAL/Statement.php

morozov · 2020-10-16T01:17:42Z

I plan to release 2.11.2 tomorrow since it contains a couple of BC fixes. I anticipate resolving this PR will take longer than that. Let me know if you think I should hold off the release.

On the other hand, this change itself is significant enough and not a bug fix that it may deserve another unplanned minor release.

greg0ire · 2020-10-16T05:59:49Z

PHP CodeSniffer is happy now, but I need some help with silencing Psalm. I'm unfamiliar with the configuration of that tool. sweat

I pushed a commit showing how to ignore one of the errors. You can run vendor/bin/psalm to find out the type of each error (for some reason Github Actions does not show them)

derrabus · 2020-10-16T06:41:15Z

I see that PHP 8 has been added to some build jobs but not to all of them

As I wrote earlier, I tried to enable the SQL Server tests, but the extension did not compile properly. Same for DB2.

MariaDB shouldn't be a problem since the MySQL tests are working already. I've giving it a try. I'll look into Oracle as well.

Is there a plan to update those?

I wouldn't make this a blocker for this PR. As soon as the extensions are ready, they can be added to the CI.

greg0ire · 2020-10-16T07:44:18Z

I wouldn't make this a blocker for this PR.

Agreed, let's not make it a blocker, your code is already covered by the jobs you added.

derrabus · 2020-10-16T07:47:53Z

Sutting up the OCI8 extension did not work either. I don't know if this is actually a problem of the GitHub action that is used to setup PECL extensions or if the extensions themselves don't compile yet. But that's something that could be done in a follow-up PR.

But the good news is: MariaDB is tested with php 8 now. :-)

derrabus · 2020-10-16T07:52:09Z

On the other hand, this change itself is significant enough and not a bug fix that it may deserve another unplanned minor release.

I am unfamiliar with your versioning policy. Personally, I think releasing this as a 2.11 bugfix should be fine. But if you want to bump to 2.12 because of this change, I'm also cool with it.

morozov

Code-wise looks good now. A couple of notes are still remaining.

psalm.xml

morozov

Looks good 👍 @beberlei has to give his approval as well since he requested some changes initially.

driesvints · 2020-10-19T19:03:13Z

Thanks @derrabus!

greg0ire · 2020-10-19T19:03:14Z

Thanks a lot @derrabus ! Awesome work!

greg0ire · 2020-10-19T19:05:09Z

@morozov let's release 2.12.0 ?

morozov · 2020-10-19T19:05:17Z

Before we release 2.12.0 and declare the PHP 8 support, we need to make sure that all extensions currently tested on CI and compatible with PHP 8 are tested with PHP 8 as well (which should be all except for ibm_db2).

greg0ire · 2020-10-19T19:09:44Z

I will make a PR so that everyone can see the CI failures.

martinssipenko · 2020-10-19T19:10:45Z

Please leave a link to that PR here.

greg0ire · 2020-10-19T19:17:39Z

Here you go: #4361

derrabus · 2020-10-19T20:48:28Z

Thank you very much everyone. Having a PHP 8 compatible DBAL is great news. 🎉

This PR was merged into the 3.4 branch. Discussion ---------- Enable Doctrine tests on php 8 | Q | A | ------------- | --- | Branch? | 3.4 | Bug fix? | no | New feature? | no | Deprecations? | no | Tickets | #36872 | License | MIT | Doc PR | N/A Following doctrine/dbal#4347, this PR enables all Doctrine tests on php 8. Commits ------- f4a99b2 Don't skip Doctrine tests on php 8.

@BenMorel

Release [2.12.0](https://github.com/doctrine/dbal/milestone/82) 2.12.0 ====== - Total issues resolved: **1** - Total pull requests resolved: **7** - Total contributors: **5** Documentation,Static Analysis ----------------------------- - [4376: Configuration should not be internal](doctrine#4376) thanks to @BenMorel CI -- - [4374: Reduce number of build jobs](doctrine#4374) thanks to @greg0ire - [4365: Fail on extension / tool installation failure](doctrine#4365) thanks to @greg0ire Bug,Static Analysis ------------------- - [4373: Psalm fails on release commits](doctrine#4373) thanks to @morozov Documentation,Error Handling ---------------------------- - [4362: Adds exception thrown by execute() method](doctrine#4362) thanks to @toby-griffiths CI,PHP ------ - [4361: Test all extensions with PHP8](doctrine#4361) thanks to @greg0ire PHP --- - [4347: &doctrine#91;2.12&doctrine#93; PHP 8 compatibility](doctrine#4347) thanks to @derrabus # gpg: Signature made Thu Oct 22 20:29:24 2020 # gpg: using DSA key 1BEDEE0A820BC30D858F9F0C2C3A645671828132 # gpg: Can't check signature: No public key

derrabus force-pushed the bugfix/php8-compat branch from f0daabc to dc3c262 Compare October 15, 2020 18:55

greg0ire reviewed Oct 15, 2020

View reviewed changes

greg0ire requested a review from morozov October 15, 2020 19:55

beberlei requested changes Oct 15, 2020

View reviewed changes

lib/Doctrine/DBAL/Driver/PDOConnectionTrait.php Outdated Show resolved Hide resolved

lib/Doctrine/DBAL/Driver/PDOConnectionTrait.php Outdated Show resolved Hide resolved

lib/Doctrine/DBAL/Driver/PDOStatementTrait.php Outdated Show resolved Hide resolved

greg0ire reviewed Oct 15, 2020

View reviewed changes

lib/Doctrine/DBAL/Driver/PDOConnectionTrait.php Outdated Show resolved Hide resolved

lib/Doctrine/DBAL/Driver/PDOConnectionTrait.php Outdated Show resolved Hide resolved

derrabus force-pushed the bugfix/php8-compat branch 4 times, most recently from e4d4343 to 9e1dfe3 Compare October 15, 2020 20:42

derrabus commented Oct 15, 2020

View reviewed changes

phpcs.xml.dist Show resolved Hide resolved

derrabus force-pushed the bugfix/php8-compat branch 2 times, most recently from 7d943ff to a1f8942 Compare October 15, 2020 21:03

mvorisek reviewed Oct 15, 2020

View reviewed changes

composer.json Show resolved Hide resolved

morozov reviewed Oct 16, 2020

View reviewed changes

lib/Doctrine/DBAL/Driver/PDOQueryImplementation.php Outdated Show resolved Hide resolved

lib/Doctrine/DBAL/Driver/PDOStatementImplementations.php Outdated Show resolved Hide resolved

lib/Doctrine/DBAL/Driver/PDOStatementImplementations.php Outdated Show resolved Hide resolved

morozov reviewed Oct 16, 2020

View reviewed changes

.github/workflows/continuous-integration.yml Outdated Show resolved Hide resolved

morozov reviewed Oct 16, 2020

View reviewed changes

lib/Doctrine/DBAL/Statement.php Show resolved Hide resolved

morozov added the PHP label Oct 16, 2020

derrabus force-pushed the bugfix/php8-compat branch 2 times, most recently from 1c9ad91 to d417c8f Compare October 16, 2020 07:41

morozov reviewed Oct 19, 2020

View reviewed changes

psalm.xml Outdated Show resolved Hide resolved

derrabus force-pushed the bugfix/php8-compat branch 2 times, most recently from bdaceb5 to 03600c2 Compare October 19, 2020 17:21

PHP 8 compatibility.

a3135e2

derrabus force-pushed the bugfix/php8-compat branch from 03600c2 to a3135e2 Compare October 19, 2020 17:41

morozov approved these changes Oct 19, 2020

View reviewed changes

beberlei approved these changes Oct 19, 2020

View reviewed changes

greg0ire merged commit b407f8b into doctrine:2.12.x Oct 19, 2020

derrabus deleted the bugfix/php8-compat branch October 19, 2020 20:47

derrabus mentioned this pull request Oct 19, 2020

Enable Doctrine tests on php 8 symfony/symfony#38636

Merged

github-actions bot mentioned this pull request Oct 22, 2020

Merge release 2.12.0 into 3.0.x #4379

Closed

nanasess mentioned this pull request Nov 2, 2020

EC-CUBE 4.1.0 Roadmap EC-CUBE/ec-cube#4603

Closed

This was referenced Nov 10, 2020

Update doctrine dependencies neos/flow-development-collection#2122

Closed

Support for PHP 8 neos/flow-development-collection#2233

Closed

TASK: Test against PHP 8 neos/flow-development-collection#2015

Merged

costasovo mentioned this pull request Dec 1, 2020

PDOStatement::fetchAll() expects (callable(): mixed)|int|string, array() given phpstan/phpstan#4150

Closed

kevinpapst mentioned this pull request Dec 17, 2020

Support for PHP 8 kimai/kimai#2158

Merged

10 tasks

amotl mentioned this pull request Feb 24, 2021

Add compatibility for PHP8 crate/crate-pdo#116

Merged

orlitzky mentioned this pull request Nov 23, 2021

PHP 8.x compatibility agendav/agendav#292

Open

github-actions bot locked as resolved and limited conversation to collaborators Jul 28, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[2.12] PHP 8 compatibility #4347

[2.12] PHP 8 compatibility #4347

derrabus commented Oct 15, 2020 •

edited

Loading

greg0ire left a comment

derrabus commented Oct 15, 2020

greg0ire commented Oct 15, 2020

beberlei left a comment

derrabus commented Oct 15, 2020

derrabus commented Oct 15, 2020

morozov left a comment

morozov commented Oct 16, 2020

morozov commented Oct 16, 2020 •

edited

Loading

greg0ire commented Oct 16, 2020

derrabus commented Oct 16, 2020

greg0ire commented Oct 16, 2020

derrabus commented Oct 16, 2020

derrabus commented Oct 16, 2020

morozov left a comment

morozov left a comment

driesvints commented Oct 19, 2020

greg0ire commented Oct 19, 2020

greg0ire commented Oct 19, 2020

morozov commented Oct 19, 2020 •

edited

Loading

greg0ire commented Oct 19, 2020

martinssipenko commented Oct 19, 2020

greg0ire commented Oct 19, 2020

derrabus commented Oct 19, 2020

[2.12] PHP 8 compatibility #4347

[2.12] PHP 8 compatibility #4347

Conversation

derrabus commented Oct 15, 2020 • edited Loading

Summary

greg0ire left a comment

Choose a reason for hiding this comment

derrabus commented Oct 15, 2020

greg0ire commented Oct 15, 2020

beberlei left a comment

Choose a reason for hiding this comment

derrabus commented Oct 15, 2020

derrabus commented Oct 15, 2020

morozov left a comment

Choose a reason for hiding this comment

morozov commented Oct 16, 2020

morozov commented Oct 16, 2020 • edited Loading

greg0ire commented Oct 16, 2020

derrabus commented Oct 16, 2020

greg0ire commented Oct 16, 2020

derrabus commented Oct 16, 2020

derrabus commented Oct 16, 2020

morozov left a comment

Choose a reason for hiding this comment

morozov left a comment

Choose a reason for hiding this comment

driesvints commented Oct 19, 2020

greg0ire commented Oct 19, 2020

greg0ire commented Oct 19, 2020

morozov commented Oct 19, 2020 • edited Loading

greg0ire commented Oct 19, 2020

martinssipenko commented Oct 19, 2020

greg0ire commented Oct 19, 2020

derrabus commented Oct 19, 2020

derrabus commented Oct 15, 2020 •

edited

Loading

morozov commented Oct 16, 2020 •

edited

Loading

morozov commented Oct 19, 2020 •

edited

Loading