PHP 7.3 compatibility

The 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.

+++ b/core/lib/Drupal/Core/Session/SessionManager.php
@@ -218,6 +218,7 @@ public function regenerate($destroy = FALSE, $lifetime = NULL) {
     if ($this->isStarted()) {
       $old_session_id = $this->getId();
+      session_destroy();
     }
     session_id(Crypt::randomBytesBase64());

So 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.

So 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.

Let'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.

I'm not convinced we're regenerating the session ID correctly. Reading the SYmfony docs it says

     * This method must invoke session_regenerate_id($destroy) unless
     * this interface is used for a storage object designed for unit
     * or functional testing where a real PHP session would interfere
     * with testing.

We're not calling session_regenerate_id() - we're doing session_id(Crypt::randomBytesBase64()); which seems wrong. PHP will assign a random session ID and we should be using session_regenerate_id() to generate a new ID for an existing session.

@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 and Drupal\Tests\migrate\Unit\MigrationPluginManagerTest are not happening locally for me on PHP 7.3.0beta3.

Working on the testbot fail in Drupal\KernelTests\Core\Entity\EntityQueryTest...

    // Calculate the cartesian product of the unit array by looking at the
    // bits of $i and add the unit at the bits that are 1. For example,
    // decimal 13 is binary 1101 so unit 3,2 and 0 will be added to the
    // entity.

Not the simplest thing to wokr out how PHP 7.3 might be messing this up :) ... and only on testbot.

+++ b/core/tests/Drupal/KernelTests/Core/Entity/EntityQueryTest.php
@@ -139,6 +139,7 @@ protected function setUp() {
           list($field_name, $langcode, $values) = $units[$key];
+          $this->assertTrue($entity->hasTranslation($langcode), "Entity has translation. Key $key Value: " . print_r($units[$key], TRUE));
           $entity->getTranslation($langcode)->{$field_name}[] = $values;

Uhoh - 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.

oh nice as @Berdir put it... we have a Heisenbug :(

Discussed 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

@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.

@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().

This should pass Drupal\Tests\Component\Serialization\YamlPeclTest

I've got a workaround for the EntityQueryTest - perhaps we can re-purpose #3001920: Investigate PHP 7.3 workarounds to remove PHP 7.3 workarounds.

So 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.

Crediting @mikelutz and @tacituseu for their work on #3001920: Investigate PHP 7.3 workarounds which has helped get this (hopefully) green.

+++ b/core/lib/Drupal/Core/Session/SessionManager.php
@@ -218,8 +218,8 @@ public function regenerate($destroy = FALSE, $lifetime = NULL) {
     if ($this->isStarted()) {
       $old_session_id = $this->getId();
+      session_regenerate_id($destroy);
     }
-    session_id(Crypt::randomBytesBase64());

This 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.

Been 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.

@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.

Fingers-crossed this should be green.

Ho hum. I was wrong. Maybe this one...

+++ b/core/includes/install.core.inc
@@ -1566,6 +1566,8 @@ function install_bootstrap_full() {
+  // Set a value to ensure the session is maintained throughout the install.
+  $session->set('_drupal_installing', TRUE);

@@ -1879,6 +1881,9 @@ function install_finished(&$install_state) {
+  // Clean up the session flag.
+  \Drupal::service('session')->remove('_drupal_installing');

I'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.

I 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.

Note 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

Nothing has changed that anyone needs to react to as far as I know.

• The session changes should be transparent.
• The serialization trait being used by PrivateTempStore is not an API change
• Everything else are workarounds for things that have been deprecated in PHP 7.3 or are considered errors so if contrib or custom does this they'll get the same warnings a core got.

Or do we need a change record to say that we support PHP 7.3?