Problem/Motivation
#2842431: [policy] Remove PHP 5.5, 5.6 support in Drupal 8.7 outlined the policy for dropping support for PHP 5.5 and 5.6, but there are some details that were never implemented as part of that issue. This issue aims to resolve those.
Proposed resolution
C&P from @xjm's comment in the original issue:
- New sites will not be able to install on PHP 5.5 or 5.6.
- An error will appear on the status report for sites on PHP 5.5 and 5.6.
- Sites on PHP 5 will still be able to run Drupal updates, but a warning will be presented on the update screen.
- The 8.8.x branch will not be tested against PHP 5.
- We will update to PHPUnit 6.
- If possible, the 8.7.x branch will have one nightly test against PHP 5.6 (with PHPUnit 4).
- If a commit breaks the 8.7 nightly test on PHP 5.6, it may be reverted, but site owners should expect bugs and fatal errors if they do not update to PHP 7
- On PHP 5.6 only, the Migrate test suite should be skipped.
Remaining tasks
None.
User interface changes
If Drupal is attempted to be installed on PHP versions below PHP 7.0.8, the installer stops and does not allow to continue.
For existing Drupal sites below PHP version 7.0.8, a warning is displayed in status report about incompatibility.
API changes
The Drupal\simpletest\InstallerTestBase::continueOnExpectedWarnings
, Drupal\FunctionalTests\Installer\InstallerTestBase::continueOnExpectedWarnings
methods were refactored and moved to Drupal\Tests\RequirementsPageTrait
.
Data model changes
None.
Release notes snippet
PHP stopped supporting version 5.5 on 21 Jul 2016, two years and eight months ago and version 5.6 on 31 Dec 2018. Drupal's end of support for PHP 5.5 and 5.6 was announced in January 2018.
Therefore, new Drupal 8 installations now require PHP 7.0.8. For now, existing sites can still run and be updated on PHP 5.5.9 or higher, but will display a warning. Note that Drupal security updates will begin requiring PHP 7 as early as Drupal 8.8.0 (December 2019), so all users are advised to update to at least PHP 7.0.8 now.
Comment | File | Size | Author |
---|---|---|---|
#89 | interdiff.txt | 2.09 KB | effulgentsia |
#89 | 3039287-89.patch | 32.72 KB | effulgentsia |
#78 | interdiff.txt | 6.88 KB | effulgentsia |
#78 | 3039287-78.patch | 31.95 KB | effulgentsia |
#69 | interdiff-3039287-65-69.txt | 3.42 KB | bendeguz.csirmaz |
Comments
Comment #2
xjmThanks @balsama!
Promoting to critical because this is an essential security and maintainability step for this release.
Comment #3
bendeguz.csirmaz CreditAttribution: bendeguz.csirmaz at Cheppers commentedChanging
DRUPAL_MINIMUM_PHP
to 7.0.0.Comment #4
bendeguz.csirmaz CreditAttribution: bendeguz.csirmaz at Cheppers commentedRemoving the warning message about support for < PHP 7 is getting dropped.
Comment #5
Krzysztof DomańskiI think we should first solve #2908079: Move some of the bootstrap.inc PHP-related constants to \Drupal and deprecate the old versions.
That issue is almost ready. It is not necessary, but it will facilitate work!
Comment #6
BerdirShould this be a meta issue maybe, e.g. the phpunit/composer stuff probably makes sense to do separately.
Comment #7
jibran#3039611: Update core PHP dependencies for 8.8.x is for composer stuff. #3008870: Drop support for PHPUnit 4.8 once PHP 5 is no longer supported (8.8.x) is for phpunit stuff.
Comment #8
xjmThanks everyone!
#2908079: Move some of the bootstrap.inc PHP-related constants to \Drupal and deprecate the old versions will not block this issue; this is an 8.7 critical whereas that is a 8.8 would-be-good-to-do-but-not-major-or-anything. :)
So one important aspect of this change that's different from how we've dropped support before:
Marking NW for that. Thank you!
Comment #9
xjmWe also need browser and update test coverage for what's described in #8; there should be some foundational work for that in #2670966: Warn users of old PHP versions.
Comment #10
bendeguz.csirmaz CreditAttribution: bendeguz.csirmaz at Cheppers commentedChanging error to warning when the phase is update.
Unfortunately I got stuck with an error and I'm not sure how to progress with this issue.
Steps to reproduce
- Install a site with PHP 5.6.37
- Apply this patch OR just change DRUPAL_MINIMUM_PHP to 7.0.0
- Go to update.php
- The warning is shown as expected
- Refresh the site
- White screen of death, "Error: Uncaught Error: Call to undefined function system_rebuild_module_data() in {...}/core/includes/common.inc:1126"
Comment #11
catchsystem_rebuild_module_data() is on its way out, so it might be worth checking if this patch works alongside #2926068: Deprecate system_rebuild_module_data() and remove usages in core.
Comment #12
Wim LeersIs that just "reload the site in the browser" or is it
/rebuild.php
or is itapachectl -k restart
?Comment #13
hampercm CreditAttribution: hampercm at Acquia commentedI was able to reproduce the WSOD on PHP 5.6.40, using the steps given by @bendeguz.csirmaz in #10.
One detail that I see differently is that the WSOD that occurs when refreshing the browser tab actually produces this PHP error:
The error related to
system_rebuild_module_data()
occurs when I attempt to rundrush cr
to fix the issue.Comment #14
hampercm CreditAttribution: hampercm at Acquia commentedTo make matters worse, loading update.php, even once, running on php 5.6.40 seems to hose the site install in a way that is not obvious to fix. Browsing to any pages on the site begins to result in WSODs, with errors like:
Uncaught PHP Exception InvalidArgumentException: "No check has been registered for access_check.permission" at /Users/chris.hamper/Sites/drupal/core/lib/Drupal/Core/Access/CheckProvider.php line 97
Reverting the patch and attempting to perform a
dr cr
produces the error:PHP Fatal error: Call to undefined function system_rebuild_module_data() in /Users/chris.hamper/Sites/drupal/core/includes/common.inc on line 1136
Comment #15
xjmHmmm. So the access_check.db_update service is specific to
update.php
:Route definition adds the
_access_system_update
route requirement:And here's the relevant hunk in the
update.php
front controller:Is this reproducible only with the patch provided, or is this happening in HEAD? Edit: Chris said it was only with the patch, which is a relief.
Comment #16
xjmMy suggestion is to exclude changing the constant for now, and temporarily hardcode a check for PHP 7.0 in the installer, to see whether that makes
update.php
work here. It sounds like the whole service container might be broken following the update, which makes me think something during the update is croaking.If that turns out to solve the issue, then we can step through it in a debugger and see what needs to be changed.
I think we might end up defining a constant like
DRUPAL_MINIMUM_UPDATE_PHP
or something, and use that explicitly anywhere that might be accessed outside of the Drupal installer. (Or maybe instead it's a new constantDRUPAL_MINIMUM_INSTALL_PHP
that's only used in the core installer.)Comment #17
bendeguz.csirmaz CreditAttribution: bendeguz.csirmaz at Cheppers commented#12
@Wim Leers
Yes, it's just a "reload the site in the browser" .
#16
@xjm
Thanks! It turns out the problem is the extension lists that use
DRUPAL_MINIMUM_PHP
as a default value for the 'php' key when reading info files.Specifically,
ModuleExtensionList
causes this exception.Comment #18
bendeguz.csirmaz CreditAttribution: bendeguz.csirmaz at Cheppers commentedAdding a new constant for the default 'php' key values.
Comment #19
bendeguz.csirmaz CreditAttribution: bendeguz.csirmaz at Cheppers commentedFixing the test fails.
Comment #20
bendeguz.csirmaz CreditAttribution: bendeguz.csirmaz at Cheppers commented@xjm, I was thinking about skipping browser tests when the installation fails due to the version error.
Comment #21
xjmThanks @bendeguz.csirmaz. I think #20 is too general; we should not skip tests. We need those test results in order to find out when we're introducing a different regression only on those environments. Instead, maybe we need to allow Drupal to be installed on the minimum update PHP version only in tests where the test environment is between the minimum update PHP and the minimum install PHP, with a @todo + followup to remove the override once we drop update support.
Then, while the workaround is in place, we might need a one-off dedicated test specifically for the installer when the install PHP version is the "real" one for the branch.
Finally, update tests might need a step similar to
InstallerTestBase::setUpRequirementsProblem()
to ensure that we continue the update on the expected warnings on the allowed update PHP version.Comment #22
bendeguz.csirmaz CreditAttribution: bendeguz.csirmaz at Cheppers commentedLet's disregard #20 and see how many tests pass with #21.
Comment #23
bendeguz.csirmaz CreditAttribution: bendeguz.csirmaz at Cheppers commentedRemoving code from
InstallerTestBase::setUpRequirementsProblem()
as it is no longer needed. This should fix the failures in #22.Comment #24
bendeguz.csirmaz CreditAttribution: bendeguz.csirmaz at Cheppers commentedConsider the
DRUPAL_DEV_SITE_PATH
environment variable when installing too. This should fix the error in #23.Comment #25
xjmYay, green tests! 🎉 Thanks @bendeguz.csirmaz.
I think we'll actually keep this codepath around in the future, because we'll want to use the same update-but-not-install pattern around for the next time we drop support for a PHP version. So this comment can actually be replaced with something like:
The hardcoded
getenv()
check surprised me. It seems like something we should have explicit API for. However, I wasn't able to find anything more when grepping, and clearly it works since those tests are green!Comment #26
bendeguz.csirmaz CreditAttribution: bendeguz.csirmaz at Cheppers commentedAdding a test for the warning on the update page. Small refactor.
Comment #27
bendeguz.csirmaz CreditAttribution: bendeguz.csirmaz at Cheppers commentedFixing returning early when it's a test site...
Comment #28
bendeguz.csirmaz CreditAttribution: bendeguz.csirmaz at Cheppers commentedFor the record, in #26 & #27 I changed system_requirements() to always show a requirement warning for updates when the PHP version is smaller than
DRUPAL_MINIMUM_PHP
.#24 didn't show the warning when it was a test site.
Comment #29
bendeguz.csirmaz CreditAttribution: bendeguz.csirmaz at Cheppers commentedAdding a "continueOnExpectedWarnings" to UpdatePathTestBase.
This should fix the tests extending this class, but the function is duplicated.
Comment #30
larowlanlets not add a new global constants, let's put it on a class/object from the get-go
Comment #31
amateescu CreditAttribution: amateescu commented#2908079: Move some of the bootstrap.inc PHP-related constants to \Drupal and deprecate the old versions introduces a
Drupal\Core\Requirements
class for this.Comment #32
bendeguz.csirmaz CreditAttribution: bendeguz.csirmaz at Cheppers commented- creating a new trait for skipping the PHP warning
- allow other warnings on update.php besides the PHP warning in system_requirements
- fixing UpdateScriptTest
Comment #33
bendeguz.csirmaz CreditAttribution: bendeguz.csirmaz at Cheppers commented- fixing the rest of the tests
- deleting the new test from #26
- fixing a typo
Comment #34
xjmThanks everyone!
I disagree with #30, I think. We should not stealth-add the constant in a different place than the others before the general deprecation of all of them is done in #2908079: Move some of the bootstrap.inc PHP-related constants to \Drupal and deprecate the old versions. We can simply add this constant to the pile of things we deprecate there; it's not too costly.
I think this trait is misnamed since it also includes a method we use in install tests. We should also make the name a little more specific. Maybe something like
ExpectedRequirementsErrorTrait
or something?Comment #35
xjmMaybe we could give this a more clear name, like
continueOnExpectedUpdateWarnings()
?Also let's use the trait in
InstallerTestBase
(with any small refactors needed) so the code is in one place.Thanks!
Here, I think we can use the constant now rather than hardcoding 7.0. (That was a special behavior when we were treating PHP 5 differently than other old versions.)
Comment #36
catchApart from xjm's outstanding review notes this looks pretty good to me. Agreed on adding the global constant for now and moving everything together.
Comment #37
alexpottThe comment about the early return still looks relevant.
This comment goes with the elseif - I think we need to explain why we only have a warning on the update.
This bit is interesting. As in we can definitely remove \Drupal\FunctionalTests\Installer\InstallerExistingConfigSyncDirectoryProfileHookInstall::setUpRequirementsProblem() now. And it's an open question about whether we can remove this method, the call to it and \Drupal\FunctionalTests\Installer\InstallerTestBase::continueOnExpectedWarnings() as this is all used code now - at least in core. I think it is worth considering whether we should deprecate \Drupal\FunctionalTests\Installer\InstallerTestBase::continueOnExpectedWarnings() here because now this is untested so we'll not know to make changes to it is we change something that affects it. Another option is to used the trait here to provide BC and leave the setUpRequirementsProblem override to do nothing.
Comment #38
bendeguz.csirmaz CreditAttribution: bendeguz.csirmaz at Cheppers commentedRefactored the trait so it can be used in
Drupal\simpletest\InstallerTestBase
andDrupal\FunctionalTests\Installer\InstallerTestBase
too. I'd like to see if the existing tests pass first.Comment #39
bendeguz.csirmaz CreditAttribution: bendeguz.csirmaz at Cheppers commentedUse the trait in InstallerTestBase.
Comment #40
bendeguz.csirmaz CreditAttribution: bendeguz.csirmaz at Cheppers commentedOne thing I forgot in 38 & 39 is it always tries to click the continue button, even if the test is not on the "requirements problem" page.
Comment #41
bendeguz.csirmaz CreditAttribution: bendeguz.csirmaz at Cheppers commented#37 1 & 2
Fixing the comments.
Comment #42
bendeguz.csirmaz CreditAttribution: bendeguz.csirmaz at Cheppers commentedComment #43
bendeguz.csirmaz CreditAttribution: bendeguz.csirmaz at Cheppers commentedAdding the test for the installer. I'm not sure if this is the best solution... it skips 2 tests on older PHP versions. We could introduce a new environment variable to allow the site install (the
DRUPAL_DEV_SITE_PATH
is not enough).Comment #44
xjmComment #45
bendeguz.csirmaz CreditAttribution: bendeguz.csirmaz at Cheppers commentedFixing the test.
Comment #46
effulgentsia CreditAttribution: effulgentsia at Acquia commentedOk, I think I get what this constant is saying.
But this is saying that not only can you run updates on DRUPAL_MINIMUM_UPDATE_PHP, but you can also install new modules/themes on DRUPAL_MINIMUM_UPDATE_PHP. Is that desired? If so, should the constant be renamed and documented as such? Or do we consider installing new modules/themes as part of being able to update a site?
Comment #47
bendeguz.csirmaz CreditAttribution: bendeguz.csirmaz at Cheppers commented@effulgentsia
Yes, to update a site you need to be able to the install modules.
Comment #48
effulgentsia CreditAttribution: effulgentsia at Acquia commentedTo address #46/#47, @xjm suggested to rename
DRUPAL_MINIMUM_UPDATE_PHP
toDRUPAL_MINIMUM_PHP
and to renameDRUPAL_MINIMUM_PHP
toDRUPAL_MINIMUM_INSTALL_PHP
. I like that!Comment #49
bendeguz.csirmaz CreditAttribution: bendeguz.csirmaz at Cheppers commentedChanging the constants according to #48.
Comment #50
effulgentsia CreditAttribution: effulgentsia at Acquia commentedThis looks good. However, what runs right above this is:
So if someone tries installing with PHP 5.4, we first tell them that we require at least 5.5.9. Then, if they update to 5.5.9 and try installing again, we tell them we require at least 7.0.0. I think it makes sense to also change the description of this first one (just the description, not the if statement) to
'%version' => DRUPAL_MINIMUM_INSTALL_PHP
. So if they try installing on 5.4, we tell them they need 7.0.0 right away.The question then becomes whether to do that for all phases, or whether to toggle between DRUPAL_MINIMUM_PHP and DRUPAL_MINIMUM_INSTALL_PHP depending on the phase. I think it's okay to do it for all phases. If they're doing anything on PHP 5.4, I think it's ok if we tell them they need 7.0.0 (rather than telling them that they can get away with 5.5.9). But I'm willing to be convinced otherwise.
Why? This is running if they're on something between 5.5.9 and 7.0.0. What's unsafe (or undesired) about continuing with other requirements checks? In fact, for the 'runtime' phase, wouldn't we want to display other requirements violations in the status report too? And even for the 'install' phase, why not display the other requirements violations?
Comment #51
xjmLooks like this check is copied from earlier in the function. You can just see the end of it in the context lines above:
This dates way back to #807622: Fatal error calling sys_get_temp_dir() in installer - pHP 5.2.0 and earlier, which was fixed before the release of Drupal 7 (!).
It's also been obsolete anyway since #722974: Install script returns a blank page for PHP4, PHP 5.1 (should display *something* instead), which was changed well before Drupal 8's release: When we bumped the required PHP version to PHP 5.3 during Drupal 8's development, we changed the way we define constants from
define()
toconst
. When we did that, it started causing syntax errors on those older versions, which broke the installer entirely and prevented anything from being reported to the user. However, it turned out it didn't work anyway, so at some point pre-release we added this tocore/install.php
......So it looks like that's another place we need to fix. That, or we could consider removing the hunk entirely (because Drupal 8 has never supported PHP 5.3 and 5.3 itself has been EOL for 5 years). Edit: Although, it could still be useful if/when we start adopting PHP 7-only syntax, so maybe we should keep it, and just clean up the later logic.
Comment #52
xjmFollowing up on #51, I think we should also add a bit to the docblocks of
DRUPAL_MINIMUM_PHP
andDRUPAL_MINIMUM_INSTALL_PHP
indicating that the line in the installer should also be updated whenever the version constants are.Comment #53
effulgentsia CreditAttribution: effulgentsia at Acquia commentedThis means that the test passes if it doesn't return requirements problems, even in a situation where it should. Can we make this more robust? Like perhaps assert that there are requirements problems when the PHP version is less than DRUPAL_MINIMUM_INSTALL_PHP, and that there aren't when it's higher?
Comment #54
jibranThis should be 7.0.8 as per composer.json and https://packagist.org/packages/symfony/http-foundation#v3.4.23 require section.
Comment #55
bendeguz.csirmaz CreditAttribution: bendeguz.csirmaz at Cheppers commented#50.1, #50.2, #51, #52, #53, #54
Fixed!
#50.2, #51
@xjm
Yes, I think we should keep exiting early in
install.php
. The early return from #50.2 can be removed I think.Comment #56
bendeguz.csirmaz CreditAttribution: bendeguz.csirmaz at Cheppers commentedComment #57
alexpottI'm not sure about these having different signatures. Are we passing in the link label for translations? I think it might be best if the trait had the same signature as the old function and we passed in ['PHP'] everywhere. That way we could remove
InstallerTestBase::continueOnExpectedWarnings()
.How can we remove this code...
Ah this is why. Hmmm. I'm not sure on one hand how do we test this code then - this makes it untestable. On the other it feels a reasonable approach as it makes everything a bit simpler. One concern I have is what happens when we have to do something similar in future that generates warning again for the installer. Another concern is that in #8 / #9 the needs tests was added for exactly this situation.
I think this code can be simplified a bit and fixed. There's a bug in that when the PHP version is 5.6 and you look at runtime you'll not get an error - you'll only get an informational about updating to the recommended version.
I think something like:
Is better because:
Comment #58
bendeguz.csirmaz CreditAttribution: bendeguz.csirmaz at Cheppers commented#57.1
@alexpott
The idea is to use this trait on both the update and install requirements pages (#35).
On the update requirements page the continue button is called 'try again', on the install requirements page it is 'continue anyway'.
Creating a new method for update requirements and hardcoding the labels might be a better solution.
Comment #59
alexpott@bendeguz.csirmaz but we never hit the install requirements page now because of the
drupal_valid_test_ua()
check - right?Comment #61
bendeguz.csirmaz CreditAttribution: bendeguz.csirmaz at Cheppers commented#57.2
@alexpott
I agree. I managed to add a test for this in #43, but it's an ugly workaround.
We have to allow test installs on older versions (#21). The solution I think is to somehow continue on this specific
InstallerException
when installing test sites.Comment #62
bendeguz.csirmaz CreditAttribution: bendeguz.csirmaz at Cheppers commented#59 Yes.
Comment #63
bendeguz.csirmaz CreditAttribution: bendeguz.csirmaz at Cheppers commented#57.3, #61
Hmmm... Yes, you're right, an info level error seems like a good solution. We hit the requirements page, but it doesn't prevent test sites from installing.
Comment #64
alexpottMaybe
could be
Because then like re-installs we'll get an error on the requirements page at runtime when PHP > DRUPAL_MINIMUM_PHP but < DRUPAL_MINIMUM_INSTALL_PHP
That said I'm not a huge fan of DRUPAL_MINIMUM_INSTALL_PHP and DRUPAL_MINIMUM_PHP and supporting updates on PHP 5 in 8.7.x and greater. We now have extended security support for 8.6.x so I'm not sure why this is necessary anymore.
Comment #65
catchFor me it still makes sense to separate the install/update requirement. Otherwise we allow people to install on a PHP version one week, but error when they try to update two weeks later. If we tie the two together to the same release, we're always introducing a hard cliff between minors.
While we have extended security support for minor releases that would allow them to update to a security release, we'd not be able to point to which release to update to on update.php where we make this check without a lot of work. So we'd just be showing an error with a link to a docs page or something. There are still people who have control over their code base but not their hosting, such as some university hosting setups.
Comment #66
bendeguz.csirmaz CreditAttribution: bendeguz.csirmaz at Cheppers commentedHow about this?
Comment #67
alexpottI think InstallerTestBase::setUpRequirementsProblem() still is unnecessary because we make it only REQUIREMENT_INFO
Comment #68
alexpott@catch but at what point do we change the requirement in core/composer.json? If we are going to remove PHP5 support during Drupal 8's lifetime at some point we need to update that. And surely that's the point we should remove PHP 5 testing and install and update support. For example if you are using a configuration install based method for bringing up development environments or installing staging or feature test environments then this two different minimum PHPs is going to be really confusing and awkward.
Comment #69
bendeguz.csirmaz CreditAttribution: bendeguz.csirmaz at Cheppers commentedFixing #67.
Comment #70
effulgentsia CreditAttribution: effulgentsia at Acquia commented@alexpott: Re #68, I think there are 3 things that we'll probably want to synchronize into the same release:
1) Updating (non-dev) vendor dependencies to versions that have a PHP 7 minimum.
2) Updating our own core/composer.json to have a PHP 7 minimum.
3) Updating DRUPAL_MINIMUM_PHP to a PHP 7 minimum.
We could theoretically do 1) without doing 2) and 3). Especially if it's a library that doesn't actually have any PHP 5 breaks in a critical path. However, even if we choose to do all 3 of those together, it's too late to do them for 8.7, so we're talking about in the 8.8.x branch at whatever point in time that a release manager approves that.
Given that we're not doing any of the above for 8.7, the question then becomes: is it valuable to prevent people from installing new 8.7 sites on PHP 5, as well as is it valuable to provide extra warnings (during update.php and in the status report) for people who update an existing site to 8.7? If so, how would you suggest doing all that without introducing a new constant?
Comment #71
dawehnerI do have a question. It can happen that someone needs to re-install a site, using an existing DB dump. Many people are using tools like https://www.drupal.org/project/backup_migrate for this.
What's the expected workflow for someone on php 5.5/5.6 then?
?
I wonder whether it would be worth documenting this case, maybe in a change record for this issue.
This feels really risky for two reasons:
It seems worth mentioning on https://www.drupal.org/docs/8/system-requirements/php-requirements that update is still supported but not the install.
Comment #72
catch#70 is exactly what I mean with this patch allowing things to be phased. Even there are arguments for doing everything at once, by default we have not done that for any of the previous times we've dropped support for a PHP version. Or in other words the patch allows us to be more aggressive with when we drop support, not less, relative to the status quo of never actually getting around to it.
Comment #73
catchFor me no. We don't actually support people updating existing sites, the patch just allows us to not actively prevent people from doing this. The PHP version will still be untested etc.
Comment #74
alexpottI'm still super confused by this approach. The moment a library is PHP7 only and has a spaceship operator or something and is used in our update process our best effort support is broken. We have been placing warning in people's status reports since we announced dropping PHP5 support. If we're arguing that it is too late to properly drop PHP5 support for 8.7.x fine I can buy that that and I can also buy that we should have made the following code warn on update too
And I also understand that we might want to introduce a DRUPAL_FUTURE_MINIMUM_PHP and continue to issue a warning on update/install/runtime if you are under it. At the moment this is hardcoded to be
if (version_compare($phpversion, '7.0') < 0) {
in several places.Comment #75
catchRight, so we haven't updated libraries to require PHP 7 in 8.7.x, because we've been running tests against PHP 5 versions during development. This has drawbacks in that we've had to hold back some library updates, but it also avoids having to change patches we backport to 8.6.x and 8.5.x too just because of syntax changes or similar in Drupal code.
However assuming we do start to upgrade to libraries requiring PHP 7 in 8.8.x, we could also increase the update minimum PHP in that version to match. When we drop support for PHP 7.0, the infrastructure is here to prevent new installs prior to upgrading PHP dependencies etc. all over again. We don't actually have a proper policy for dropping PHP version support, which is partly why it's taken so long to drop PHP 5.5 and PHP 5.6, this issue is a symptom of that lack of policy, not its cause.
It's unlikely that we'll update a library dependency in 8.7.x to require PHP 7 from this point forward (i.e. post beta), but if we did so, it would most likely be due to a highly critical security release. In that case, unless the library update causes a fatal error on every request or something, the upgraded site on PHP 5.6 is likely to work better than a hacked one (or one stuck in a limbo of a new code base and updates not having been run).
Comment #76
alexpottBut that's what I don't understand. Is this site not better to be on 8.6.x which is still security supported and has PHP 5 support. Yes their urgency to get to support PHP7 is still there but at least they are not broken.
Comment #77
catchYes, but if they don't realise that 8.6 security support is available and better for them, they could end up trying to update to 8.7 anyway.
Additionally 8.7 is supported for a year, and the highly critical security release might not happen until 8.8.1, so if they take advantage of the longer security support at that point, they'll be better off than staying on a completely unsupported 8.6 (which might only be a month out of support by that point).
Comment #78
effulgentsia CreditAttribution: effulgentsia at Acquia commentedBased on this statement, here's a patch that renames the new constant from
DRUPAL_MINIMUM_INSTALL_PHP
toDRUPAL_MINIMUM_SUPPORTED_PHP
. I also added some more code docs (but did not change any user-facing text) to help clarify the difference betweenDRUPAL_MINIMUM_SUPPORTED_PHP
andDRUPAL_MINIMUM_PHP
.I don't think that really addresses @alexpott's deepest concerns with this patch, but maybe it helps a little?
Comment #79
effulgentsia CreditAttribution: effulgentsia at Acquia commentedI think this is @alexpott's main concern with this patch (as opposed to simply upping
DRUPAL_MINIMUM_PHP
to 7.0.8 in 8.7.x).To answer that, I don't see how a site could possibly be better off on 8.6 than on 8.7. So long as we're running any PHP 5 tests on 8.6, why wouldn't we also run them on 8.7? If a vendor library issues an SA and only fixes it in a PHP 7 compatible version, then that affects 8.6 just as much as it affects 8.7. If we backport the fix to the version that's in 8.6, we can also backport it to the version that's in 8.7. If a PHP 5 backport is not feasible, then potentially we update the library in a way that breaks on PHP 5. But if that's the situation we're in, it applies equally to 8.6.
One could potentially argue that a site is equally well off on 8.6 as on 8.7 (for the duration that 8.6 is still security supported), but that's not true for several reasons:
One could still argue that the above benefits aren't big enough to justify us doing the work of this patch, or creating confusion around the difference between "this is supported" vs. "it's not supported, but you can still do it, and we'll try to be courteous about not breaking your stuff without a really good reason". As far as the work of this patch goes, that's already pretty much done, so I don't think that should factor into the decision. As far as the confusion about what we mean by supported vs. still-functional and whether there's enough value in keeping unsupported things functional, I think that's a release management decision, so tagging for that.
Comment #80
tedbowI am new to this patch so I might be missing something but...
I don't think method is actually called from anywhere anymore.
I search my code base after apply the patch this declaration is the only match I find.
If
continueOnExpectedWarnings()
indeed can be removed then assertWarningSummaries in only called once with the argument['PHP']
. if that is case then we don't need the logic around more selectors, correct?Comment #81
catchI think both release managers (i.e. me and xjm) are agreed we need the flexibility to differentiate between these two things - although as above I see this as an implementation detail of the update system rather than any official extra level of support.
Comment #82
effulgentsia CreditAttribution: effulgentsia at Acquia commentedNeeds work for #80. Other than that, given #81, I think this is very close. Possibly committable after #80 is fixed.
Comment #83
plachMinor nit:
What about changing this to "their PHP version", at first sight I thought the comment was referring to the PHP version of the constant being documented.
Comment #84
tedbowOk I was wrong in #80.
Since basically we moved
continueOnExpectedWarnings
from directly being in\Drupal\FunctionalTests\Installer\InstallerTestBase
to being in a trait we can't actually remove it from core. We would have to deprecate them since our BC policy saysSo even though we aren't using it anymore distro probably are.
So that would be out of scope for this issue. Same for
assertWarningSummaries()
. And we can't the method signature just because are using it for just 1 warning.Sorry for the false alarm.
The patch looks good to me. I looked over the test changes.
Comment #85
pandaski CreditAttribution: pandaski at govCMS (Australian Government Department of Finance) commentedThe patch looks good to me. Also, test in 8.8.x
Comment #86
alexpottIt's not just security. It's having DRUPAL_MINIMUM_PHP and thereby our composer.json be at PHP less than what we're running tests on and declaring we support. Even with regular PHP 5 tests we've broken PHP 5 loads of times now that patch testing is done on PHP 7 by default. I feel stupid because I don't understand how having code for allowing updates on PHP 5 and not testing it will lead to a good user experience. Additionally, there are so many workflows that companies have that start from an install - eg the backup and migrate flow, config install etc.. that having different minimum PHPs for update and install feels like inviting trouble.
Comment #87
alexpottSo here's another thing. If we don't change our minimum PHP in composer.json then people will be able to composer install drupal 8.7 on PHP 5 but hey you're not allowed to install it. If we do change on 8.7 then no one will be able to update their code on PHP 5 even though with this patch they would be able to run updates.
Comment #88
pandaski CreditAttribution: pandaski at govCMS (Australian Government Department of Finance) commentedAnd this section is quite confused to the site user
Comment #89
effulgentsia CreditAttribution: effulgentsia at Acquia commentedThis fixes #83 and #88 and adds a comment for #84. Thanks for those reviews!
Comment #90
effulgentsia CreditAttribution: effulgentsia at Acquia commentedThe "Needs tests" tag was added in #9 and referenced again in #57.2. From what I can tell, the two required test cases from #8 are in this patch.
This implements #8.1.
This implements #8.2.
Therefore, removing the "Needs tests" tag.
Comment #91
effulgentsia CreditAttribution: effulgentsia at Acquia commentedCrediting people who've contributed through reviews or significant comments. Thank you!
Comment #92
effulgentsia CreditAttribution: effulgentsia at Acquia commentedEarlier this evening, I discussed this issue at length with @alexpott, given his concerns in #86 and other comments. He continues to believe that we should either fully drop PHP 5 support (by changing DRUPAL_MINIMUM_PHP and our composer.json) in Drupal 8.7, or wait and not do that until Drupal 8.8. He remains skeptical about what anyone really gains by a halfway step of allowing updates but not installs and is concerned about the downsides of such a halfway step (as he expressed in #86 and #87). However, he also accepts that it's a release management decision, so given #81, he told me he's ok with it going in despite his concerns.
Comment #95
effulgentsia CreditAttribution: effulgentsia at Acquia commentedThe interdiffs in #78 and #89 include some not entirely trivial decisions, so given that, this should normally exclude me from RTBC'ing or committing this patch. However, because we're only half a day or so from the beta release, I'm breaking protocol, and pushed this to 8.8.x and 8.7.x.
I'm ok with this, because #84 and #85 provided some additional eyes on this (thank you!), and I think it's better to have a few extra hours where this is in the branches getting whatever additional accidental testing comes from that.
Any committer is free to revert this if they disagree with my decision. Or, we can open a followup to discuss or refine anything that still needs that.
Thanks again for everyone's contribution to this!
Comment #96
Gábor HojtsyLet's document this :)
Comment #97
Gábor HojtsyAlso screenshots would help people (in the change record at least) to see how they can identify if something is wrong.
Comment #98
Gábor HojtsyComment #99
Gábor HojtsyHas all the missing parts now.
Comment #101
pfrenssenI'm having serious trouble with some of my contributed projects because of the way this has been implemented.
I am co-maintainer of the Behat Drupal Extension project. I am starting to implement PHP 7 only features on a number of modules that support Behat which are intended to be released for Drupal 8.8. It is a very frustrating experience to get tests up and running for them. Behat tests are running against an installed Drupal. I cannot do that any more for Drupal 8.7 on PHP 5.6, even though users are still allowed to use my software.
Like @alexpott mentioned in #87 we now have a declaration in composer.json that says that Drupal 8.7 is compatible with PHP 5.6, but this is not correct. It does not resolve into a working package (meaning a package in which I can install Drupal so I can run my tests on them).
The minimum PHP version that is declared in composer.json should not be considered as some kind of suggestion. This is intended to be read by automated systems and result in a working system under all circumstances.
Comment #102
pfrenssenI think the only proper approach to this is to allow to install Drupal 8.7.x on PHP 5.6 but show a warning. Then at least we are honoring the minimum PHP version compatibility as stipulated in composer.json.