Various methods of \Drupal\Core\Database\StatementInterface have changed their signature for PHP 8

se/StatementInterface.php
+++ b/core/lib/Drupal/Core/Database/StatementInterface.php
@@ -109,7 +109,7 @@ public function setFetchMode($mode, $a1 = NULL, $a2 = []);

@@ -109,7 +109,7 @@ public function setFetchMode($mode, $a1 = NULL, $a2 = []);
    * @return
    *   A result, formatted according to $mode.
    */
-  public function fetch($mode = NULL, $cursor_orientation = NULL, $cursor_offset = NULL);
+  public function fetch(int $mode = \PDO::FETCH_BOTH, int $cursor_orientation = \PDO::FETCH_ORI_NEXT, int $cursor_offset = 0);
 

Should we remove this method altogether so that it just inherits from PHP?

1. +++ b/core/lib/Drupal/Core/Access/CsrfTokenGenerator.php
   @@ -86,8 +86,14 @@ public function validate($token, $value = '') {
   +    $value = $this->computeToken($seed, $value);
   ...
   -    return hash_equals($this->computeToken($seed, $value), $token);
   +    return hash_equals($value, $token);
   

   We can just remove these changes from the patch. The method computeToken() does not change the value of the variable $token.

2. This change will be a BC for every contrib database driver that overrides the method fetch(). I am not if/how we can fix that.

+++ b/core/lib/Drupal/Core/Access/CsrfTokenGenerator.php
@@ -86,8 +86,14 @@ public function validate($token, $value = '') {
+    // PHP 8.0 strictly typehints for hash_equals. Maintain BC until we can
+    // enforce scalar typehints on this method.
+    if (!is_string($token)) {
+      return FALSE;
+    }

We should add testing for this change.

There are related changes in PDOStatement which were introduced in PHP 8.0.0beta3. These are the two commits which made the relevant changes:

• https://github.com/php/php-src/commit/cfaa270da5b511e2821aa0a5e036ef5879...
• https://github.com/php/php-src/commit/7d3e530f4e8700a4cdfc4ff2a747d9dcb7...

I think it makes sense to include these changes here as well. Hence, renaming the issue for clarity.

I noticed a few issues during manual testing. It was funny how such a critical error was not visible except in dynamic routes.

Hopefully, this workaround should be fine to fix the problem.

It looks big API change for db layer, so could use wait til RC1 of PHP 8 but ++ to keep it single issue

I agree. Case in point: The site installed and ran well when I forgot to use the new StatementTrait in the \Drupal\Core\Database\Statement class. I then realised I was missing it. I added it and created the patch in #13 (which failed manual testing).

I wonder if it would be better to refactor after making it work with PHP 8 or how much refactoring would even be possible in the current release cycle. I would at least want to move away from the current func_get_args() logic which now gets repeated in multiple places in the patch.

+++ b/core/lib/Drupal/Core/Database/Statement.php
@@ -2,6 +2,13 @@
+if (PHP_VERSION_ID >= 80000) {
+  class_alias('\Drupal\Core\Database\Php8StatementTrait', '\Drupal\Core\Database\StatementTrait');
+}
+else {
+  class_alias('\Drupal\Core\Database\Php7StatementTrait', '\Drupal\Core\Database\StatementTrait');
+}
+

@@ -14,6 +21,7 @@
+  use StatementTrait;

Clever! I agree this is not easy to maintain per say, but we would need to do the same in Drupal 8 and also to keep BC in 9. We can get rid of it in Drupal 10 ;) Should we include our own deprecation here of the old interfaces that we exposed, targeting Drupal 10?

@alexpott, re #21 and #22, you're right. This seems a little unintuitive (especially the fact that this happens twice in case of \Drupal\Core\Database\Statement) but it's required. The comment explains why and not having this resulted in the bugs I mentioned where routes with dynamic parameters wouldn't work.

Sidenote about that issue: It's weird that a PDO behaviour would cause impact to dynamic parameters in routes but this happens because a PDO exception is swallowed in \Drupal\Core\Routing\RouteProvider::getRoutesByPath.

@Gábor Hojtsy, re: #20

We can get rid of it in Drupal 10 ;) Should we include our own deprecation here of the old interfaces that we exposed, targeting Drupal 10?

Removing this in Drupal 10 would mean that we would also have to remove PHP 7.4 support. We'll have to refactor the methods into new ones and then deprecate existing methods. But that would be tricky too as these are actually concrete methods in \PDOStatement and we extend that class.

@hussainweb: PHP 7.4 is the last version series of PHP 7 and is EOL is a few months after the release of Drupal 10 (https://www.php.net/supported-versions.php). The current plan in #3118147: [meta] Set Drupal 10 platform and browser requirements six months before the release is to require PHP 8 in Drupal 10.x as soon as the branch opens for development. That would be too late to deprecate anything for Drupal 10. I don't know how to make sure we revisit this as a possible deprecation once the PHP requirement is decided but soon enough so contrib can act on it.

Added a change record at https://www.drupal.org/node/3170913, the issue summary totally needs updating though. I am not sure I am the best person to do that. Also need reviews on the change record :D

Updated the issue summary as well (please verify). Also added a release notes snippet, as this kind of change cannot go without one. Will need a release notes tag once we know which version it lands in.

+++ b/core/lib/Drupal/Core/Database/Statement.php
@@ -2,6 +2,13 @@
+if (PHP_VERSION_ID >= 80000) {
+  class_alias('\Drupal\Core\Database\Php8StatementTrait', '\Drupal\Core\Database\StatementTrait');
...
+else {
+  class_alias('\Drupal\Core\Database\Php7StatementTrait', '\Drupal\Core\Database\StatementTrait');

+++ b/core/lib/Drupal/Core/Database/StatementEmpty.php
@@ -2,6 +2,13 @@
+if (PHP_VERSION_ID >= 80000) {
+  class_alias('\Drupal\Core\Database\Php8StatementTrait', '\Drupal\Core\Database\StatementTrait');
...
+else {
+  class_alias('\Drupal\Core\Database\Php7StatementTrait', '\Drupal\Core\Database\StatementTrait');

+++ b/core/lib/Drupal/Core/Database/StatementInterface.php
@@ -2,6 +2,13 @@
+if (PHP_VERSION_ID >= 80000) {
+  class_alias('\Drupal\Core\Database\Php8StatementInterface', '\Drupal\Core\Database\StatementInterfaceBase');
...
+else {
+  class_alias('\Drupal\Core\Database\Php7StatementInterface', '\Drupal\Core\Database\StatementInterfaceBase');

+++ b/core/lib/Drupal/Core/Database/StatementPrefetch.php
@@ -2,6 +2,13 @@
+  class_alias('\Drupal\Core\Database\Php8StatementTrait', '\Drupal\Core\Database\StatementPrefetchTrait');
...
+  class_alias('\Drupal\Core\Database\Php7StatementTrait', '\Drupal\Core\Database\StatementPrefetchTrait');

Why they are not using ::class - from PHP POV strings are slower and code should prevent using duplicated strings in the same file

I think we can deprecate for Drupal 10 now on the assumption we require PHP 8, and we always have the option of updating the deprecation to Drupal 11 for whatever reason later.

+1 on #35. I filed #3174662: Encapsulate \PDOStatement instead of extending from it to (potentially) address the root cause.

Added https://github.com/php/php-src/pull/6220 to IS

#3174662: Encapsulate \PDOStatement instead of extending from it is looking very viable, so going to mark this postponed - we can mark duplicate once that issue lands, or re-open here if there's a nasty surprise.

One more change in PDO parameters https://github.com/php/php-src/pull/6272

The alternative approach in #3174662: Encapsulate \PDOStatement instead of extending from it was committed.

FYI this cropped up for me today, and I beat my head on it for an hour before I realized what happened (and thought I would share here). We updated an environment to PHP 8.0 and Drupal 9.3.x. Except, that the deployment of 9.3.x silently failed and we were actually still running Drupal 8.9.x on a PHP 8.0 server (which obviously isn't going to work). So, if you are hitting this, definitely take the 30 seconds required to 100% confirm that you are running Drupal 9.1.x or newer (which actually supports PHP 8.0)!