Problem/Motivation

Today, whenever a patch includes an updated composer.lock file, the dependencies in that lock file are calculated using whatever PHP version the patch submitter happens to be using. This means that the generated dependency graph might be different depending on who generated the patch file. Worse than that, some results might actually be erroneous. Erroneous results might result in a failure from the composer update command, or, worse still, might produce dependencies that do not work with the advertised minimum version of Drupal. Some of these erroneous results would not be caught by the tests.

One example where this used to go wrong in Drupal 9.0.x related to the minimum PHP version in the drupal/drupal composer.json file. At the time, PHP 7.2.3 is required; however, if you installed this version of PHP and run composer update, you got an error:

  Problem 1
    - symfony/mime v5.0.0 requires php ^7.2.5 -> your PHP version (7.2.3) does not satisfy that requirement.

In addition to symfony/mime, there were other packages with minimum PHP versions that are higher than when 7.2.3 was originally selected as the minimum for Drupal 9.x. For example, symfony/service-contracts v2.0.0 requires php ^7.2.9.

Proposed resolution

  • Set the minimum version of PHP explicitly to 7.3.0, so that the version that we declare as our minimum matches what our dependencies require.
  • Set config.platform.php in the drupal/drupal root-level composer.json file to match the minimum PHP version in core/composer.json, so that dependency resolution is always done consistently, regardless of the version of PHP installed on the machine running the composer update command.
  • Add an integration test that ensures that the version of PHP stipulated in config.platform.php always matches the minimum version of PHP in core/composer.json, so that the tests will always catch any mistakes that may be made updating one of these without the other.

Note that since the minimum version of PHP 7.3 is currently 7.3.0, and there is no PHP 7.3 version lower than 7.3.0, the safeguards in this patch are currently unnecessary. However, if we include these tests now, it will protect us from errors similar to those described above once Drupal 9 starts requiring dependencies with higher patch versions of PHP 7.3.

Remaining tasks

We need to decide if this should be backported to the 8.9.x and/or the 8.8.x branches. In theory, the dependency versions on these branches should no longer change very often. However, it is possible that they might, e.g. due to a security update. With PHP 7.0 and 7.1 both being at EOL, some projects may decide to drop support for them in a minor or patch release. (I personally recommend against this, but it may happen.) Having this backported to the earlier branches therefore might be helpful, even though the odds of needing it there are fairly low.

Follow-on Work

User interface changes

None.

API changes

None.

Data model changes

None.

Release notes snippet

n/a

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

greg.1.anderson created an issue. See original summary.

greg.1.anderson’s picture

Status: Active » Needs review
Issue tags: +Composer initiative
FileSize
4.57 KB

Here's the patch.

Status: Needs review » Needs work

The last submitted patch, 2: 3098281-2.patch, failed testing. View results

greg.1.anderson’s picture

Title: Ensure the 'composer update' evaluates dependencies using the correct PHP version » Ensure that 'composer update' evaluates dependencies using the correct PHP version
FileSize
4.6 KB

Fix test failure. No interdiff; just ignore #2.

greg.1.anderson’s picture

Status: Needs work » Needs review
heddn’s picture

+1 on approach.

greg.1.anderson’s picture

We discovered that it can be inconvenient to have config.platform.php set in the Composer template projects, e.g. setting Drupal 8.8.x projects to evaluate with PHP 7.0.x (the 8.8.x minimum PHP version) can prevent the use of Drush 10,etc.

However, setting config.platform.php in the root composer.json project should be fine, as this setting will not be exposed to actual Drupal sites, as sites should not be built with `git clone` + `composer install`, and the root-level composer.json is not copied into the Composer project templates, or the tar/zip downloads.

Mile23’s picture

There's some history here. We had exactly that test up until we needed to support symfony 4 in Drupal 8. If we basically revert #3020301: Remove composer integration PHP version test (rely on DrupalCI PHP version runs instead) here for D9, we shouldn't backport for D8 for that reason.

And of course a min/max or at least a min test would be best. Apparently that gets run on commit? We could/should add an issue similar to #3044175: [DO NOT COMMIT] Vendor minimum testing issue for D9, but even then DrupalCI uses the latest PHP 7.2 and not our minimum spec.

greg.1.anderson’s picture

#3020301: Remove composer integration PHP version test (rely on DrupalCI PHP version runs instead) was to allow both PHP5 and PHP7. PHP5 hasn't been supported on Drupal 8 since May 2019, and we don't need anything older than PHP 7.2 for Drupal 9. I don't think we need to bring #3020301 back for Drupal 9. I think it's fine if we make this patch Drupal-9-only and do not backport it.

Min/max testing would probably be a good thing to have, but for now we have drupal/recommended-core, so that's out of scope for this patch. This patch will ensure that the contents of the composer.lock file is consistent regardless of who generates the lock file (regardless of what version of PHP is used to generate the lock file, that is), which is important and orthogonal to min/max testing.

I rerolled the patch so that it would apply to the HEAD of 9.0.x again. No difference in patch contents, so no interdiff.

heddn’s picture

Could/should we set the version instead of 7.2.9 to be DRUPAL_MINIMUM_PHP? Then we could have tests that validate everything is kept in sync. If though, that really doesn't work (as per the IS that could lead to issues), should we be bumping the minimum version?

greg.1.anderson’s picture

It is not possible to use a constant e.g. DRUPAL_MINIMUM_PHP in a .json file. However, you are correct that the value we use should be consistent with DRUPAL_MINIMUM_PHP.

Mile23’s picture

It is not possible to use a constant e.g. DRUPAL_MINIMUM_PHP in a .json file. However, you are correct that the value we use should be consistent with DRUPAL_MINIMUM_PHP.

In that case....

+++ b/core/tests/Drupal/Tests/Composer/ComposerTest.php
@@ -44,4 +44,14 @@ public function testEnsureBehatDriverVersions() {
+  public function testEnsurePhpConfiguredVersion() {
+    $composer_json = json_decode(file_get_contents($this->root . '/composer.json'), TRUE);
+    $composer_core_json = json_decode(file_get_contents($this->root . '/core/composer.json'), TRUE);
+
+    $this->assertEquals($composer_core_json['require']['php'], '>=' . $composer_json['config']['platform']['php'], 'The extra.platform.php configured version in the root composer.json file should always be exactly the same as the minimum php version configured in core/composer.json.');

Add an assertion that compares this value to DRUPAL_MINIMUM_PHP.

Also: "The extra.platform.php configured version..." should be config.platform.php.

greg.1.anderson’s picture

Good catch. It's not easy to test DRUPAL_MINIMUM_PHP (see #2908079: Move some of the bootstrap.inc PHP-related constants to \Drupal and deprecate the old versions). The workaround I used is safe, but not pretty.

Mile23’s picture

+++ b/core/tests/Drupal/Tests/Composer/ComposerTest.php
@@ -44,4 +44,27 @@ public function testEnsureBehatDriverVersions() {
+    $drupal_minimum_php = exec("php -r 'include \"{$this->root}/core/includes/bootstrap.inc\"; print DRUPAL_MINIMUM_PHP;'", $status);

The solution is to make this a kernel test and then just look at the value.

greg.1.anderson’s picture

Ah yeah, that works much better indeed.

Status: Needs review » Needs work

The last submitted patch, 15: 3098281-15.patch, failed testing. View results

greg.1.anderson’s picture

Um, any ideas why the test runner is saying this:

17:14:55 Fatal error: Cannot declare class Drupal\KernelTests\Component\Utility\ComposerKernelTest, because the name is already in use in /var/www/html/core/tests/Drupal/KernelTests/Composer/ComposerKernelTest.php on line 0

Test passes locally when run in isolation. There is only one ComposerKernelTest. Why is it included twice? I got the wrong namespace, but it doesn't seem that would cause this problem. Is there something I need to do when adding a new KernelTest?

greg.1.anderson’s picture

Fixing the namespace.

heddn’s picture

Status: Needs review » Needs work
Issue tags: +Needs change record updates

We should update the CR that documents the minimum PHP version. See https://www.drupal.org/node/3089166

greg.1.anderson’s picture

How do we deal with updating a change record that has already been published? If I edit it now, then the information in it will be wrong until this issue is committed. If I unpublish, that would be awkward if this issue is not committed for a long time. If I make a new CR, then we have an extra one we don't need after this is merged. What's the usual trade-off here? Just unpublish? Edit with a "preliminary change" note, and then edit again when this is committed?

heddn’s picture

Why not just change it to read 7.2 in the title? https://www.drupal.org/docs/9/how-drupal-9-is-made-and-what-is-included/... doesn't include it. And then just update the CR body to note when it was changed 7.2.3 and when it will change to 7.2.9?

greg.1.anderson’s picture

All right, I'll do it like that. I guess it's not a big deal to change it in advance, since the minimum PHP version is effectively already 7.2.9; it just isn't enforced in Drupal's checks yet.

greg.1.anderson’s picture

Status: Needs work » Needs review

Updated the CR to note that the effective PHP minimum version is already 7.2.9 due to Drupal's dependencies. The current test is accurate before and after this issue is committed, although it would be best to edit the CR again after committing here to fix the tense.

Also note that with this change, it will be necessary to increase the enforced minimum PHP in order to use newer dependency versions that require newer versions of PHP, so there will no longer be a risk that Drupal will advertise a lower-than-actually-needed minimum PHP version.

heddn’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs change record updates

Looks good to me. LGTM.

Mile23’s picture

For future reference, if you need to change the CR but you're not sure about whether it's actually appropriate at the moment, you can add the 'needs change record updates' tag. Then someone will clean it up around commit time.

Also note that with this change, it will be necessary to increase the enforced minimum PHP in order to use newer dependency versions that require newer versions of PHP, so there will no longer be a risk that Drupal will advertise a lower-than-actually-needed minimum PHP version.

This is why this gets a +1 from me. It's especially nice to get this kind of process happening early in the D9 cycle so we're used to thinking about it moving forward.

We might well see a situation like that in #3020301: Remove composer integration PHP version test (rely on DrupalCI PHP version runs instead) where upstream frameworks that we want to support span different PHP versions. If that happens again, we can remove the platform config from composer.json again, or we could figure out some other solution.

Ideally, we'd also have automated regression testing. :-) #2874198: Create and run dependency regression tests for core

greg.1.anderson’s picture

We could maybe back off the minimum PHP version to 7.2.5 now, as it seems that 7.2.9 was not really needed by our dependencies, and 7.2.9 prohibits SLES 12 (SP4).

greg.1.anderson’s picture

Oh, wait, while we could change our DRUPAL_MINIMUM_PHP to 7.2.5 right now, we cannot change our minimum PHP requirement in our composer.json until we actually have (and are using) the new stable releases of our dependencies that allow PHP 7.2.5.

Right now, our effective minimum PHP is in fact 7.2.9, so let's consistently advertise that fact.

greg.1.anderson’s picture

alexpott’s picture

Status: Reviewed & tested by the community » Needs work

D9 now requires PHP 7.3

greg.1.anderson’s picture

Status: Needs work » Needs review
FileSize
4.9 KB

Rerolled. No interdiff, because HEAD of 9.0.x now already does things from #18, so it's just confusing.

Status: Needs review » Needs work

The last submitted patch, 30: 3098281-30.patch, failed testing. View results

greg.1.anderson’s picture

Issue summary: View changes
Status: Needs work » Needs review
FileSize
418 bytes
5.31 KB

Tests are working exactly as they should.

xjm’s picture

Issue tags: +beta target

I could see this being a beta target.

jungle’s picture

+++ b/core/tests/Drupal/KernelTests/Composer/ComposerKernelTest.php
@@ -0,0 +1,33 @@
+    // If DRUPAL_MINIMUM_PHP were available in an autoloadable class, then this
...
+    $this->assertEquals(DRUPAL_MINIMUM_PHP, $composer_json['config']['platform']['php'], 'The DRUPAL_MINIMUM_PHP constant should always be exactly the same as the config.platform.php in the root composer.json.');

DRUPAL_MINIMUM_PHP is deprecated, see #2908079: Move some of the bootstrap.inc PHP-related constants to \Drupal and deprecate the old versions, use \Drupal::MINIMUM_PHP instead,

Edited: #13 already mentioned, in fact. missed that.

jungle’s picture

+1 to backport this to 8.x to make updating dependencies easier, and less error-prone. in #3122112: Update dependencies for Drupal 8.9, @longwave had to manually setted config.platform.php to 7.0.8 to update dependencies and removed it later, because, on his local, the PHP version is 7.4

longwave’s picture

The problem with doing this in 8.x is that it stops PHPUnit from being upgraded with composer drupal-phpunit-upgrade if you are on a higher PHP version.

$ git diff
diff --git a/composer.json b/composer.json
index a8602e3335..5fa2f81609 100644
--- a/composer.json
+++ b/composer.json
@@ -39,7 +39,10 @@
     "prefer-stable": true,
     "config": {
         "preferred-install": "dist",
-        "autoloader-suffix": "Drupal8"
+        "autoloader-suffix": "Drupal8",
+        "platform": {
+            "php": "7.0.8"
+        }
     },
     "extra": {
         "_readme": [

$ php -v
PHP 7.4.5 (cli) (built: Apr 19 2020 07:36:30) ( NTS )
Copyright (c) The PHP Group
Zend Engine v3.4.0, Copyright (c) Zend Technologies
    with Zend OPcache v7.4.5, Copyright (c), by Zend Technologies

$ vendor/bin/phpunit --version
PHPUnit 6.5.14 by Sebastian Bergmann and contributors.

$ composer drupal-phpunit-upgrade
...

$ vendor/bin/phpunit --version
PHPUnit 6.5.14 by Sebastian Bergmann and contributors.

$ composer why-not phpunit/phpunit:7
phpunit/phpunit  7.0.0      requires          php (^7.1 but 7.0.8 is installed)  

This can perhaps be worked around by using the --ignore-platform-reqs switch in Composer but I am unsure if that has other unwanted side effects.

I am also unsure if this also prevents other packages that require higher PHP versions from being installed when Drupal is installed using a Composer template, which might be the case for some contrib modules or third party Composer libraries. This probably isn't an issue now in Drupal 9, but it might be further down the line if a package requires PHP 7.4?

greg.1.anderson’s picture

We could make composer drupal-phpunit-upgrade remove the php platform requirement when it ran, if we wanted to be able to upgrade phpunit and specify a php platform requirement.

However, as mentioned in the issue summary, updating composer dependencies is not going to happen very often in the 8.8.x / 8.9.x branches any longer. It might be simpler to just forgo the backports, and just do this for 9.

alexpott’s picture

re #36

I am also unsure if this also prevents other packages that require higher PHP versions from being installed when Drupal is installed using a Composer template, which might be the case for some contrib modules or third party Composer libraries. This probably isn't an issue now in Drupal 9, but it might be further down the line if a package requires PHP 7.4?

As far as I can see the composer templates are completely unaffected (which is a good thing). Therefore projects will not be affected by this change - another good thing. To me this looks like a sensible safeguard for core. If we have to do a similar phpunit upgrade thing for D9 then we can do the ignore platform req switch to get it to work.

+++ b/core/tests/Drupal/Tests/Composer/Generator/MetapackageUpdateTest.php
@@ -7,6 +7,7 @@
+use Drupal\Composer\Composer;

@@ -70,7 +71,7 @@ public function testUpdated($builderClass, $path) {
-    $version = str_replace('.0-dev', '.x-dev', \Drupal::VERSION);
+    $version = Composer::drupalVersionBranch();

+++ b/core/tests/Drupal/Tests/ComposerIntegrationTest.php
@@ -3,6 +3,7 @@
+use Drupal\Composer\Composer;

@@ -19,7 +20,9 @@ class ComposerIntegrationTest extends UnitTestCase {
-    $this->assertSame($content_hash, $lock['content-hash']);
+
+    $drupal_version_branch = Composer::drupalVersionBranch();
+    $this->assertSame($content_hash, $lock['content-hash'], "Please run 'COMPOSER_ROOT_VERSION=$drupal_version_branch composer update --lock' to update Composer lock hash.");

These changes surprise me - why are they necessary - or in scope?

Pasqualle’s picture

Version: 9.0.x-dev » 9.1.x-dev
alexpott’s picture

+++ b/core/composer.json
@@ -17,7 +17,7 @@
-        "php": ">=7.3",
+        "php": ">=7.3.0",

Is this really necessary?

xjm’s picture

Version: 9.1.x-dev » 9.2.x-dev
Issue tags: -beta target

Cleaning up pre-9.0.0 beta targets.

alexpott’s picture

This has become important for PHP 8 compatibility because our composer.lock with it's locked PHPUnit dependencies is not PHP 8 compatible. If we set this a user would have the chance to install and then run composer run drupal-phpunit-upgrade. If they forget to run drupal-phpunit-upgrade they will be told to when running phpunit tests.

alexpott’s picture

Rerolled on 9.2.x and 9.1.x

Since this was last worked on DRUPAL_MINIMUM_PHP is available in unit tests - bootstrap.inc is always loaded (yay).

Also the test changes to MetapackageUpdateTest and ComposerIntegrationTest in #32 are actually unrelated.

alexpott’s picture

Answering #40

+++ b/composer.lock
@@ -554,7 +554,7 @@
-                "php": ">=7.3",
+                "php": ">=7.3.0",

+++ b/core/tests/Drupal/Tests/Composer/ComposerTest.php
@@ -25,4 +25,17 @@ public function testEnsureComposerVersion() {
+    $this->assertEquals(DRUPAL_MINIMUM_PHP, $composer_json['config']['platform']['php'], 'The DRUPAL_MINIMUM_PHP constant should always be exactly the same as the config.platform.php in the root composer.json.');
+    $this->assertEquals($composer_core_json['require']['php'], '>=' . $composer_json['config']['platform']['php'], 'The config.platform.php configured version in the root composer.json file should always be exactly the same as the minimum php version configured in core/composer.json.');

Making this changing makes the test easy. I guess we could argue that the second assertion is not really necessary but it is nice to keep everything the same and consistent.

longwave’s picture

Status: Needs review » Reviewed & tested by the community

This looks good to me. The justification is sound and the concerns I had earlier no longer apply. If in the future we add support for PHPUnit 10 and it has higher minimum PHP requirements we might run into an issue, but we can work around it if and when that happens.

RTBC assuming testbot agrees.

andypost’s picture

  • catch committed 2c757ba on 9.2.x
    Issue #3098281 by greg.1.anderson, alexpott, heddn, Mile23, xjm, jungle...

  • catch committed b774736 on 9.1.x
    Issue #3098281 by greg.1.anderson, alexpott, heddn, Mile23, xjm, jungle...
catch’s picture

Status: Reviewed & tested by the community » Fixed

Committed/pushed the respective patches to 9.1.x and 9.2.x, thanks!

xjm’s picture

Issue tags: +Needs change record

Wait, so what happens with this change if someone tries to install on PHP 7.4 or 8.0?

alexpott’s picture

This only affects core developers as it is the root composer.json and the affects it has are:

  • ensures that when a core developer runs composer update it is compatible with our minimum php
  • allows a core developer / DrupalCI to run composer install on PHP 8.0 even though the our composer.lock is locked to dependencies that claim they are incompatible.

I don't think a change record is necessary here. No one has to change anything.

xjm’s picture

Issue tags: -Needs change record

Seems alright then; someone else had misunderstood the issue.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

dpi’s picture

This only affects core developers

Thought I'd mention that this does affect contrib in an interesting way. Specifically the way DrupalCI works: tests break if a contrib being tested has a higher PHP req than core does. See #3194848: Not possible to run DrupalCI when projects require a PHP version higher than 7.3.0