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 thecomposer 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
Comment | File | Size | Author |
---|---|---|---|
#43 | 3098281-9.1.x-43.patch | 3.45 KB | alexpott |
#43 | 3098281-43.patch | 3.45 KB | alexpott |
Comments
Comment #2
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commentedHere's the patch.
Comment #4
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commentedFix test failure. No interdiff; just ignore #2.
Comment #5
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commentedComment #6
heddn+1 on approach.
Comment #7
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commentedWe 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.
Comment #8
Mile23There'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.
Comment #9
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commented#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.
Comment #10
heddnCould/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?Comment #11
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commentedIt 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 withDRUPAL_MINIMUM_PHP
.Comment #12
Mile23In that case....
Add an assertion that compares this value to
DRUPAL_MINIMUM_PHP
.Also: "The extra.platform.php configured version..." should be config.platform.php.
Comment #13
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commentedGood 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.
Comment #14
Mile23The solution is to make this a kernel test and then just look at the value.
Comment #15
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commentedAh yeah, that works much better indeed.
Comment #17
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commentedUm, any ideas why the test runner is saying this:
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?
Comment #18
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commentedFixing the namespace.
Comment #19
heddnWe should update the CR that documents the minimum PHP version. See https://www.drupal.org/node/3089166
Comment #20
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commentedHow 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?
Comment #21
heddnWhy 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?
Comment #22
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commentedAll 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.
Comment #23
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commentedUpdated 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.
Comment #24
heddnLooks good to me. LGTM.
Comment #25
Mile23For 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.
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
Comment #26
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commentedWe 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).
Comment #27
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commentedOh, 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.
Comment #28
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commentedI created #3103416: Roll back our minimum PHP version support from 7.2.9 to 7.2.5 as a follow-on task.
Comment #29
alexpottD9 now requires PHP 7.3
Comment #30
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commentedRerolled. No interdiff, because HEAD of 9.0.x now already does things from #18, so it's just confusing.
Comment #32
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commentedTests are working exactly as they should.
Comment #33
xjmI could see this being a beta target.
Comment #34
jungleDRUPAL_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.
Comment #35
jungle+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
Comment #36
longwaveThe 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.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?
Comment #37
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commentedWe 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.
Comment #38
alexpottre #36
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.
These changes surprise me - why are they necessary - or in scope?
Comment #39
PasqualleComment #40
alexpottIs this really necessary?
Comment #41
xjmCleaning up pre-9.0.0 beta targets.
Comment #42
alexpottThis 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.Comment #43
alexpottRerolled 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.
Comment #44
alexpottAnswering #40
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.
Comment #45
longwaveThis 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.
Comment #46
andypostBtw every core component's composer.json has platform defined except root one, ++ to rtbc
Comment #49
catchCommitted/pushed the respective patches to 9.1.x and 9.2.x, thanks!
Comment #50
xjmWait, so what happens with this change if someone tries to install on PHP 7.4 or 8.0?
Comment #51
alexpottThis only affects core developers as it is the root composer.json and the affects it has are:
I don't think a change record is necessary here. No one has to change anything.
Comment #52
xjmSeems alright then; someone else had misunderstood the issue.
Comment #54
dpiThought 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