Reintroduce support for PHP 7.1 and 7.2 #4386

beberlei · 2020-10-24T14:17:19Z

Q	A
Type	/improvement
BC Break	no

Summary

While working on the ORM support for PHP 8 (2.8 branch) realized i had to update PHP to 7.3 because DBAL requires it. Some investigation uncovered that this constraint is arbitrarily set, only minor syntax breaks in tests folder, not even in lib/src lead to errors. Straightforward to support with PHPUnit 7, 8 and 9 as well.

This PR makes DBAL 2.11 compatible with PHP 7.1 and 7.2 again in preparation for a future DBAL in the 2.x branch that includes the doctrine/deprecations component.

Individual parts:

Downgrade some code for PHP 7.1/7.2
Add support for PHP 7.1, 7.2, run tests only on SQLite for these two platforms without pcov because PHPUnit has no support.
Introduce LegacyAssertions and MockBuilderProxy to support running testsuite on phpunit 7 to 9

beberlei · 2020-10-24T14:23:30Z

Depends on doctrine/coding-standard#226 due to composer.json platform constraint to 7.1.3

morozov · 2020-10-24T15:10:23Z

This looks like an unnecessary burden in terms of CI and dependencies. 👎

beberlei · 2020-10-24T15:41:17Z

@morozov the testsuite runs a few minutes faster on Github than on Travis, there is surely enough space to add 2 jobs for 7.1, 7.2 with Sqlite. In terms of dependencies this PR doesn't change much, the coding-standard can be upped to 8.1.x again after the related PR is merged and released. Considering that 2.11 and 2.12 are stepping stones for migrating to 3 and don't get feature work done on them anymore this does not cause much problems.

morozov · 2020-10-24T15:43:30Z

Why only SQLite? If we support a PHP version, we support and need to test all DB drivers with this version because many of them are shipped with PHP.

Considering that 2.11 and 2.12 are stepping stones for migrating to 3

I don't get this point. In order to use DBAL 3, one has to use the corresponding PHP version. How does it help that the stepping-stone version supports an older PHP? It has to be upgraded anyway.

greg0ire · 2020-10-24T16:18:06Z

This PR makes DBAL 2.11 compatible with PHP 7.1 and 7.2 again in preparation for a future DBAL in the 2.x branch that includes the doctrine/deprecations component.

I don't understand… doctrine/deprecations can work on 7.3 and above, right?

Overall this looks like it will imply a lot of work for you in this PR, and for us when maintaining the repository because of the reasons you tweeted (we are needlessly constrained by some of our tools). Here is how I solved that issue in doctrine/sql-formatter in case you are interested: https://github.com/doctrine/sql-formatter/pull/55/files

beberlei · 2020-10-24T18:00:51Z

@morozov there is very little changes in database extension code between minor php versions. If you look at PDO Sqlite and PDO MySql for example, except cleanup of tests and build scripts really nothing changed between 2015 and 2019. Given that PHP upmerges bugfixes over quite long timeframes of 3 years this makes the extensions stable enough to risk a tiny blind spot.

beberlei · 2020-10-24T18:03:25Z

@greg0ire PHP is the most static dependency in applications, because the packages come from the operating system, wheras composer packages are much more flexible. If you are running based on Ubuntu LTSes or CentOS systems then usually your PHP code is quire current, but you are still working with an old PHP version. That means you can more easily upgrade your PHP dependencies than PHP itself.

With this change somebody on a LTS PHP 7.1 could upgrade to DBAL 2.11, and then prepare the migration to DBAL 3 APIs, regardless of their longer running PHP upgrade cycle.

morozov · 2020-10-24T18:27:08Z

What prevents the users of LTS distros from upgrading PHP first and then upgrading the DBAL? If the end goal is to get to the latest DBAL (otherwise, what is this all for?) then PHP has to be upgraded anyways.

beberlei · 2020-10-24T21:24:04Z

I don't see the argument how this is a lot of work for us, this PR took me 2 hours to work on, it will work as soon as the doctrine/coding-standard supports PHP 7.1. In addition it will also not take us a lot of work from here on, because there is no work to be done on the 2.11.x and 2.12.x branch anymore except bugfixing. This will not be affected by code running on PHP 7.3+ or 7.1+.

The argument for raising the versions has constantly been that it puts a burden on us maintainers, and that new features should be used because they make life easier. yet it only took me 2 hours to get it working on 7.1 even though the code is not being target anymore for this version since 2.9. Not a single new feature or syntax was used, and no change was necessary in lib/ folder. In between many hundreds of commits were made that weren't made with 7.1 in mind at all.

Meanwhile our users that are on older versions of PHP can't get the latest DBAL with potentially new fixes or features.

greg0ire · 2020-10-25T17:56:54Z

You will need to run composer update at some point because some of the jobs use composer install and cannot benefit from https://twitter.com/greg0ire/status/1320417776813678593

morozov · 2020-10-25T18:09:15Z

So again, the question still is:

What prevents the users of LTS distros from upgrading PHP first and then upgrading the DBAL?

So far, I'm absolutely not getting behind this.

beberlei · 2020-10-25T22:13:12Z

@morozov DBAL is the only Doctrine component that currently requires 7.3+, ORM is up with 7.2 - but that is also artificially set. Evrey other component requires only 7.1. The new MongoDB ODM requires 7.2, but i am not sure syntax wise how much they depend on it.

Only recent LTS versions of distributions came out with 7.3 support (CentOS 8, Debian Buster), Ubuntu 18.04 is still widespread and only has 7.1. Composer version stats show "only" 50% usage by 7.3 and 7.4 and 7.2, 7.1 still have almost 40%: https://blog.packagist.com/php-versions-stats-2020-1-edition/

We really don't have a lot of work supporting our users on those old PHP versions, I don't have any problems investing my time into it, i am not asking you for your time on this.

.appveyor.yml

morozov · 2020-10-27T19:45:15Z

composer.json

@@ -51,10 +51,7 @@
    },
    "bin": ["bin/doctrine-dbal"],
    "config": {
-        "sort-packages": true,
-        "platform": {
-            "php": "7.3.0"


W/o the platform fixed, all dependency updates will have to be made on the lowest supported version. Otherwise, there's a risk of locking the dependencies that are not compatible with it.

It might be better to unset the platform version on CI before the update but have it committed in the repo.

When supporting this range of versions, you need to "composer update" anyways to get to the right packages. So the solution is probably rather to remove composer.lock in this step. With this setting in, you cannot get the code running locally with 7.1 and 7.2

But without this setting, the one performing a dependency update will have to use PHP 7.1. Otherwise, they will learn that the new resolution doesn't work with PHP 7.1 only on CI. Is there a Composer CLI switch to pass config.platform.php at runtime?

This point is not relevant anymore since ce3421e composer.lock is not committed anymore and the Ci always fetches the latest packeges matching all constraints.

greg0ire · 2020-10-28T09:44:09Z

I think the usage statistics of doctrine/dbal shows a big part of our users are not quite ready for ^7.3 yet, so this might be the right move.

https://github.com/orgs/doctrine/teams/doctrinecore/discussions/21 might make the maintenance burden this move might cause lighter.

Seb33300 · 2020-10-30T05:05:52Z

What prevents the users of LTS distros from upgrading PHP first and then upgrading the DBAL?

You should know that developers cannot always choose the PHP version they want.
How many time did I see an old PHP version because the application is hosted on a shared server and a PHP upgrade will break others apps...

greg0ire

@beberlei @morozov , this is very related to https://github.com/orgs/doctrine/teams/doctrinecore/discussions/21, I would love your input on that.

.github/workflows/continuous-integration.yml

.github/workflows/static-analysis.yml

morozov · 2020-10-30T15:31:45Z

I would love your input on that.

I see that helps solve the PHP 7.1 compatibility problem which I'm not interested in solving. In the rest of the cases, the approach with using composer.lock is quite simple and stable. What other problems is this approach meant to solve?

greg0ire · 2020-10-30T17:10:16Z

What other problems is this approach meant to solve?

Upgrades of some tools that imply upgrades of some other tools because both tools share dependencies.
We mean to pin tools and tools only, because not doing so could imply failing builds. I don't think we mean to pin other libraries, since bugs in these libraries should rather be discovered by ourselves with a breaking build than by users.
Avoiding awkward constraints such as 4.0 || ^5.0 for tools that we run with the latest version of PHP but we still need to be able to install on low versions of PHP when running other, unrelated tools.

morozov · 2020-10-30T18:48:03Z

Upgrades of some tools that imply upgrades of some other tools because both tools share dependencies.

Is it a practical or theoretical issue?

We mean to pin tools and tools only, because not doing so could imply failing builds. I don't think we mean to pin other libraries, since bugs in these libraries should rather be discovered by ourselves with a breaking build than by users.

Agree.

Avoiding awkward constraints such as 4.0 || ^5.0 for tools that we run with the latest version of PHP but we still need to be able to install on low versions of PHP when running other, unrelated tools.

I believe this is only the case when our library supports the PHP version lower than the dependencies support. So it's only valid if we support the unsupported PHP versions.

greg0ire · 2020-10-31T11:23:27Z

Upgrades of some tools that imply upgrades of some other tools because both tools share dependencies.

It's very practical, I recall having issue with shared dependencies, in particular nikic/php-parser:

composer why nikic/php-parser         
phpunit/php-code-coverage  9.2.0   requires  nikic/php-parser (^4.8)                                      
sebastian/complexity       2.0.1   requires  nikic/php-parser (^4.7)                                      
sebastian/lines-of-code    1.0.1   requires  nikic/php-parser (^4.6)                                      
vimeo/psalm                3.17.2  requires  nikic/php-parser (4.3.* || 4.4.* || 4.5.* || 4.6.* || ^4.8)

I believe this is only the case when our library supports the PHP version lower than the dependencies support. So it's only valid if we support the unsupported PHP versions.

Agree, and in the whole Doctrine ecosystem, they are more supported than not ATM.

bmack · 2020-11-29T21:39:44Z

Hey everybody,

just wanted to jump in here. Just to get to a point why this would be SUPER important for me / us at TYPO3 CMS (typo3.org). We've been using and loving Doctrine DBAL since we introduced it back then in 2017. So here's our situation:

TYPO3 v10 has a PHP 7.2 min requirement (can also be installed via composer), and ships with Doctrine DBAL 2.10 (as this is the last version that supports PHP 7.2). Naturally we have to take HUGE steps to prepare OUR code base (that is TYPO3, and not even the thousands of TYPO3 plugins that are out there) to the deprecations and new code that was introduced in 2.11 and 2.12, in our TYPO3 v10.

TYPO3 v11 we decided to have PHP 7.4 as minimum requirement, allowing us to bump to Doctrine DBAL 3.0. This would technically be possible, but there are a lot of really really really hard changes for us (e.g. final classes, and I know why you did that) on things we extended previously, or the Statement / Result Interface changes, which are semantically really really good), so we will have to do a lot of work to even get Doctrine 3.0 running.

In any case, for us it's now not possible to even allow TYPO3 v10 with PHP8, because of Doctrine 2.12 (no PHP 7.2 support but PHP 8 support, but Doctrine 2.10 with no PHP 8 support).

I know it's a burden for you folks, and I'm willing to help in any kind of ways - be it money or code (I've reached out to donations / sponsorships for the Doctrine project via email as suggested on the website several weeks ago with no answer), but I'd love to know if there are changes to make live easier for a lot of PHP folks:

Have PHP 7.2 + PHP 8 support in Doctrine DBAL 2.x branch
Add some helpers to ease the pain for migrations between Doctrine DBAL 2 + 3 in 3.x branch (if that is interesting) OR even support for PHP 7.2 in Doctrine DBAL 3.x so we could even jump to the latest stable even earlier.

I'd love to have as many people go with Doctrine DBAL 3.x as soon as possible, but given the current circumstances, looks like our project will have to stick with Doctrine 2.10 for a long time.

I don't expect anything from you, because you've done a tremendous work, and I highly appreciate it. Nonetheless, just wanted to raise my voice, as I'm happy to see such a PR / issue popping up.

simonberger · 2020-12-18T16:25:14Z

Anything we can do to get this out or is it not yet considered to get merged?

beberlei · 2020-12-18T16:27:18Z

We just need some time, it will be happening

beberlei · 2021-03-06T15:23:13Z

@morozov @greg0ire This PR is done from my POV now

As I haven't changed anything in lib/ and the composer "downgrades" constitute more a bugfix than a feature or improvement I personally would be fine to merge this into 2.12 as a bugfix instead of 2.13 to keep it simple, but releasing a new minor would be fine either. What do you think?

I cleaned a few things up from the previous state, since the 2.12.x branch already did a few things w.r.t to Composer and Github Workflow simpliciatios.

As soon as you have approved the PR and nothing is open from your side I will squash all the commits together.

morozov · 2021-03-06T16:33:56Z

lib/Doctrine/DBAL/Platforms/SQLServerPlatform.php

@@ -1691,12 +1690,11 @@ private function generateIdentifierName($identifier)
    protected function getCommentOnTableSQL(string $tableName, ?string $comment): string
    {
        return sprintf(
-            <<<'SQL'
+            "


Why is this change necessary?

HEREDOCs were added in PHP 7.3

Correction, moving the closing tag for now/heredocs to the same indentation as starting was added in 7.3

Won't moving the heredoc terminator to the beginning of the line have more or less the same effect?

It applies a tab more for every line, but works.
imho it's better to not indent heredoc blocks.

return sprintf( <<<SQL EXEC sys.sp_addextendedproperty @name=N'MS_Description', @value=N%s, @level0type=N'SCHEMA', @level0name=N'dbo', @level1type=N'TABLE', @level1name=N%s SQL, $this->quoteStringLiteral((string) $comment), $this->quoteStringLiteral($tableName) );

@beberlei you could use git restore --source=origin/2.12.x lib/Doctrine/DBAL/Platforms/SQLServerPlatform.php && git revert 9a8ea5e039268368d48f38c853a00840e8201bde to make it 7.1-compatible, that way syntactic coloration is kept.

morozov · 2021-03-06T16:36:34Z

composer.json

@@ -51,10 +51,7 @@
    },
    "bin": ["bin/doctrine-dbal"],
    "config": {
-        "sort-packages": true,
-        "platform": {
-            "php": "7.3.0"


But without this setting, the one performing a dependency update will have to use PHP 7.1. Otherwise, they will learn that the new resolution doesn't work with PHP 7.1 only on CI. Is there a Composer CLI switch to pass config.platform.php at runtime?

tests/Doctrine/Tests/DBAL/LegacyAssertions.php

morozov · 2021-03-06T16:59:32Z

[...] the composer "downgrades" constitute more a bugfix than a feature or improvement

This is by no means a bugfix. The support for the old PHP version was dropped deliberately.

I personally would be fine to merge this into 2.12 as a bugfix instead of 2.13 to keep it simple, but releasing a new minor would be fine either. What do you think?

I think I stop caring about 2.x at this point.

beberlei · 2021-03-06T21:39:18Z

Yes, we should do the 2.13 anyways because it should also include the Deprecations #4535 PR and thats going to be larger.

beberlei requested review from morozov and greg0ire October 24, 2020 14:17

jwage previously approved these changes Oct 24, 2020

View reviewed changes

greg0ire closed this Oct 25, 2020

greg0ire reopened this Oct 25, 2020

beberlei dismissed jwage’s stale review via 9023b11 October 25, 2020 20:09

beberlei force-pushed the DBAL-Support-PHP7.2 branch from 31dabf6 to 8d6a884 Compare October 25, 2020 21:53

beberlei marked this pull request as ready for review October 25, 2020 22:22

morozov reviewed Oct 27, 2020

View reviewed changes

.appveyor.yml Outdated Show resolved Hide resolved

morozov reviewed Oct 27, 2020

View reviewed changes

greg0ire reviewed Oct 30, 2020

View reviewed changes

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

.github/workflows/static-analysis.yml Outdated Show resolved Hide resolved

greg0ire mentioned this pull request Oct 31, 2020

Strong type $sql param in logger interfaces #4394

Closed

beberlei mentioned this pull request Nov 15, 2020

PHP8 Support doctrine/orm#8303

Merged

4 tasks

greg0ire mentioned this pull request Nov 16, 2020

Fixes for compatibility with DBAL 3 doctrine/DoctrineBundle#1263

Merged

morozov mentioned this pull request Nov 16, 2020

Upgrade phpstan and psalm #4412

Merged

morozov changed the base branch from 2.11.x to 2.12.x November 29, 2020 21:38

greg0ire mentioned this pull request Dec 30, 2020

Drop support for PHP 7.1 doctrine/persistence#152

Closed

beberlei mentioned this pull request Mar 6, 2021

[GH-8327] Deprecate EntityManagerHelper for a provider abstraction. doctrine/orm#8524

Merged

2 tasks

derrabus previously approved these changes Mar 6, 2021

View reviewed changes

morozov reviewed Mar 6, 2021

View reviewed changes

beberlei dismissed derrabus’s stale review via 4d4db9c March 7, 2021 18:52

beberlei changed the base branch from 2.12.x to 2.13.x March 8, 2021 08:50

beberlei added this to the 2.13.0 milestone Mar 8, 2021

Reintroduce Support for PHP 7.1 and 7.2

24625f0

beberlei force-pushed the DBAL-Support-PHP7.2 branch from 4d4db9c to 24625f0 Compare March 22, 2021 20:11

morozov approved these changes Mar 22, 2021

View reviewed changes

beberlei merged commit 6d7cd0d into doctrine:2.13.x Mar 27, 2021

beberlei deleted the DBAL-Support-PHP7.2 branch March 27, 2021 17:47

github-actions bot mentioned this pull request Mar 30, 2021

Merge release 2.13.0 into 3.0.x #4563

Merged

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

Reintroduce support for PHP 7.1 and 7.2 #4386

Reintroduce support for PHP 7.1 and 7.2 #4386

Conversation

beberlei commented Oct 24, 2020 • edited Loading

Summary

beberlei commented Oct 24, 2020

morozov commented Oct 24, 2020

beberlei commented Oct 24, 2020

morozov commented Oct 24, 2020 • edited Loading

greg0ire commented Oct 24, 2020

beberlei commented Oct 24, 2020

beberlei commented Oct 24, 2020

morozov commented Oct 24, 2020

beberlei commented Oct 24, 2020

greg0ire commented Oct 25, 2020

morozov commented Oct 25, 2020 • edited Loading

beberlei commented Oct 25, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

greg0ire commented Oct 28, 2020

Seb33300 commented Oct 30, 2020

greg0ire left a comment

Choose a reason for hiding this comment

morozov commented Oct 30, 2020

greg0ire commented Oct 30, 2020

morozov commented Oct 30, 2020

greg0ire commented Oct 31, 2020

bmack commented Nov 29, 2020

simonberger commented Dec 18, 2020

beberlei commented Dec 18, 2020

beberlei commented Mar 6, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

simonberger Mar 7, 2021 • edited Loading

Choose a reason for hiding this comment

greg0ire Mar 23, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

morozov commented Mar 6, 2021 • edited Loading

beberlei commented Mar 6, 2021

beberlei commented Oct 24, 2020 •

edited

Loading

morozov commented Oct 24, 2020 •

edited

Loading

morozov commented Oct 25, 2020 •

edited

Loading

simonberger Mar 7, 2021 •

edited

Loading

greg0ire Mar 23, 2021 •

edited

Loading

morozov commented Mar 6, 2021 •

edited

Loading