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
Comment | File | Size | Author |
---|---|---|---|
#57 | interdiff-55-57.txt | 1.02 KB | mpdonadio |
#57 | 2843328-57.patch | 3.48 KB | mpdonadio |
#55 | 2843328-55.patch | 3.12 KB | mpdonadio |
Comments
Comment #2
alexpottComment #3
webflo CreditAttribution: webflo at UEBERBIT GmbH commentedComment #4
alexpottI'm not 100% sure about this change. One mitigation would be to have a testbot running 5.5.9 - this has been mooted.
Comment #6
Saphyel CreditAttribution: Saphyel as a volunteer commentedComment #7
alexpott@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.
Comment #8
Saphyel CreditAttribution: Saphyel as a volunteer commented@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...
Comment #9
cilefen CreditAttribution: cilefen commentedIt 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.
Comment #10
webflo CreditAttribution: webflo at UEBERBIT GmbH commented#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
Comment #11
tstoecklerI'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, becausecomposer.json
is explicitly a user-editable file.Comment #12
webflo CreditAttribution: webflo at UEBERBIT GmbH commentedThe 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?
Comment #13
alexpottRerolled.
Comment #14
alexpottFor the types of issues not doing this can cause see #2862254-19: Update non-Symfony dependencies before 8.3.0
Comment #15
cilefen CreditAttribution: cilefen commented#11 is correct about what I wrote in #9. I would RTBC this but for the points brought up in #12.
Comment #16
timmillwood@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.
Comment #17
dawehnerI am wondering whether this PHP version requirement could be moved to the core/composer.json file. Would that help with the problem described here?
Comment #18
timmillwoodThe "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.
Comment #19
cburschka-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.)
Comment #21
cilefen CreditAttribution: cilefen commentedComment #22
Mile23Since 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.
Comment #23
fjgarlin CreditAttribution: fjgarlin at Amazee Labs commentedHi,
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.
Comment #24
cilefen CreditAttribution: cilefen commented8.3.7 was released yesterday.
Comment #25
fjgarlin CreditAttribution: fjgarlin at Amazee Labs commentedI know, that's why I asked about #20. It suggests that 8.3.7 won't receive the fix.
Comment #26
cilefen CreditAttribution: cilefen commentedIt isn't "won't"—it "did not". Sorry if this is just semantics.
Comment #27
fjgarlin CreditAttribution: fjgarlin as a volunteer commentedYou'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.
Comment #28
cburschkaThis 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.
Comment #29
fjgarlin CreditAttribution: fjgarlin as a volunteer commentedWon't this affect the PHP requirements for Drupal 8?
Comment #30
cilefen CreditAttribution: cilefen commented+1 to #28, and I just released the beta.
Comment #31
mpdonadioRead 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.
Comment #32
dawehnerThe 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.
Comment #33
mpdonadioRe #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.
?
Comment #34
Mile23That'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
Comment #35
mpdonadioIf 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.
Comment #36
Mile23I ran
composer update
(on my PHP 7.1 system) and then ran this test, and here's what it said:Tracking it down, it's doctrine/annotations failing the test for me, and sure enough:
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.
Add a @todo about https://www.drupal.org/node/2874198
Put the package name in the fail message so it's useful.
Comment #37
mpdonadio#36 plus IS updates (edit if anyone feels I misstated something). If we go with this, then we don't need a CR.
Comment #38
Mile23Since it's a test instead of a dependency change, we can move it to 8.4.x.
Comment #39
dawehnerCould we read this value theoretically from the
core/composer.json
fileComment #40
cburschkaPossibly, 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:
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.Comment #41
timmillwoodThe 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 😜
Comment #42
cburschkaI 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. :)
Comment #43
Mile23So we've determined that
ComposerIntegrationTest::MIN_PHP_VERSION
can't or really shouldn't be replaced with values parsed fromcore/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.
Comment #44
Mile23Updated 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."
Comment #45
xjmThis is duplicating:
const DRUPAL_MINIMUM_PHP = '5.5.9';
in bootstrap.inc.
Do we really want to have it hardcoded in three separate places?
Comment #46
mpdonadio#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.
Comment #47
Mile23+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
Comment #48
Mile23Needs a @todo for the follow-up: #2908079: Move some of the bootstrap.inc PHP-related constants to \Drupal and deprecate the old versions
Comment #49
mpdonadioHere 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.
Comment #50
Mile23Sorry, 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.
Comment #51
mpdonadioHere we go.
Comment #52
heddn#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.
Comment #53
xjmI'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.
:(
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.Comment #54
xjmOkay, 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.
Comment #55
mpdonadio#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+.
Comment #56
alexpottSo 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.Comment #57
mpdonadioHere is a comment about dev version. Still need to update IS.
Comment #58
Mile23Update IS to basically say #56.
Added test for PHP 7.2 to #57.
Comment #59
Mile23In the console record for the PHP 7.2 test build we see:
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:
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. :-)
Comment #62
catch