Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Problem/Motivation
PHP 7.3 will released late 2018. We should be compatible the moment it lands.
Errors fixed:
- Warning: "continue" targeting switch is equivalent to "break". Did you mean to use "continue 2"? in /var/www/html/core/modules/file/file.module
- PHPUnit\Framework\Exception: PHP Warning: "continue" targeting switch is equivalent to "break". Did you mean to use "continue 2"? in /var/www/html/core/tests/Drupal/KernelTests/AssertConfigTrait.php
- Exception: Warning: session_id(): Cannot change session id when session is active Drupal\Core\Session\SessionManager->regenerate()() (Line: 222)
Proposed resolution
Fix all test failures on PHP 7.3. Some workarounds for PHP 7.3 bugs are necessary. Resolving these is tracked in #3001920: Investigate PHP 7.3 workarounds
Remaining tasks
User interface changes
None
API changes
None
Data model changes
None
Comments
Comment #2
alexpottThe one that seems to be causing the most warnings is https://wiki.php.net/rfc/continue_on_switch_deprecation and this reveals that file.module probably is not working as intended.
Comment #4
mondrakeAdding related issue
Comment #5
alexpottSo this is not going to work :( The only reliable solution I've found so far is not migrating the session during an install. Which feels really bad.
Comment #6
alexpottSo here's the bad fix for the session issue. I think we might need to work on #2238561: Use the default PHP session ID instead of generating a custom one
And also weirdly
$this->getSession()->getPage()->getHtml()
in tests does not work but$this->getSession()->getPage()->getContent()
does. I've not dug into why and where the problem lies.Comment #8
dpiComment #9
dpiFail
Comment #10
Ayesh CreditAttribution: Ayesh commentedComment #11
alexpottLet's remove the session changes and see where we are at. PHP 7.3 has some changes in the beta that have fixed a few errors as far as I can see.
Comment #12
alexpottI'm not convinced we're regenerating the session ID correctly. Reading the SYmfony docs it says
We're not calling
session_regenerate_id()
- we're doingsession_id(Crypt::randomBytesBase64());
which seems wrong. PHP will assign a random session ID and we should be usingsession_regenerate_id()
to generate a new ID for an existing session.Comment #13
Berdir@alexpott: That sounds very related to #2238561: Use the default PHP session ID instead of generating a custom one, we always set our session ID ourself currently. And according to the patch in the other issue, that whole code block only exists because we do things our way.
Comment #14
alexpott@Berdir thanks for finding that issue and yes it does seem very related. I think the approach here is okay to get PHP 7.3 compatibility and then we can deal with the bigger issue of what code do we actually still need there. With the current patch we use PHP's inbuilt session ID creation which according to that issue is as security as what we do in HEAD.
The migrate fails in #12appear to be a regression introduced in PHP 7.3 - there's something that we can serialize and unserialize in PrivateTempStore that works on 7.2 but not in 7.3. However the bigger question is why are we serializing so much in the batch. I've been try to extract the bit that is causing the fail to open a sensible bug report on bugs.php.net but not managed to so far. But our other option here is to use DependencySerializationTrait in PrivateTempStore which stops this from happening and seems very sensible to me.
The fails in
Drupal\KernelTests\Core\Entity\EntityQueryTest
andDrupal\Tests\migrate\Unit\MigrationPluginManagerTest
are not happening locally for me onPHP 7.3.0beta3
.Comment #15
alexpottWorking on the testbot fail in
Drupal\KernelTests\Core\Entity\EntityQueryTest
...Not the simplest thing to wokr out how PHP 7.3 might be messing this up :) ... and only on testbot.
Comment #16
alexpottUhoh - how has the act of checking for the language fixed the test? That's not fair. Re-uploading the patch from #14 to make sure it's not something fixed the PHP7.3 build.
Comment #17
alexpottoh nice as @Berdir put it... we have a Heisenbug :(
Comment #18
alexpottDiscussed with @catch. We agreed that as PHP 7.3 release is imminent and the beta is out now we could proceed with this issue as it is and handle the other two fails in a followup. This means that 8.6.2 will hopefully support PHP 7.3 without warnings.
Opened #3001920: Investigate PHP 7.3 workarounds
Comment #19
catchThe session changes here are much more minimal than the ones in #2238561: Use the default PHP session ID instead of generating a custom one, which additionally removes some other @todos and workarounds. Is there a reason not to bump that issue and get it done, or are you deliberately making a smaller change and leaving that as a follow-up? I can see arguments for both..
Everything else looks straightforward.
Comment #20
alexpott@catch I deliberately made the smallest change. And yeah this is the most tricky part of this patch. I played with #2238561: Use the default PHP session ID instead of generating a custom one yesterday to try to bump that one in front. It appears that not setting the session id causes some problems for our installer. So this patch would put us in the interesting state of assigning our own session ID unless we migrate sessions and regenerate one when we'll let PHP do the generation. On one hand I don't think this really matters, on the other it feels very inconsistent. I'll try and work on #2238561: Use the default PHP session ID instead of generating a custom one and see what I can work out - because I think getting it done is the right course but to be pragmatic if we haven't got it done and in 8.6.2 then we should consider going forward here without it.
Comment #21
tacituseu CreditAttribution: tacituseu commentedThere's also a
PDOException
generated byDrupal\Tests\Component\Serialization\YamlPeclTest
(see console log).Comment #22
alexpott@tacituseu that'll be because the test is skipped because there is no yaml extension built. So there is no file :) - we can fix that in simpletest_phpunit_testcase_to_row().
Comment #23
alexpottThis should pass
Drupal\Tests\Component\Serialization\YamlPeclTest
Comment #24
alexpottI've got a workaround for the EntityQueryTest - perhaps we can re-purpose #3001920: Investigate PHP 7.3 workarounds to remove PHP 7.3 workarounds.
Comment #25
alexpottSo the fail in \Drupal\Tests\migrate\Unit\MigrationPluginManagerTest is actual a real fail. We should be guaranteeing a stable sort but it's not. So let's make it a stable sort so migrations will run in the same order regardless of PHP version.
Comment #27
alexpottCrediting @mikelutz and @tacituseu for their work on #3001920: Investigate PHP 7.3 workarounds which has helped get this (hopefully) green.
Comment #28
alexpottComment #29
mondrakeTested patch in #25 on Travis CI for the Imagemagick module with
Drupal 8.7.x
PHP 7.4.0-dev
PHPUnit 6.5.13
and tests run smoothly https://travis-ci.org/mondrake/imagemagick/jobs/440495985
Without the patch, they were failing https://travis-ci.org/mondrake/imagemagick/jobs/440492343
Setting to RTBC, tentatively. IDNK is there anything left to do here?
Comment #30
alexpottThis is the only tricky change because it totally changes how our session IDs are generated. We need a decent CR for this explaining we now obey PHP's session generation in certain circumstances. But also we need to consider whether this is sane or if the disruption caused by #2238561: Use the default PHP session ID instead of generating a custom one is preferable.
Comment #31
Gábor HojtsyLet's make it clear in status that this needs more discussion.
Comment #32
DamienMcKennaShouldn't this go in 8.7 first and then be backported to 8.6?
Comment #33
alexpottBeen thinking about this some more I think we can do this without changing how the session ID is regenerated. It might involve an extra write to the DB if the session already exists which I think is rare - anonymous users normally do not have a session - a case that comes to mind with core is after an install but one extra db operation there is hardly a problem.
Comment #34
alexpott@DamienMcKenna yep this has to go in 8.7 first but it is targeting 8.6.x because PHP 7.3 is out in November way before 8.7.0.
Comment #36
alexpottFingers-crossed this should be green.
Comment #38
alexpottHo hum. I was wrong. Maybe this one...
Comment #39
catchNice find with the session change, that looks a lot less intrusive than both the earlier changes here and the other issue.
Comment #40
alexpottI'd love to understand what is happening with a multi-lingual install that makes this necessary. On one level fixing the session during an install makes heaps of sense but on another that fact this change makes this necessary is worrying. The test that fails without this code is:
Drupal\FunctionalTests\Installer\InstallerExistingConfigSyncDirectoryMultilingualTest
.Will try looking into this. But tracking down this stuff in the installer is hard.
Comment #41
andypost@alexpott I bet multilingual needs rebuild container and download translation in batch, which in turn needs session
Comment #42
alexpottI think I understand it. So with the multi-lingual install we have a proper multi-stage batch process during the install just before logging the user in. When this happens we declare the session obsolete during the regeneration and destroy it instead of saving it. That's not what we should be doing because then \Drupal\Core\Session\SessionManager::migrateStoredSession() has nothing to do. Here's a patch the removes the changes to the installer in favour of ensuring we don't destroy a session during session regeneration.
Comment #43
alexpottNote wrt to multi-lingual install and session we've had a very similar issue in the past. See #2108623: Installing via the UI no longer logs in user 1 every time
Comment #44
jibranLet get this in. Do we need a change record for this? Maybe a g.d.o/core post as well.
Comment #45
andypost@alexpott yes, that explained in #2108623-23: Installing via the UI no longer logs in user 1 every time
I guess it needs follow-up to investigate #14
Comment #46
andypostFiled follow-up #3011684: Properly serialize \Drupal\Core\TempStore\PrivateTempStore
Comment #47
catchThat looks much better.
Committed 39c3361 and pushed to 8.7.x. Thanks!
Looks looks backportable to 8.6.x, however definitely not the day before a release window. So leaving RTBC there.
Comment #49
xjmCan we get a change record please? Thanks!
Comment #50
alexpottNothing has changed that anyone needs to react to as far as I know.
Or do we need a change record to say that we support PHP 7.3?
Comment #51
Wim LeersNot sure what we've done in the past (for PHP 7.2 f.e.), but that sure sounds helpful and valuable?
Comment #52
catchThat makes sense to me too, posted one here: https://www.drupal.org/node/3011831
Added a note in the CR to say that people should consult this issue to see what changes core had to make.
Back to RTBC for 8.6.x
Comment #54
larowlanC/p as 5d17da1f31 and pushed to 8.6.x
Comment #56
hchonovBoth in the change record and in the documentation only Drupal 8.7.x is listed as supporting PHP 7.3.
See https://www.drupal.org/docs/8/system-requirements/php-requirements and https://www.drupal.org/node/3011831 .
However the issue has been backported to 8.6x. and is now included in 8.6.4. I've updated the documentation to include the 8.6.4 version. Is it fine to update the CR as well?
Comment #57
hansfn CreditAttribution: hansfn commented@hchonov: Thx for updating the documentation. It was based on the CR. I wasn't aware that this was backported to 8.6.x - which is great news by the way.
Comment #58
mfbIt seems like we're currently in a state where some services work on PHP 7.3 but some don't - e.g. PrivateTempStore was fixed here, but SharedTempStore was not.
Comment #59
mfbComment #60
klausiOpened #3072872: Upgrade zendframework/zend-feed for PHP 7.3 compatibility as follow-up.
Comment #61
liquidcms CreditAttribution: liquidcms commenteddeleted.