Problem/Motivation

When doing updates on or adding new composer managed dependencies, it is possible to accidentally include packages that do not work with the minimum supported PHP version (currently 5.5.9) because package choices get made based on the PHP version where composer is run to generate the lockfile.

Proposed resolution

There are two options:

1. Composer allows us to configure a platform so that when resolving dependencies it'll behave as though you are using that version of PHP. See https://getcomposer.org/doc/06-config.md#platform. Doing this will prevent composer from automatically including software that is incompatible with our minimum version. This option is no longer viable, as of Drupal 8.5.x. See below.

2. Add a unit test to check the PHP version for packages in the lockfile.

Of the options (2) is a better stop-gap until we can figure out #2874198: Create and run dependency regression tests for core.

As of Drupal 8.5.x we have different PHPUnit requirements for different versions of PHP, in order to account for incompatibilities with PHP 7.2. Therefore we can't specify a maximum PHP platform in composer.json or we won't be able to run tests on PHP 7.2. This leaves us with option 2 above: Testing the lock file.

Remaining tasks

User interface changes

None

API changes

None

Data model changes

None

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

alexpott created an issue. See original summary.

alexpott’s picture

Status: Active » Needs review
FileSize
1.14 KB
webflo’s picture

Issue tags: +Composer
alexpott’s picture

I'm not 100% sure about this change. One mitigation would be to have a testbot running 5.5.9 - this has been mooted.

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.0-alpha1 will be released the week of January 30, 2017, which means new developments and disruptive changes should now be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Saphyel’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: +SprintWeekend2017, +london_2017_january
alexpott’s picture

Status: Reviewed & tested by the community » Needs review

@Saphyel - neither @webflo or I are 100% sure about this change. It'd be great if #6 can contain a reason why you think this is a good change.

Saphyel’s picture

@alexpott I think is a good change because forces you to behave like you are using php 5.5.9
A lot of project use them in order to avoid incompatibilities...

Anyway if you changed your mind about this could you argue why not? I saw nothing commented here so I thought it was ok...

cilefen’s picture

It certainly works as advertised. To be honest, I didn't know this option exists until now! But whether to include this in the shipped composer.json is an interesting question. What if a site admin builds a Drupal site with composer that is targeted for production on PHP 7.1 but a contrib module needs a library that is PHP 7.0+ compatible only? That should be fine, but with this patch composer would refuse to install the library.

webflo’s picture

#2 prevents the installation of packages with PHP 7 as minimum requirement. I build a custom package to test few things https://packagist.org/packages/webflo/drupal-2843328

7.0.x-dev: A developer should be able to install this version, if the target version of the site is PHP 7.
5.5.x-dev: Should be the preferred package for core 
5.4.x-dev: Not installable
tstoeckler’s picture

I've also just discovered this setting and wanted to open an issue for this... but it already exists, nice!

Wanted to RTBC, but...

Re #7: Can you elaborate on the reservations. I've played around with this for a project where the production server was still using PHP 5.5, while all developers where already on PHP 7. Setting the platform PHP version worked perfectly, and I don't really see any downside.

Re #9: Then that person has to override the platform config in composer.json to the minimal PHP version used on their setup, i.e. update their platform PHP version accordingly. That's fine, because composer.json is explicitly a user-editable file.

webflo’s picture

Issue tags: +Needs change record

The change makes sense, but we have to it carefully. composer.json is user-editable but its also the base for Drupal CI. 5.5.9 will prevent the installation of packages with PHP 5.6 or 7.x as minimum version. This should break the Drupal CI integration for some contrib projects.

Maybe Drupal CI should override the settings depending on the PHP Version on the testbot? But this has other implications on the composer hash?

alexpott’s picture

alexpott’s picture

For the types of issues not doing this can cause see #2862254-19: Update non-Symfony dependencies before 8.3.0

cilefen’s picture

#11 is correct about what I wrote in #9. I would RTBC this but for the points brought up in #12.

timmillwood’s picture

@webflo just pointed me to this issue because I have a client project based off drupal-composer/drupal-project (which I also opened a PR for https://github.com/drupal-composer/drupal-project/pull/292), when doing a composer update it pulled in doctrine/inflector (via doctrine/common) which new requires PHP7.

This discovery with doctrine/inflector means we should either:
- Add the platform config to core.
- Force doctrine/inflector version 1.1.0 which doesn't require PHP7.
- Be very careful when making composer updates in core that we don't accidentally add a PHP7 dependency.

I have added the platform config to my client project, and thus also support this change in core. The top level composer.json is there to be changes, so we should document somewhere that this config should be updated to the version of the production environment the site will be deployed to.

dawehner’s picture

I am wondering whether this PHP version requirement could be moved to the core/composer.json file. Would that help with the problem described here?

timmillwood’s picture

The "platform" setting is part of "config" which is a root-only value, so needs to be in the top level composer.json in the project root.

core/composer.json already has a php version requirement.

cburschka’s picture

-1.

My concern is that hardcoding a value for this seems to come with a substantial cost for a common use case while fixing a problem that only occurs in an edge case:

With this change, if your environments are all PHP7 (which afaik is >50% at this point), then you need to manually edit the root composer.json file to install PHP7+ packages. Worse, the platform config overrides any checks on what version is actually running, so you may get code that breaks on PHP7* without being warned. IMHO this can't be mitigated by documentation - we would be actively subverting the expected behavior of composer and suppressing compatibility checks.

Meanwhile, the problem that this patch intends to fix only exists on cross-platform deployments. If you're using PHP5 both locally and on the production server, you have no problems; you only need to be careful with composer.lock if your environments are not in sync.

(*A problem that is being overlooked in this issue is that PHP7 is not backward-compatible with PHP5, so we cannot safely force packages to support the latter. Even though Drupal supports both platforms at once, we cannot guarantee that all third party packages do. Some may release different versions for PHP5 and PHP7, and overriding the platform check will cause incompatible versions to be installed.)

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.0-alpha1 will be released the week of July 31, 2017, which means new developments and disruptive changes should now be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

cilefen’s picture

Mile23’s picture

Since we're using wikimedia/composer-merge-plugin, config won't be merged from core/composer.json, so there's no point in putting it there.

There's kind of no point in putting it in DRUPAL_ROOT/composer.json either. That's because the tarball builder (I'm pretty sure) uses --ignore-platform-req anyway.

The only benefit to adding it to root composer.json would be that core developers who end up submitting patches with a changed lock file would specify the minimum requirement.

DrupalCI does multiple Composer passes. The first two use --ignore-platform-req because they're always under PHP 7: An install of core after getting the codebase, and an install of whatever contrib is being tested.

Then patches get applied, which could change composer.json files.

Then within the PHP environment container, it performs composer install, in order to gather the requirements if they've changed due to patches, and to make sure the build is possible based on the platform and whatever other considerations.

If we add a platform config, then we lose the ability of the testbot to check our requirements per platform, and in some edge cases might end up leading to errors we can't track down.

So -1 here.

fjgarlin’s picture

Hi,

Does #20 mean that it won't be fixed for Drupal 8.3.7? We're having this problem after updating due to this: https://www.drupal.org/SA-CORE-2017-004. Applying the patch works but I wonder whether it'll be applied to core or not.

Thanks.

cilefen’s picture

8.3.7 was released yesterday.

fjgarlin’s picture

I know, that's why I asked about #20. It suggests that 8.3.7 won't receive the fix.

cilefen’s picture

It isn't "won't"—it "did not". Sorry if this is just semantics.

fjgarlin’s picture

You're right, reading it back it is confusing. My question was (and is) if there will be a 8.3.8 version (with the patch in #13 applied) or not? #20 suggests it won't. At this point, it's mere curiosity. Thanks.

cburschka’s picture

This would be a pretty significant architectural change with an impact on existing installations, so I doubt it would even still be included in 8.4.x now that we're close to beta. 8.3.x will only get security fixes at this point.

fjgarlin’s picture

Won't this affect the PHP requirements for Drupal 8?

cilefen’s picture

+1 to #28, and I just released the beta.

mpdonadio’s picture

Read through this a few times, and I have been following/working on several of the related composer issues.

I added Needs issue summary update because I think it doesn't clearly describe what problem we are trying to solve, and a few scenarios are mentioned/implied in the comments:

1. Core devs submitting patches with composer changes, eg me running PHP7 locally and bumping a version in a core patch
2a. DrupalCI for core
2b. DrupalCI for contrib
3. Deployment, running PHP7 locally and deploying to a PHP5.5 server

It seems like our primary case is (1).

I think I am -1 to this because of potential impact on end-users. I think this is better handled by manual testing when we have core dependency changes.

dawehner’s picture

The other alternative we could do is to have some kind of linting / static analysis do figure out which PHP version we need, along other things.

mpdonadio’s picture

Re #32, it think it should be totally feasible to add ComposerIntegrationTest::checkPHPVersions() to loop through compser.lock and check packages.*.require.php for >= 5.5.9 (prob should have this as a real constant somewhere), assuming there is an existing function to parse the semver requirement syntax that composer uses.

?

Mile23’s picture

Re #32, it think it should be totally feasible to add ComposerIntegrationTest::checkPHPVersions() to loop through compser.lock and check packages.*.require.php for >= 5.5.9 (prob should have this as a real constant somewhere), assuming there is an existing function to parse the semver requirement syntax that composer uses.

That's what Composer does when we build. So therefore let's build on different platforms as a repeatable CI step. #2874198: Create and run dependency regression tests for core

mpdonadio’s picture

If we have it as a core test, though, devs can run it locally and we can have it as part of normal tests w/o needing special runs. Not sure what this would means for contrib testing, though.

Mile23’s picture

Status: Needs review » Needs work

I ran composer update (on my PHP 7.1 system) and then ran this test, and here's what it said:

$ ./vendor/bin/phpunit -c core/ --testsuite unit --filter ComposerIntegrationTest
PHPUnit 4.8.36 by Sebastian Bergmann and contributors.

Testing 
.......................S...F

Time: 4.53 seconds, Memory: 60.00MB

There was 1 failure:

1) Drupal\Tests\ComposerIntegrationTest::testMinPHPVersion
Failed asserting that false is true.

/Users/paulmitchum/projects/drupal8/core/tests/Drupal/Tests/ComposerIntegrationTest.php:190

FAILURES!
Tests: 28, Assertions: 163, Failures: 1, Skipped: 1.

Tracking it down, it's doctrine/annotations failing the test for me, and sure enough:

$ composer show doctrine/annotations
name     : doctrine/annotations
[..]
requires
doctrine/lexer 1.*
php ^7.1
[..]

This outcome is good in that a patch using my new lock file would fail.

This is bad in that it shows that our core version constraints aren't actually honest, because our composer.json files allow packages that will fail a run-time test.

If we add config:platform:php:5.5.9 to root/composer.json, then we're expressing that, but then we're always testing a 5.5.9-compatible set of dependencies in drupalci, without testing any other set. (Which is sort of what we're doing now anyway, which is bad.)

In the any-test-is-better-than-no-test department, the patch in #35 is a good easy step to take towards a specific goal, but meanwhile testbot work continues. :-)

Here's a code review:

Still needs IS update.

  1. +++ b/core/tests/Drupal/Tests/ComposerIntegrationTest.php
    @@ -170,6 +179,19 @@ public function testAllModulesReplaced() {
    +   * Tests package requirements for the minimum supported PHP version by Drupal.
    

    Add a @todo about https://www.drupal.org/node/2874198

  2. +++ b/core/tests/Drupal/Tests/ComposerIntegrationTest.php
    @@ -170,6 +179,19 @@ public function testAllModulesReplaced() {
    +        $this->assertTrue(Semver::satisfies(static::MIN_PHP_VERSION, $package['require']['php']));
    

    Put the package name in the fail message so it's useful.

mpdonadio’s picture

Title: Add minimum PHP as platform config to composer.json » Enforce minimum PHP version in composer dependencies
Issue summary: View changes
Status: Needs work » Needs review
Issue tags: -Needs change record, -Needs issue summary update
Related issues: +#2874198: Create and run dependency regression tests for core
FileSize
1.54 KB
1.08 KB

#36 plus IS updates (edit if anyone feels I misstated something). If we go with this, then we don't need a CR.

Mile23’s picture

Version: 8.5.x-dev » 8.4.x-dev

Since it's a test instead of a dependency change, we can move it to 8.4.x.

dawehner’s picture

+++ b/core/tests/Drupal/Tests/ComposerIntegrationTest.php
@@ -10,6 +12,13 @@
   /**
+   * The minimum PHP version supported by Drupal.
+   *
+   * @see https://www.drupal.org/docs/8/system-requirements/web-server
+   */
+  const MIN_PHP_VERSION = '5.5.9';
+

Could we read this value theoretically from the core/composer.json file

cburschka’s picture

Possibly, but the core/composer.json contains a constraint (>=5.5.9) rather than a single version - and if we assume that constraint will always be of the form '>=version' it gets brittle and is probably not as good as using an explicit constant.

SemVer::satisfies() expects a version on the left side, and essentially checks if the constraints are compatible with an '==$version' constraint. If we try to do the same with '>=$version', it will always work because the constraints only have to overlap.

Specifically:

$parser = new Composer\Semver\VersionParser();
$parser->parseConstraints('>=5.5.9')->matches($parser->parseConstraints('>=5.4.9')); // true
$parser->parseConstraints('>=5.5.9')->matches($parser->parseConstraints('>=5.6.9')); // true

I don't see any function in composer/semver that can compare two constraints for containment, ie an assertion that accepts >=5.5.9 ⊂ ^5.5, but rejects >=5.5.9 ⊂ ^5.6. That is basically what we want to check, as I understand - that the root composer.lock doesn't contain any requirements stricter than the one in core/composer.json.

timmillwood’s picture

The latest patches will prevent the core composer.lock from being updated with incompatible dependencies, which is good, but if an end user runs composer update it'll still pull down (potentially) incompatible dependencies. The patch in #13 would resolve this, but if the end user is running PHP 7 on all environments it will prevent them getting the latest versions.

Ideally we should raise the Drupal minimum version to PHP 7 😜

cburschka’s picture

I do hope we can drop PHP5 support at some point before D9, but we've been really conservative in that area. ;)

More seriously, though, dependency requirements should be left fully to the end user. At most, we might add a note to https://www.drupal.org/docs/8/system-requirements/php explaining that composer update should be run in an environment identical to the production system.

Our minimum system requirements only apply to the core code and the default versions in composer.lock - if you run composer update, you are leaving that behind, and there are lots of other potential issues that Drupal's composer file can't protect you from. For example, a contrib module might depend on some other ext- or lib- package (maybe a PECL extension), and an update might change the version requirements on that, in a way that will break if your dev and prod environments are not equivalent.

Testing our composer.lock should be sufficient. :)

Mile23’s picture

Status: Needs review » Reviewed & tested by the community

So we've determined that ComposerIntegrationTest::MIN_PHP_VERSION can't or really shouldn't be replaced with values parsed from core/composer.json.

A couple folks also think there should be documentation about lack of BC for older PHP versions for an individual user's codebase should they perform composer update.

And generally the idea is sound and no one's saying hell no. Therefore: RTBC. I restarted the test.

Considering the idea of different extensions, I'm reminded of this issue: #2713073: Add required php extensions to composer.json

The patch applies to 8.5.x and 8.4.x. This really should be in 8.4.x.

Mile23’s picture

Updated https://www.drupal.org/docs/8/system-requirements/php with: "Drupal's Composer-based dependencies are packaged using PHP 5.5.9. If you are using a higher PHP version, you might benefit from issuing a composer update command to get more appropriate versions of Composer-based dependencies."

xjm’s picture

Status: Reviewed & tested by the community » Needs review
+++ b/core/tests/Drupal/Tests/ComposerIntegrationTest.php
@@ -10,6 +12,13 @@
+   * The minimum PHP version supported by Drupal.
+   *
+   * @see https://www.drupal.org/docs/8/system-requirements/web-server
+   */
+  const MIN_PHP_VERSION = '5.5.9';
+

This is duplicating:
const DRUPAL_MINIMUM_PHP = '5.5.9';
in bootstrap.inc.

Do we really want to have it hardcoded in three separate places?

mpdonadio’s picture

#45, DRUPAL_MINIMUM_PHP is part of the global namespace in the include, and ComposerIntegrationTest is a UnitTestCase, so it won't see that constant.

I'm wondering if we should do this as-is, then do a followup to deprecate DRUPAL_MINIMUM_PHP and move it to a class (hate to say it, but maybe \Drupal ?), and handle the few places where it actually gets used.

See also core/tests/Drupal/Tests/Core/Extension/ThemeHandlerTest.php which either needs to be fixed or fixed as part of the proposed f/up.

Mile23’s picture

Status: Needs review » Reviewed & tested by the community

+1 on moving things out of includes in a follow-up. See: Kill includes tag.

Also, it's kind of a question mark as to when we'll stop supporting PHP 5.5 so we might end up moving to 5.6 or above (and thus filing a bug report against this change... :-) ) before the follow-up happens.

#2578813: Figure out PHPUnit version support

#2899825: Release and support cycles for PHP dependencies

Mile23’s picture

Status: Reviewed & tested by the community » Needs work
mpdonadio’s picture

Status: Needs work » Needs review
FileSize
1.62 KB
582 bytes

Here is the todo. Do we also need an @internal so we don't need to provide BC in case (for some strange reason) someone extends this test? This is really a temp thing until the f/up in place.

Mile23’s picture

Status: Needs review » Needs work
+++ b/core/tests/Drupal/Tests/ComposerIntegrationTest.php
@@ -170,6 +181,22 @@ public function testAllModulesReplaced() {
+    $lock = json_decode(file_get_contents($this->root . '/composer.lock'), TRUE);

Sorry, I should have caught this before. If this file isn't present, the test should skip. The reasoning: This test exists to make sure we don't commit certain kinds of changes to composer.lock in the repo. If someone moves around their Drupal webroot and runs the tests they'll get a fail related to this.

mpdonadio’s picture

Status: Needs work » Needs review
FileSize
1.86 KB
948 bytes

Here we go.

heddn’s picture

Status: Needs review » Reviewed & tested by the community

#45 seems answered in #46 and a follow-up was opened in #48.

I don't think we need to mark a test as internal. In fact, unless it is a base class test class, I wouldn't think anything is something you can depend functioning in a certain way.

With that, I think we can return to RTBC.

xjm’s picture

I'm still a little worried about this test constant getting out of sync with our actual minimum version because we forget to update it. And, looking at the followup, it looks like this concern is legit... because ThemeHandlerTest's version of it has already gotten out of sync.

if (!defined('DRUPAL_MINIMUM_PHP')) {
  define('DRUPAL_MINIMUM_PHP', '5.3.10');
}

:(

Not bumping the issue back again but I want to think about this a little more. I also am not sure why it is better to define an out-of-sync constant than to manually include the file. Manually including a file the size of bootstrap.inc is not at all pretty, but neither is having minimum PHP versions set in tests that don't match the minimum PHP version we actually have. I guess it's not like the minimum version is going to go down, only up, so it's probably better than HEAD by adding this... but ech.

xjm’s picture

Status: Reviewed & tested by the community » Needs work

Okay, I have an idea. Maybe we can add a @todo to bootstrap.inc for the followup as part of this patch as well? That way there's at least a chance we'll look at it the next time we increase our version requirement and so maybe remember to check these other places. :P

But would also like to at least document here why redefining the constant is preferable over including a global file. Neither is pretty and either would require followup work. It's a question I guess of the test not being as cleanly isolated (which is gross, but how much would it actually hurt in this test?) vs. maintainability.

mpdonadio’s picture

Version: 8.4.x-dev » 8.6.x-dev
Status: Needs work » Needs review
Issue tags: +Needs issue summary update
FileSize
3.12 KB
1.26 KB

#54a should be done.

Will update IS for #54b later tonight.

Would also love to get this in 8.5.x during alpha, if possible, so we can move forward on some of the related issues that may land in 8.5.0+.

alexpott’s picture

So option 1 from the issue summary is now a complete no-go because we'd then be unable to test on PHP7.2 as that does a composer update to upgrade PHPUnit to a version that supports PHP7.2

The test added in the latest packages does work on PHP7.2 because it is only testing $lock['packages'] and not $lock['packages-dev'] which as it turns out is super sensible. I think we should add a comment explaining why we're not testing dev packages.

mpdonadio’s picture

Mile23’s picture

Issue summary: View changes
Issue tags: -Needs issue summary update

Update IS to basically say #56.

Added test for PHP 7.2 to #57.

Mile23’s picture

Status: Needs review » Reviewed & tested by the community

In the console record for the PHP 7.2 test build we see:

20:35:52 > Drupal\Core\Composer\Composer::upgradePHPUnit
[..]
20:37:29   - Updating phpunit/phpunit (4.8.36 => 6.5.5): Downloading (100%)

So PHPUnit 6.5 was installed, which would have placed it in the lock file, with its platform requirement of "php": "^7.0"

Then later we see:

20:38:59 Drupal\Tests\ComposerIntegrationTest                          28 passes  

So our new test passed, because it doesn't look at dev requirements. That's OK because we don't guarantee that dev dependencies will run on PHP 5.5, only that Drupal itself will. And if you have PHP version less than that, PHP 4.8 is installed.

Therefore RTBC. :-)

  • catch committed 35986f5 on 8.6.x
    Issue #2843328 by mpdonadio, alexpott, Mile23, cburschka, xjm: Enforce...

  • catch committed 6db700c on 8.5.x
    Issue #2843328 by mpdonadio, alexpott, Mile23, cburschka, xjm: Enforce...
catch’s picture

Version: 8.6.x-dev » 8.5.x-dev
Status: Reviewed & tested by the community » Fixed

Status: Fixed » Closed (fixed)

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