Remove try...catch from SessionHandler::write

Great observation! I doubt that we should protect the session subsystem, because when this particular DB write fails, other reads might fail as well for example.

It seems to be that this try catch is inherited from D7: https://api.drupal.org/api/drupal/includes%21session.inc/function/_drupa...

I'm not sure about this because it looks like we're trying to obey PHP's session hander interface...

	 * @return bool
	 * The return value (usually TRUE on success, FALSE on failure).
	 * Note this value is returned internally to PHP for processing.

The patch from #1 miraculously still applies to 8.1.x.

Re-uploading it here for the testbot. Note that this is @znerol's work and not mine.

This is a really nice improvement IMHO. On the other hand I'm wondering, was the php7 failure a random one?

On top of that I'm wondering whether someone actually tried this out, so drop the database table and see what happens. Does maybe the logging system maybe rely on some session stuff?

This patch applies at 8.3.x too. Bumping up the version.

@anish.a
If a patch still applied there is no reason to reupload it :) Just trigger the retesting and be done with it.

One thing we maybe could do to reduce the maybe potential risk of this patch is to move the exception handler registration further up in \Drupal\Core\DrupalKernel::bootEnvironment basically to the top. The earlier, especially before the test setup, the better.

I'm not sure about this because it looks like we're trying to obey PHP's session hander interface...

* @return bool
* The return value (usually TRUE on success, FALSE on failure).
* Note this value is returned internally to PHP for processing.

+++ b/core/lib/Drupal/Core/Session/SessionHandler.php
@@ -73,32 +73,18 @@ public function read($sid) {
+    $request = $this->requestStack->getCurrentRequest();
+    $fields = array(
+      'uid' => $request->getSession()->get('uid', 0),
+      'hostname' => $request->getClientIP(),
+      'session' => $value,
+      'timestamp' => REQUEST_TIME,
+    );
+    $this->connection->merge('sessions')
+      ->keys(array('sid' => Crypt::hashBase64($sid)))
+      ->fields($fields)
+      ->execute();
+    return TRUE;

After removal, it always returns TRUE, looking at the code from the patch in #10, the database operation can go wrong, which may throw an exception and return FALSE accordingly. So proposing the following, then the all possible return values -- TRUE/FALSE of SessionHandlerInterface::write() get respected:

public function write($sid, $value) {
    $request = $this->requestStack->getCurrentRequest();
    $fields = [
      'uid' => $request->getSession()->get('uid', 0),
      'hostname' => $request->getClientIP(),
      'session' => $value,
      'timestamp' => \Drupal::time()->getRequestTime(),
    ];
    try {
      $this->connection->merge('sessions')
        ->keys(['sid' => Crypt::hashBase64($sid)])
        ->fields($fields)
        ->execute();
      return TRUE;
    }
    catch (\Exception $exception) {
      return FALSE;
    }
  }

(Rerolled the patch for 9.1.x, meanwhile, made the change proposed, no interdiff)

1. Removed an unused use statement

2. +++ b/core/lib/Drupal/Core/Session/SessionHandler.php
   @@ -68,16 +68,14 @@ public function read($sid) {
   +      'timestamp' => \Drupal::time()->getRequestTime(),
   

   Use IoC, not sure if we need a BC layer. so used a placeholder CR URL in @trigger_error in the attached patch.

I reviewed the patch and found one nit although I can't comment on the validity of what this patch is doing. Also, does this need a test?

+++ b/core/lib/Drupal/Core/Session/SessionHandler.php
@@ -37,10 +44,17 @@ class SessionHandler extends AbstractProxy implements \SessionHandlerInterface {
+   * @param \Drupal\Component\Datetime\TimeInterface $time

Needs a 'null' added, \Drupal\Component\Datetime\TimeInterface|null

Thanks @quietone! Nice catch, tagging "Needs tests".

__NAMESPACE__ . '\SessionHandler::__construct() could use __METHOD__ instead

This includes the suggestions mentioned above, this still needs a testcase.

+++ b/core/lib/Drupal/Core/Session/SessionHandler.php
@@ -37,10 +44,17 @@ class SessionHandler extends AbstractProxy implements \SessionHandlerInterface {
+      @trigger_error('Calling ' . __METHOD__ . ' without the $time argument is deprecated in drupal:9.1.0 and will be required in drupal:10.0.0. See https://www.drupal.org/node/TODO', E_USER_DEPRECATED);

should be 9.3.0 and will be required needs change to is required

Re #21 and the change for REQUEST_TIME. That's not in scope here. This issue is about removing the try / catch and not about replacing REQUEST_TIME.

Also the change to catch the exception and return FALSE and then remove all logging is not correct - that will eat the errors even more than HEAD. An exception is not a return value it's an unrecoverable error - returning FALSE at this point does not matter - we expect this to fail. FALSE will mean the system soldiers on but we've failed.

One thing that is interesting is that if this returns FALSE then PHP will write to the log... see https://github.com/cakephp/cakephp/issues/13581 for a very similar discussion for another CMS.

Not sure what the correct thing to do here is. It'd be good to add a test to see what happens if the sessions table is dropped mid request and we try to write to it.

Was previously tagged for a test which still needs to happen I believe.

Will add related issue to my review list.

Seems to have a test failure.

Perfect LGTM

Test coverage looks good. Read back through the discussion and just removing the try/catch seems right here, anything else can happen in other issues. Committed ba5d338 and pushed to 11.x. Thanks!