From 2efb494f0335c3d505d777695d0d2266456b325f Mon Sep 17 00:00:00 2001
From: Brent Shaffer <betterbrent@google.com>
Date: Tue, 20 Aug 2019 15:55:00 -0700
Subject: [PATCH 01/20] adds Firestore session handler

---
 Firestore/src/FirestoreSessionHandler.php     | 265 ++++++++++++
 .../Unit/FirestoreSessionHandlerTest.php      | 389 ++++++++++++++++++
 2 files changed, 654 insertions(+)
 create mode 100644 Firestore/src/FirestoreSessionHandler.php
 create mode 100644 Firestore/tests/Unit/FirestoreSessionHandlerTest.php

diff --git a/Firestore/src/FirestoreSessionHandler.php b/Firestore/src/FirestoreSessionHandler.php
new file mode 100644
index 000000000000..0da31c792a70
--- /dev/null
+++ b/Firestore/src/FirestoreSessionHandler.php
@@ -0,0 +1,265 @@
+<?php
+/**
+ * Copyright 2019 Google LLC
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *      http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+namespace Google\Cloud\Firestore;
+
+use Exception;
+use InvalidArgumentException;
+use SessionHandlerInterface;
+
+/**
+ * Custom session handler backed by Cloud Firestore.
+ *
+ * Instead of storing the session data in a local file, it stores the data to
+ * Cloud Firestore. The biggest benefit of doing this is the data can be
+ * shared by multiple instances, so it's suitable for cloud applications.
+ *
+ * The downside of using Cloud Firestore is the write operations will cost you
+ * some money, so it is highly recommended to minimize the write operations
+ * with your session data with this handler. In order to do so, keep the data
+ * in the session as limited as possible; for example, it is ok to put only
+ * signed-in state and the user id in the session with this handler. However,
+ * for example, it is definitely not recommended that you store your
+ * application's whole undo history in the session, because every user
+ * operations will cause the Firestore write and then it will cost you lot of
+ * money.
+ *
+ * If you are building an AJAX application which may issue multiple requests
+ * to the server, please design the session data carefully, in order to avoid
+ * possible data contentions. Also please see the 2nd example below for how to
+ * properly handle errors on `write` operations.
+ *
+ * It uses the session.save_path as the Firestore namespace for isolating the
+ * session data from your application data, it also uses the session.name as
+ * the Firestore kind, the session id as the Firestore id. By default, it
+ * does nothing on gc for reducing the cost. Pass positive value up to 1000
+ * for $gcLimit parameter to delete entities in gc.
+ *
+ *
+ * Example without error handling:
+ * ```
+ * use Google\Cloud\Firestore\FirestoreClient;
+ *
+ * $firestore = new FirestoreClient(['projectId' => $projectId]);
+ *
+ * $handler = new FirestoreSessionHandler($firestore);
+ *
+ * session_set_save_handler($handler, true);
+ * session_save_path('sessions');
+ * session_start();
+ *
+ * // Then read and write the $_SESSION array.
+ *
+ * ```
+ *
+ * The above example automatically writes the session data. It's handy, but
+ * the code doesn't stop even if it fails to write the session data, because
+ * the `write` happens when the code exits. If you want to know the session
+ * data is correctly written to the Firestore, you need to call
+ * `session_write_close()` explicitly and then handle `E_USER_WARNING`
+ * properly like the following example.
+ *
+ * Example with error handling:
+ *
+ * ```
+ * use Google\Cloud\Firestore\FirestoreClient;
+ *
+ * $firestore = new FirestoreClient(['projectId' => $projectId]);
+ *
+ * $handler = new FirestoreSessionHandler($firestore);
+ * session_set_save_handler($handler, true);
+ * session_save_path('sessions');
+ * session_start();
+ *
+ * // Then read and write the $_SESSION array.
+ *
+ * function handle_session_error($errNo, $errStr, $errFile, $errLine) {
+ *     # We throw an exception here, but you can do whatever you need.
+ *     throw new Exception("$errStr in $errFile on line $errLine", $errNo);
+ * }
+ * set_error_handler('handle_session_error', E_USER_WARNING);
+ * // If `write` fails for any reason, an exception will be thrown.
+ * session_write_close();
+ * restore_error_handler();
+ * // You can still read the $_SESSION array after closing the session.
+ * ```
+ * @see http://php.net/manual/en/class.sessionhandlerinterface.php SessionHandlerInterface
+ */
+class FirestoreSessionHandler implements SessionHandlerInterface
+{
+    /* @var int */
+    private $gcLimit;
+    /* @var FirestoreClient */
+    private $firestore;
+    /* @var CollectionReference */
+    private $collection;
+
+    /**
+     * Create a custom session handler backed by Cloud Firestore.
+     *
+     * @param FirestoreClient $firestore Firestore client.
+     * @param int $gcLimit [optional] A number of entities to delete in the
+     *        garbage collection.  Defaults to 0 which means it does nothing.
+     *        The value larger than 1000 will be cut down to 1000.
+     */
+    public function __construct(
+        FirestoreClient $firestore,
+        $gcLimit = 0
+    ) {
+        $this->firestore = $firestore;
+        // Cut down to 1000
+        $this->gcLimit = min($gcLimit, 1000);
+    }
+
+    /**
+     * Start a session, by getting the Firebase collection from $savePath.
+     *
+     * @param string $savePath The value of `session.save_path` setting will be
+     *        used here as the Firestore collection ID.
+     * @param string $sessionName The value of `session.name` setting will be
+     *        used here as the Firestore document ID prefix (ex: "PHPSESSID:").
+     * @return bool
+     */
+    public function open($savePath, $sessionName)
+    {
+        if (false !== strpos($savePath, '/')) {
+            throw new InvalidArgumentException(
+                sprintf('The given save_path "%s" not allowed', $savePath)
+            );
+        }
+        $this->sessionName = $sessionName;
+        $this->collection = $this->firestore->collection($savePath);
+        return true;
+    }
+
+    /**
+     * Just return true for this implementation.
+     */
+    public function close()
+    {
+        return true;
+    }
+
+    /**
+     * Read the session data from Cloud Firestore.
+     *
+     * @param string $id Identifier used for the session.
+     * @return string
+     */
+    public function read($id)
+    {
+        try {
+            $docRef = $this->collection->document($this->formatId($id));
+            $snapshot = $docRef->snapshot();
+            if ($snapshot->exists() && isset($snapshot['data'])) {
+                return $snapshot->get('data');
+            }
+        } catch (Exception $e) {
+            trigger_error(
+                sprintf('Firestore lookup failed: %s', $e->getMessage()),
+                E_USER_WARNING
+            );
+        }
+        return '';
+    }
+
+    /**
+     * Write the session data to Cloud Firestore.
+     *
+     * @param string $id Identifier used for the session.
+     * @param string $data The session data to write to the Firestore document.
+     * @return bool
+     */
+    public function write($id, $data)
+    {
+        try {
+            $docRef = $this->collection->document($this->formatId($id));
+            $docRef->set([
+                'data' => $data,
+                't' => time()
+            ]);
+        } catch (Exception $e) {
+            trigger_error(
+                sprintf('Firestore upsert failed: %s', $e->getMessage()),
+                E_USER_WARNING
+            );
+            return false;
+        }
+        return true;
+    }
+
+    /**
+     * Delete the session data from Cloud Firestore.
+     *
+     * @param string $id Identifier used for the session
+     * @return bool
+     */
+    public function destroy($id)
+    {
+        try {
+            $this->collection->document($this->formatId($id))->delete();
+        } catch (Exception $e) {
+            trigger_error(
+                sprintf('Firestore delete failed: %s', $e->getMessage()),
+                E_USER_WARNING
+            );
+            return false;
+        }
+        return true;
+    }
+
+    /**
+     * Delete the old session data from Cloud Firestore.
+     *
+     * @param int $maxlifetime Remove all session data older than this number
+     *        in seconds.
+     * @return bool
+     */
+    public function gc($maxlifetime)
+    {
+        if (0 === $this->gcLimit) {
+            return true;
+        }
+        try {
+            $query = $this->collection
+                ->limit($this->gcLimit)
+                ->orderBy('t')
+                ->where('t', '<', time() - $maxlifetime);
+            foreach ($query->documents() as $snapshot) {
+                $snapshot->reference()->delete();
+            }
+        } catch (Exception $e) {
+            trigger_error(
+                sprintf('Session gc failed: %s', $e->getMessage()),
+                E_USER_WARNING
+            );
+            return false;
+        }
+        return true;
+    }
+
+    /**
+     * Format the Firebase document ID from the PHP session ID and session name.
+     * ex: PHPSESSID:abcdef
+     *
+     * @param string $id Identifier used for the session
+     * @return string
+     */
+    private function formatId($id)
+    {
+        return sprintf('%s:%s', $this->sessionName, $id);
+    }
+}
diff --git a/Firestore/tests/Unit/FirestoreSessionHandlerTest.php b/Firestore/tests/Unit/FirestoreSessionHandlerTest.php
new file mode 100644
index 000000000000..bb8f98955011
--- /dev/null
+++ b/Firestore/tests/Unit/FirestoreSessionHandlerTest.php
@@ -0,0 +1,389 @@
+<?php
+/**
+ * Copyright 2019 Google LLC
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+namespace Google\Cloud\Firestore\Tests\Unit;
+
+use Exception;
+use Google\Cloud\Firestore\FirestoreClient;
+use Google\Cloud\Firestore\FirestoreSessionHandler;
+use Google\Cloud\Firestore\CollectionReference;
+use Google\Cloud\Firestore\DocumentReference;
+use Google\Cloud\Firestore\DocumentSnapshot;
+use InvalidArgumentException;
+use Prophecy\Argument;
+use PHPUnit\Framework\TestCase;
+
+/**
+ * @group firestore
+ */
+class FirestoreSessionHandlerTest extends TestCase
+{
+    const SESSION_SAVE_PATH = 'sessions';
+    const SESSION_NAME = 'PHPSESID';
+
+    private $firestore;
+    private $collection;
+
+    public function setUp()
+    {
+        $this->firestore = $this->prophesize(FirestoreClient::class);
+        $this->collection = $this->prophesize(CollectionReference::class);
+    }
+
+    public function testOpen()
+    {
+        $this->firestore->collection(self::SESSION_SAVE_PATH)
+            ->shouldBeCalledTimes(1)
+            ->willReturn($this->collection->reveal());
+        $firestoreSessionHandler = new FirestoreSessionHandler(
+            $this->firestore->reveal()
+        );
+        $ret = $firestoreSessionHandler->open(self::SESSION_SAVE_PATH, self::SESSION_NAME);
+        $this->assertTrue($ret);
+    }
+
+    /**
+     * @expectedException InvalidArgumentException
+     */
+    public function testOpenNotAllowed()
+    {
+        $this->firestore->collection(self::SESSION_SAVE_PATH)
+            ->shouldNotBeCalled()
+            ->willReturn($this->collection->reveal());
+        $firestoreSessionHandler = new FirestoreSessionHandler(
+            $this->firestore->reveal()
+        );
+        $firestoreSessionHandler->open('/tmp/sessions', self::SESSION_NAME);
+    }
+
+
+    public function testClose()
+    {
+        $firestoreSessionHandler = new FirestoreSessionHandler(
+            $this->firestore->reveal()
+        );
+        $ret = $firestoreSessionHandler->close();
+        $this->assertTrue($ret);
+    }
+
+    public function testReadNothing()
+    {
+        $this->firestore->collection(self::SESSION_SAVE_PATH)
+            ->shouldBeCalledTimes(1)
+            ->willReturn($this->collection->reveal());
+        $snapshot = $this->prophesize(DocumentSnapshot::class);
+        $snapshot->exists()
+            ->shouldBeCalledTimes(1)
+            ->willReturn(false);
+        $docRef = $this->prophesize(DocumentReference::class);
+        $docRef->snapshot()
+            ->shouldBeCalledTimes(1)
+            ->willReturn($snapshot);
+        $this->collection->document(self::SESSION_NAME . ':sessionid')
+            ->shouldBeCalledTimes(1)
+            ->willReturn($docRef);
+        $firestoreSessionHandler = new FirestoreSessionHandler(
+            $this->firestore->reveal()
+        );
+        $firestoreSessionHandler->open(self::SESSION_SAVE_PATH, self::SESSION_NAME);
+        $ret = $firestoreSessionHandler->read('sessionid');
+
+        $this->assertEquals('', $ret);
+    }
+
+    /**
+     * @expectedException PHPUnit_Framework_Error_Warning
+     */
+    public function testReadWithException()
+    {
+        $this->firestore->collection(self::SESSION_SAVE_PATH)
+            ->shouldBeCalledTimes(1)
+            ->willReturn($this->collection->reveal());
+        $firestoreSessionHandler = new FirestoreSessionHandler(
+            $this->firestore->reveal()
+        );
+        $this->collection->document(self::SESSION_NAME . ':sessionid')
+            ->shouldBeCalledTimes(1)
+            ->willThrow((new Exception()));
+
+        $firestoreSessionHandler->open(self::SESSION_SAVE_PATH, self::SESSION_NAME);
+        $ret = $firestoreSessionHandler->read('sessionid');
+
+        $this->assertEquals('', $ret);
+    }
+
+    public function testReadEntity()
+    {
+        $id = self::SESSION_NAME . ':sessionid';
+        $snapshot = $this->prophesize(DocumentSnapshot::class);
+        $snapshot->exists()
+            ->shouldBeCalledTimes(1)
+            ->willReturn(true);
+        $snapshot->offsetExists('data')
+            ->shouldBeCalledTimes(1)
+            ->willReturn(true);
+        $snapshot->get('data')
+            ->shouldBeCalledTimes(1)
+            ->willReturn('sessiondata');
+        $docRef = $this->prophesize(DocumentReference::class);
+        $docRef->snapshot()
+            ->shouldBeCalledTimes(1)
+            ->willReturn($snapshot);
+        $this->collection->document($id)
+            ->shouldBeCalledTimes(1)
+            ->willReturn($docRef);
+        $this->firestore->collection(self::SESSION_SAVE_PATH)
+            ->shouldBeCalledTimes(1)
+            ->willReturn($this->collection->reveal());
+        $firestoreSessionHandler = new FirestoreSessionHandler(
+            $this->firestore->reveal()
+        );
+        $firestoreSessionHandler->open(self::SESSION_SAVE_PATH, self::SESSION_NAME);
+        $ret = $firestoreSessionHandler->read('sessionid');
+
+        $this->assertEquals('sessiondata', $ret);
+    }
+
+    public function testWrite()
+    {
+        $id = self::SESSION_NAME . ':sessionid';
+        $docRef = $this->prophesize(DocumentReference::class);
+        $this->firestore->collection(self::SESSION_SAVE_PATH)
+            ->shouldBeCalledTimes(1)
+            ->willReturn($this->collection->reveal());
+        $this->collection->document($id)
+            ->shouldBeCalledTimes(1)
+            ->willReturn($docRef);
+        $phpunit = $this;
+        $docRef->set(Argument::type('array'))
+            ->will(function ($args) use ($phpunit) {
+                $phpunit->assertEquals('sessiondata', $args[0]['data']);
+                $phpunit->assertInternalType('int', $args[0]['t']);
+                $phpunit->assertGreaterThanOrEqual($args[0]['t'], time());
+                // 2 seconds grace period should be enough
+                $phpunit->assertLessThanOrEqual(2, time() - $args[0]['t']);
+            });
+        $firestoreSessionHandler = new FirestoreSessionHandler(
+            $this->firestore->reveal()
+        );
+        $firestoreSessionHandler->open(self::SESSION_SAVE_PATH, self::SESSION_NAME);
+        $ret = $firestoreSessionHandler->write('sessionid', 'sessiondata');
+
+        $this->assertTrue($ret);
+    }
+
+    /**
+     * @expectedException PHPUnit_Framework_Error_Warning
+     */
+    public function testWriteWithException()
+    {
+        $id = self::SESSION_NAME . ':sessionid';
+        $docRef = $this->prophesize(DocumentReference::class);
+        $docRef->set(Argument::type('array'))
+            ->shouldBeCalledTimes(1)
+            ->willThrow(new Exception());
+        $this->firestore->collection(self::SESSION_SAVE_PATH)
+            ->shouldBeCalledTimes(1)
+            ->willReturn($this->collection->reveal());
+        $this->collection->document($id)
+            ->shouldBeCalledTimes(1)
+            ->willReturn($docRef);
+
+        $firestoreSessionHandler = new FirestoreSessionHandler(
+            $this->firestore->reveal()
+        );
+        $firestoreSessionHandler->open(self::SESSION_SAVE_PATH, self::SESSION_NAME);
+        $ret = $firestoreSessionHandler->write('sessionid', 'sessiondata');
+
+        $this->assertFalse($ret);
+    }
+
+    public function testDestroy()
+    {
+        $id = self::SESSION_NAME . ':sessionid';
+        $docRef = $this->prophesize(DocumentReference::class);
+        $docRef->delete()
+            ->shouldBeCalledTimes(1);
+        $this->firestore->collection(self::SESSION_SAVE_PATH)
+            ->shouldBeCalledTimes(1)
+            ->willReturn($this->collection->reveal());
+        $this->collection->document($id)
+            ->shouldBeCalledTimes(1)
+            ->willReturn($docRef);
+        $firestoreSessionHandler = new FirestoreSessionHandler(
+            $this->firestore->reveal()
+        );
+        $firestoreSessionHandler->open(self::SESSION_SAVE_PATH, self::SESSION_NAME);
+        $ret = $firestoreSessionHandler->destroy('sessionid');
+
+        $this->assertTrue($ret);
+    }
+
+    /**
+     * @expectedException PHPUnit_Framework_Error_Warning
+     */
+    public function testDestroyWithException()
+    {
+        $id = self::SESSION_NAME . ':sessionid';
+        $docRef = $this->prophesize(DocumentReference::class);
+        $docRef->delete()
+            ->shouldBeCalledTimes(1)
+            ->willThrow(new Exception());
+        $this->firestore->collection(self::SESSION_SAVE_PATH)
+            ->shouldBeCalledTimes(1)
+            ->willReturn($this->collection->reveal());
+        $this->collection->document($id)
+            ->shouldBeCalledTimes(1)
+            ->willReturn($docRef);
+        $firestoreSessionHandler = new FirestoreSessionHandler(
+            $this->firestore->reveal()
+        );
+        $firestoreSessionHandler->open(self::SESSION_SAVE_PATH, self::SESSION_NAME);
+        $ret = $firestoreSessionHandler->destroy('sessionid');
+
+        $this->assertFalse($ret);
+    }
+
+    public function testDefaultGcDoesNothing()
+    {
+        $this->firestore->collection(self::SESSION_SAVE_PATH)
+            ->shouldBeCalledTimes(1)
+            ->willReturn($this->collection->reveal());
+        $this->collection->limit()->shouldNotBeCalled();
+        $firestoreSessionHandler = new FirestoreSessionHandler(
+            $this->firestore->reveal()
+        );
+        $firestoreSessionHandler->open(self::SESSION_SAVE_PATH, self::SESSION_NAME);
+        $ret = $firestoreSessionHandler->gc(100);
+
+        $this->assertTrue($ret);
+    }
+
+    public function testGc()
+    {
+        $docRef1 = $this->prophesize(DocumentReference::class);
+        $docRef1->delete()
+            ->shouldBeCalledTimes(1);
+        $snapshot1 = $this->prophesize(DocumentSnapshot::class);
+        $snapshot1->reference()
+            ->shouldBeCalledTimes(1)
+            ->willReturn($docRef1);
+        $docRef2 = $this->prophesize(DocumentReference::class);
+        $docRef2->delete()
+            ->shouldBeCalledTimes(1);
+        $snapshot2 = $this->prophesize(DocumentSnapshot::class);
+        $snapshot2->reference()
+            ->shouldBeCalledTimes(1)
+            ->willReturn($docRef2);
+        $phpunit = $this;
+        $collection = $this->collection;
+        $collection->where(
+            Argument::type('string'),
+            Argument::type('string'),
+            Argument::type('int')
+        )
+            ->shouldBeCalledTimes(1)
+            ->will(function ($args) use ($phpunit, $collection) {
+                $phpunit->assertEquals('t', $args[0]);
+                $phpunit->assertEquals('<', $args[1]);
+                $phpunit->assertInternalType('int', $args[2]);
+                $diff = time() - $args[2];
+                // 2 seconds grace period should be enough
+                $phpunit->assertLessThanOrEqual(102, $diff);
+                $phpunit->assertGreaterThanOrEqual(100, $diff);
+                return $collection->reveal();
+            });
+
+        $collection->orderBy('t')
+            ->shouldBeCalledTimes(1)
+            ->willReturn($collection->reveal());
+        $collection->limit(1000)
+            ->shouldBeCalledTimes(1)
+            ->willReturn($collection->reveal());
+        $collection->documents()
+            ->shouldBeCalledTimes(1)
+            ->willReturn([$snapshot1, $snapshot2]);
+        $this->firestore->collection(self::SESSION_SAVE_PATH)
+            ->shouldBeCalledTimes(1)
+            ->willReturn($collection->reveal());
+
+        $firestoreSessionHandler = new FirestoreSessionHandler(
+            $this->firestore->reveal(),
+            1000
+        );
+
+        $firestoreSessionHandler->open(self::SESSION_SAVE_PATH, self::SESSION_NAME);
+        $ret = $firestoreSessionHandler->gc(100);
+
+        $this->assertTrue($ret);
+    }
+
+    /**
+     * @expectedException PHPUnit_Framework_Error_Warning
+     */
+    public function testGcWithException()
+    {
+        $docRef = $this->prophesize(DocumentReference::class);
+        $docRef->delete()
+            ->shouldBeCalledTimes(1)
+            ->willThrow(new Exception());
+        $snapshot = $this->prophesize(DocumentSnapshot::class);
+        $snapshot->reference()
+            ->shouldBeCalledTimes(1)
+            ->willReturn($docRef);
+        $phpunit = $this;
+        $collection = $this->collection;
+        $collection->where(
+            Argument::type('string'),
+            Argument::type('string'),
+            Argument::type('int')
+        )
+            ->shouldBeCalledTimes(1)
+            ->will(function ($args) use ($phpunit, $collection) {
+                $phpunit->assertEquals('t', $args[0]);
+                $phpunit->assertEquals('<', $args[1]);
+                $phpunit->assertInternalType('int', $args[2]);
+                $diff = time() - $args[2];
+                // 2 seconds grace period should be enough
+                $phpunit->assertLessThanOrEqual(102, $diff);
+                $phpunit->assertGreaterThanOrEqual(100, $diff);
+                return $collection->reveal();
+            });
+
+        $collection->orderBy('t')
+            ->shouldBeCalledTimes(1)
+            ->willReturn($collection->reveal());
+        $collection->limit(1000)
+            ->shouldBeCalledTimes(1)
+            ->willReturn($collection->reveal());
+        $collection->documents()
+            ->shouldBeCalledTimes(1)
+            ->willReturn([$snapshot]);
+        $this->firestore->collection(self::SESSION_SAVE_PATH)
+            ->shouldBeCalledTimes(1)
+            ->willReturn($collection->reveal());
+        $firestoreSessionHandler = new FirestoreSessionHandler(
+            $this->firestore->reveal(),
+            1000
+        );
+
+        $firestoreSessionHandler->open(self::SESSION_SAVE_PATH, self::SESSION_NAME);
+        $ret = $firestoreSessionHandler->gc(100);
+
+        $this->assertFalse($ret);
+    }
+}

From e6909118c4f3ad5380d27d8b049b3df6f1e56442 Mon Sep 17 00:00:00 2001
From: Brent Shaffer <betterbrent@google.com>
Date: Thu, 22 Aug 2019 12:49:25 -0600
Subject: [PATCH 02/20] Update Firestore/src/FirestoreSessionHandler.php

Co-Authored-By: John Pedrie <jdpedrie@users.noreply.github.com>
---
 Firestore/src/FirestoreSessionHandler.php | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/Firestore/src/FirestoreSessionHandler.php b/Firestore/src/FirestoreSessionHandler.php
index 0da31c792a70..23085bc8118f 100644
--- a/Firestore/src/FirestoreSessionHandler.php
+++ b/Firestore/src/FirestoreSessionHandler.php
@@ -111,7 +111,10 @@ class FirestoreSessionHandler implements SessionHandlerInterface
      * Create a custom session handler backed by Cloud Firestore.
      *
      * @param FirestoreClient $firestore Firestore client.
-     * @param int $gcLimit [optional] A number of entities to delete in the
+     * @param int $gcLimit [optional] The number of entities to delete in the
+     *        garbage collection. Values larger than 1000 will be limited to
+     *        1000. **Defaults to** `0`, indicating garbage collection is
+     *        disabled by default.
      *        garbage collection.  Defaults to 0 which means it does nothing.
      *        The value larger than 1000 will be cut down to 1000.
      */

From 2ad13e27a6e19064b0be0c5899dbf19e56f9bcee Mon Sep 17 00:00:00 2001
From: Brent Shaffer <betterbrent@google.com>
Date: Thu, 22 Aug 2019 12:04:29 -0700
Subject: [PATCH 03/20] wraps writes in transactions

---
 Firestore/src/FirestoreSessionHandler.php            | 12 ++++++++----
 Firestore/tests/Unit/FirestoreSessionHandlerTest.php |  7 ++++---
 2 files changed, 12 insertions(+), 7 deletions(-)

diff --git a/Firestore/src/FirestoreSessionHandler.php b/Firestore/src/FirestoreSessionHandler.php
index 23085bc8118f..0894aba23e26 100644
--- a/Firestore/src/FirestoreSessionHandler.php
+++ b/Firestore/src/FirestoreSessionHandler.php
@@ -190,10 +190,14 @@ public function write($id, $data)
     {
         try {
             $docRef = $this->collection->document($this->formatId($id));
-            $docRef->set([
-                'data' => $data,
-                't' => time()
-            ]);
+            $this->firestore->runTransaction(
+                function (Transaction $transaction) use ($docRef, $data) {
+                    $transaction->set($docRef, [
+                        'data' => $data,
+                        't' => time()
+                    ]);
+                }
+            );
         } catch (Exception $e) {
             trigger_error(
                 sprintf('Firestore upsert failed: %s', $e->getMessage()),
diff --git a/Firestore/tests/Unit/FirestoreSessionHandlerTest.php b/Firestore/tests/Unit/FirestoreSessionHandlerTest.php
index bb8f98955011..f161309aca07 100644
--- a/Firestore/tests/Unit/FirestoreSessionHandlerTest.php
+++ b/Firestore/tests/Unit/FirestoreSessionHandlerTest.php
@@ -23,6 +23,7 @@
 use Google\Cloud\Firestore\CollectionReference;
 use Google\Cloud\Firestore\DocumentReference;
 use Google\Cloud\Firestore\DocumentSnapshot;
+use Google\Cloud\Firestore\Transaction;
 use InvalidArgumentException;
 use Prophecy\Argument;
 use PHPUnit\Framework\TestCase;
@@ -165,6 +166,8 @@ public function testWrite()
         $this->firestore->collection(self::SESSION_SAVE_PATH)
             ->shouldBeCalledTimes(1)
             ->willReturn($this->collection->reveal());
+        $this->firestore->runTransaction(Argument::type('closure'))
+            ->shouldBeCalledTimes(1);
         $this->collection->document($id)
             ->shouldBeCalledTimes(1)
             ->willReturn($docRef);
@@ -192,10 +195,8 @@ public function testWrite()
     public function testWriteWithException()
     {
         $id = self::SESSION_NAME . ':sessionid';
+        $transaction = $this->prophesize(Transaction::class);
         $docRef = $this->prophesize(DocumentReference::class);
-        $docRef->set(Argument::type('array'))
-            ->shouldBeCalledTimes(1)
-            ->willThrow(new Exception());
         $this->firestore->collection(self::SESSION_SAVE_PATH)
             ->shouldBeCalledTimes(1)
             ->willReturn($this->collection->reveal());

From d5212d117ca3904d5d36029409659bba892e1145 Mon Sep 17 00:00:00 2001
From: Brent Shaffer <betterbrent@google.com>
Date: Thu, 22 Aug 2019 15:01:44 -0700
Subject: [PATCH 04/20] removes collection name validation, mocks
 runTransaction better

---
 Firestore/src/FirestoreSessionHandler.php     |  5 ---
 .../Unit/FirestoreSessionHandlerTest.php      | 32 +++++++++++--------
 2 files changed, 18 insertions(+), 19 deletions(-)

diff --git a/Firestore/src/FirestoreSessionHandler.php b/Firestore/src/FirestoreSessionHandler.php
index 0894aba23e26..67282daf6493 100644
--- a/Firestore/src/FirestoreSessionHandler.php
+++ b/Firestore/src/FirestoreSessionHandler.php
@@ -138,11 +138,6 @@ public function __construct(
      */
     public function open($savePath, $sessionName)
     {
-        if (false !== strpos($savePath, '/')) {
-            throw new InvalidArgumentException(
-                sprintf('The given save_path "%s" not allowed', $savePath)
-            );
-        }
         $this->sessionName = $sessionName;
         $this->collection = $this->firestore->collection($savePath);
         return true;
diff --git a/Firestore/tests/Unit/FirestoreSessionHandlerTest.php b/Firestore/tests/Unit/FirestoreSessionHandlerTest.php
index f161309aca07..b48fc3e4ad02 100644
--- a/Firestore/tests/Unit/FirestoreSessionHandlerTest.php
+++ b/Firestore/tests/Unit/FirestoreSessionHandlerTest.php
@@ -62,13 +62,13 @@ public function testOpen()
      */
     public function testOpenNotAllowed()
     {
-        $this->firestore->collection(self::SESSION_SAVE_PATH)
-            ->shouldNotBeCalled()
-            ->willReturn($this->collection->reveal());
+        $this->firestore->collection('invalid/savepath')
+            ->shouldBeCalledTimes(1)
+            ->willThrow(new InvalidArgumentException());
         $firestoreSessionHandler = new FirestoreSessionHandler(
             $this->firestore->reveal()
         );
-        $firestoreSessionHandler->open('/tmp/sessions', self::SESSION_NAME);
+        $firestoreSessionHandler->open('invalid/savepath', self::SESSION_NAME);
     }
 
 
@@ -166,20 +166,24 @@ public function testWrite()
         $this->firestore->collection(self::SESSION_SAVE_PATH)
             ->shouldBeCalledTimes(1)
             ->willReturn($this->collection->reveal());
+        $phpunit = $this;
         $this->firestore->runTransaction(Argument::type('closure'))
-            ->shouldBeCalledTimes(1);
+            ->shouldBeCalledTimes(1)
+            ->will(function($args) use ($phpunit, $docRef) {
+                $transaction = $phpunit->prophesize(Transaction::class);
+                $transaction->set($docRef, Argument::type('array'))
+                    ->will(function ($args) use ($phpunit) {
+                        $phpunit->assertEquals('sessiondata', $args[1]['data']);
+                        $phpunit->assertInternalType('int', $args[1]['t']);
+                        $phpunit->assertGreaterThanOrEqual($args[1]['t'], time());
+                        // 2 seconds grace period should be enough
+                        $phpunit->assertLessThanOrEqual(2, time() - $args[1]['t']);
+                    });
+                $args[0]($transaction->reveal());
+            });
         $this->collection->document($id)
             ->shouldBeCalledTimes(1)
             ->willReturn($docRef);
-        $phpunit = $this;
-        $docRef->set(Argument::type('array'))
-            ->will(function ($args) use ($phpunit) {
-                $phpunit->assertEquals('sessiondata', $args[0]['data']);
-                $phpunit->assertInternalType('int', $args[0]['t']);
-                $phpunit->assertGreaterThanOrEqual($args[0]['t'], time());
-                // 2 seconds grace period should be enough
-                $phpunit->assertLessThanOrEqual(2, time() - $args[0]['t']);
-            });
         $firestoreSessionHandler = new FirestoreSessionHandler(
             $this->firestore->reveal()
         );

From 91660181e61caeb009c17703c20b3e1dc98e7307 Mon Sep 17 00:00:00 2001
From: John Pedrie <john@pedrie.com>
Date: Wed, 28 Aug 2019 13:24:10 -0400
Subject: [PATCH 05/20] Add snippet and system tests

---
 Firestore/src/FirestoreSessionHandler.php     |  93 ++++++------
 .../Snippet/FirestoreSessionHandlerTest.php   | 136 ++++++++++++++++++
 .../System/FirestoreSessionHandlerTest.php    |  61 ++++++++
 .../Unit/FirestoreSessionHandlerTest.php      |  19 ++-
 4 files changed, 262 insertions(+), 47 deletions(-)
 create mode 100644 Firestore/tests/Snippet/FirestoreSessionHandlerTest.php
 create mode 100644 Firestore/tests/System/FirestoreSessionHandlerTest.php

diff --git a/Firestore/src/FirestoreSessionHandler.php b/Firestore/src/FirestoreSessionHandler.php
index 67282daf6493..60afa934aa02 100644
--- a/Firestore/src/FirestoreSessionHandler.php
+++ b/Firestore/src/FirestoreSessionHandler.php
@@ -16,44 +16,55 @@
  */
 namespace Google\Cloud\Firestore;
 
-use Exception;
-use InvalidArgumentException;
+use Google\Cloud\Core\Exception\ServiceException;
 use SessionHandlerInterface;
 
 /**
  * Custom session handler backed by Cloud Firestore.
  *
- * Instead of storing the session data in a local file, it stores the data to
- * Cloud Firestore. The biggest benefit of doing this is the data can be
- * shared by multiple instances, so it's suitable for cloud applications.
+ * Instead of storing the session data in a local file, this handler stores the
+ * data in Firestore. The biggest benefit of doing this is the data can be
+ * shared by multiple instances, making it suitable for cloud applications.
  *
- * The downside of using Cloud Firestore is the write operations will cost you
- * some money, so it is highly recommended to minimize the write operations
- * with your session data with this handler. In order to do so, keep the data
- * in the session as limited as possible; for example, it is ok to put only
- * signed-in state and the user id in the session with this handler. However,
- * for example, it is definitely not recommended that you store your
- * application's whole undo history in the session, because every user
- * operations will cause the Firestore write and then it will cost you lot of
- * money.
+ * The downside of using Firestore is that write operations will cost you some
+ * money, so it is highly recommended to minimize the write operations while
+ * using this handler. In order to do so, keep the data in the session as
+ * limited as possible; for example, it is ok to put only signed-in state and
+ * the user id in the session with this handler. However, for example, it is
+ * definitely not recommended that you store your application's whole undo
+ * history in the session, because every user operation will cause a Firestore
+ * write, potentially costing you a lot of money.
+ *
+ * This handler doesn't provide pessimistic lock for session data. Instead, it
+ * uses a Firestore transaction for data consistency. This means that if
+ * multiple requests are modifying the same session data simultaneously, there
+ * will be more probablity that some of the `write` operations will fail.
  *
  * If you are building an AJAX application which may issue multiple requests
- * to the server, please design the session data carefully, in order to avoid
+ * to the server, please design your session data carefully in order to avoid
  * possible data contentions. Also please see the 2nd example below for how to
  * properly handle errors on `write` operations.
  *
- * It uses the session.save_path as the Firestore namespace for isolating the
- * session data from your application data, it also uses the session.name as
- * the Firestore kind, the session id as the Firestore id. By default, it
- * does nothing on gc for reducing the cost. Pass positive value up to 1000
- * for $gcLimit parameter to delete entities in gc.
+ * The handler stores data in a collection provided by the value of
+ * session.save_path, isolating the session data from your application data. It
+ * creates documents in the specified collection where the session name and ID
+ * are concatenated. By default, it does nothing on gc for reducing the cost.
+ * Pass a positive value up to 1000 for $gcLimit parameter to delete entities in
+ * gc.
  *
+ * The first example automatically writes the session data. It's handy, but
+ * the code doesn't stop even if it fails to write the session data, because
+ * the `write` happens when the code exits. If you want to know whether the
+ * session data is correctly written to Firestore, you need to call
+ * `session_write_close()` explicitly and then handle `E_USER_WARNING`
+ * properly. See the second example for a demonstration.
  *
- * Example without error handling:
+ * Example:
  * ```
  * use Google\Cloud\Firestore\FirestoreClient;
+ * use Google\Cloud\Firestore\FirestoreSessionHandler;
  *
- * $firestore = new FirestoreClient(['projectId' => $projectId]);
+ * $firestore = new FirestoreClient();
  *
  * $handler = new FirestoreSessionHandler($firestore);
  *
@@ -61,23 +72,17 @@
  * session_save_path('sessions');
  * session_start();
  *
- * // Then read and write the $_SESSION array.
- *
+ * // Then write and read the $_SESSION array.
+ * $_SESSION['name'] = 'Bob';
+ * echo $_SESSION['name'];
  * ```
  *
- * The above example automatically writes the session data. It's handy, but
- * the code doesn't stop even if it fails to write the session data, because
- * the `write` happens when the code exits. If you want to know the session
- * data is correctly written to the Firestore, you need to call
- * `session_write_close()` explicitly and then handle `E_USER_WARNING`
- * properly like the following example.
- *
- * Example with error handling:
- *
  * ```
+ * // Session handler with error handling:
  * use Google\Cloud\Firestore\FirestoreClient;
+ * use Google\Cloud\Firestore\FirestoreSessionHandler;
  *
- * $firestore = new FirestoreClient(['projectId' => $projectId]);
+ * $firestore = new FirestoreClient();
  *
  * $handler = new FirestoreSessionHandler($firestore);
  * session_set_save_handler($handler, true);
@@ -85,17 +90,25 @@
  * session_start();
  *
  * // Then read and write the $_SESSION array.
+ * $_SESSION['name'] = 'Bob';
  *
  * function handle_session_error($errNo, $errStr, $errFile, $errLine) {
- *     # We throw an exception here, but you can do whatever you need.
- *     throw new Exception("$errStr in $errFile on line $errLine", $errNo);
+ *     // We throw an exception here, but you can do whatever you need.
+ *     throw new RuntimeException(
+ *         "$errStr in $errFile on line $errLine",
+ *         $errNo
+ *     );
  * }
  * set_error_handler('handle_session_error', E_USER_WARNING);
+ *
  * // If `write` fails for any reason, an exception will be thrown.
  * session_write_close();
  * restore_error_handler();
+ *
  * // You can still read the $_SESSION array after closing the session.
+ * echo $_SESSION['name'];
  * ```
+ *
  * @see http://php.net/manual/en/class.sessionhandlerinterface.php SessionHandlerInterface
  */
 class FirestoreSessionHandler implements SessionHandlerInterface
@@ -165,7 +178,7 @@ public function read($id)
             if ($snapshot->exists() && isset($snapshot['data'])) {
                 return $snapshot->get('data');
             }
-        } catch (Exception $e) {
+        } catch (ServiceException $e) {
             trigger_error(
                 sprintf('Firestore lookup failed: %s', $e->getMessage()),
                 E_USER_WARNING
@@ -193,7 +206,7 @@ function (Transaction $transaction) use ($docRef, $data) {
                     ]);
                 }
             );
-        } catch (Exception $e) {
+        } catch (ServiceException $e) {
             trigger_error(
                 sprintf('Firestore upsert failed: %s', $e->getMessage()),
                 E_USER_WARNING
@@ -213,7 +226,7 @@ public function destroy($id)
     {
         try {
             $this->collection->document($this->formatId($id))->delete();
-        } catch (Exception $e) {
+        } catch (ServiceException $e) {
             trigger_error(
                 sprintf('Firestore delete failed: %s', $e->getMessage()),
                 E_USER_WARNING
@@ -243,7 +256,7 @@ public function gc($maxlifetime)
             foreach ($query->documents() as $snapshot) {
                 $snapshot->reference()->delete();
             }
-        } catch (Exception $e) {
+        } catch (ServiceException $e) {
             trigger_error(
                 sprintf('Session gc failed: %s', $e->getMessage()),
                 E_USER_WARNING
diff --git a/Firestore/tests/Snippet/FirestoreSessionHandlerTest.php b/Firestore/tests/Snippet/FirestoreSessionHandlerTest.php
new file mode 100644
index 000000000000..78436570caee
--- /dev/null
+++ b/Firestore/tests/Snippet/FirestoreSessionHandlerTest.php
@@ -0,0 +1,136 @@
+<?php
+/**
+ * Copyright 2019 Google LLC.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+namespace Google\Cloud\Firestore\Tests\Snippet;
+
+use Google\Cloud\Core\Testing\Snippet\SnippetTestCase;
+use Google\Cloud\Core\Testing\TestHelpers;
+use Google\Cloud\Firestore\Connection\ConnectionInterface;
+use Google\Cloud\Firestore\FirestoreClient;
+use Google\Cloud\Firestore\FirestoreSessionHandler;
+use Prophecy\Argument;
+
+/**
+ * @group firestore
+ * @group firestore-session
+ * @runTestsInSeparateProcesses
+ */
+class FirestoreSessionHandlerTest extends SnippetTestCase
+{
+    const TRANSACTION = 'transaction-id';
+
+    private $connection;
+    private $client;
+
+    public static function setUpBeforeClass()
+    {
+        parent::setUpBeforeClass();
+
+        // Since the tests in this class must run in isolation, they won't be
+        // recognized as having been covered, and will cause a CI error.
+        // We can call `snippetFromClass` in the parent process to mark the
+        // snippets as having been covered.
+        self::snippetFromClass(FirestoreSessionHandler::class);
+        self::snippetFromClass(FirestoreSessionHandler::class, 1);
+    }
+
+    public function setUp()
+    {
+        $this->connection = $this->prophesize(ConnectionInterface::class);
+        $this->client = TestHelpers::stub(FirestoreClient::class);
+    }
+
+    public function testClass()
+    {
+        $snippet = $this->snippetFromClass(FirestoreSessionHandler::class);
+        $snippet->replace('$firestore = new FirestoreClient();', '');
+
+        $this->connection->batchGetDocuments(Argument::withEntry('documents', Argument::type('array')))
+            ->shouldBeCalled()
+            ->willReturn(new \ArrayIterator([
+                'found' => [
+                    [
+                        'name' => '',
+                        'fields' => []
+                    ]
+                ]
+            ]));
+
+        $this->connection->beginTransaction(Argument::any())
+            ->shouldBeCalled()
+            ->willReturn([
+                'transaction' => self::TRANSACTION
+            ]);
+
+        $value = 'name|' . serialize('Bob');
+        $this->connection->commit(Argument::allOf(
+            Argument::that(function ($args) use ($value) {
+                return strpos($args['writes'][0]['update']['name'], 'PHPSESSID:') !== false
+                    && $args['writes'][0]['update']['fields']['data']['stringValue'] === $value
+                    && isset($args['writes'][0]['update']['fields']['t']['integerValue']);
+            }),
+            Argument::withEntry('transaction', self::TRANSACTION)
+        ))->shouldBeCalled()->willReturn([
+            'writeResults' => []
+        ]);
+
+        $this->client->___setProperty('connection', $this->connection->reveal());
+        $snippet->addLocal('firestore', $this->client);
+
+        $res = $snippet->invoke();
+        session_write_close();
+
+        $this->assertEquals('Bob', $res->output());
+    }
+
+    /**
+     * @expectedException \RuntimeException
+     */
+    public function testClassErrorHandler()
+    {
+        $snippet = $this->snippetFromClass(FirestoreSessionHandler::class, 1);
+        $snippet->replace('$firestore = new FirestoreClient();', '');
+
+        $this->connection->batchGetDocuments(Argument::any())
+            ->shouldBeCalled()
+            ->willReturn(new \ArrayIterator([
+                'found' => [
+                    [
+                        'name' => '',
+                        'fields' => []
+                    ]
+                ]
+            ]));
+
+        $this->connection->beginTransaction(Argument::any())
+            ->shouldBeCalled()
+            ->willReturn([
+                'transaction' => self::TRANSACTION
+            ]);
+
+        $this->connection->commit(Argument::any())
+            ->shouldBeCalled()
+            ->will(function () {
+                trigger_error('oops!', E_USER_WARNING);
+            });
+
+        $this->client->___setProperty('connection', $this->connection->reveal());
+        $snippet->addLocal('firestore', $this->client);
+
+        $res = $snippet->invoke();
+    }
+}
diff --git a/Firestore/tests/System/FirestoreSessionHandlerTest.php b/Firestore/tests/System/FirestoreSessionHandlerTest.php
new file mode 100644
index 000000000000..a37cde387170
--- /dev/null
+++ b/Firestore/tests/System/FirestoreSessionHandlerTest.php
@@ -0,0 +1,61 @@
+<?php
+/**
+ * Copyright 2019 Google LLC.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *      http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+namespace Google\Cloud\Firestore\Tests\System;
+
+use Google\Cloud\Firestore\FirestoreSessionHandler;
+
+/**
+ * @group firestore
+ * @group firestore-session
+ *
+ * @runTestsInSeparateProcesses
+ */
+class FirestoreSessionHandlerTest extends FirestoreTestCase
+{
+    public function testSessionHandler()
+    {
+        $client = self::$client;
+
+        $namespace = uniqid('sess-' . self::COLLECTION_NAME);
+        $content = 'foo';
+        $storedValue = 'name|' . serialize($content);
+
+        $handler = new FirestoreSessionHandler($client);
+
+        session_set_save_handler($handler, true);
+        session_save_path($namespace);
+        session_start();
+
+        $sessionId = session_id();
+        $_SESSION['name'] = $content;
+
+        session_write_close();
+        sleep(1);
+
+        $hasDocument = false;
+        $query = $client->collection($namespace);
+        foreach ($query->documents() as $snapshot) {
+            self::$deletionQueue->add($snapshot->reference());
+            if (!$hasDocument) {
+                $hasDocument = $snapshot['data'] === $storedValue;
+            }
+        }
+
+        $this->assertTrue($hasDocument);
+    }
+}
diff --git a/Firestore/tests/Unit/FirestoreSessionHandlerTest.php b/Firestore/tests/Unit/FirestoreSessionHandlerTest.php
index b48fc3e4ad02..42d97bcc1f1c 100644
--- a/Firestore/tests/Unit/FirestoreSessionHandlerTest.php
+++ b/Firestore/tests/Unit/FirestoreSessionHandlerTest.php
@@ -18,18 +18,20 @@
 namespace Google\Cloud\Firestore\Tests\Unit;
 
 use Exception;
-use Google\Cloud\Firestore\FirestoreClient;
-use Google\Cloud\Firestore\FirestoreSessionHandler;
+use Google\Cloud\Core\Exception\ServiceException;
 use Google\Cloud\Firestore\CollectionReference;
 use Google\Cloud\Firestore\DocumentReference;
 use Google\Cloud\Firestore\DocumentSnapshot;
+use Google\Cloud\Firestore\FirestoreClient;
+use Google\Cloud\Firestore\FirestoreSessionHandler;
 use Google\Cloud\Firestore\Transaction;
 use InvalidArgumentException;
-use Prophecy\Argument;
 use PHPUnit\Framework\TestCase;
+use Prophecy\Argument;
 
 /**
  * @group firestore
+ * @group firestore-session
  */
 class FirestoreSessionHandlerTest extends TestCase
 {
@@ -119,7 +121,7 @@ public function testReadWithException()
         );
         $this->collection->document(self::SESSION_NAME . ':sessionid')
             ->shouldBeCalledTimes(1)
-            ->willThrow((new Exception()));
+            ->willThrow((new ServiceException('')));
 
         $firestoreSessionHandler->open(self::SESSION_SAVE_PATH, self::SESSION_NAME);
         $ret = $firestoreSessionHandler->read('sessionid');
@@ -169,7 +171,7 @@ public function testWrite()
         $phpunit = $this;
         $this->firestore->runTransaction(Argument::type('closure'))
             ->shouldBeCalledTimes(1)
-            ->will(function($args) use ($phpunit, $docRef) {
+            ->will(function ($args) use ($phpunit, $docRef) {
                 $transaction = $phpunit->prophesize(Transaction::class);
                 $transaction->set($docRef, Argument::type('array'))
                     ->will(function ($args) use ($phpunit) {
@@ -204,6 +206,9 @@ public function testWriteWithException()
         $this->firestore->collection(self::SESSION_SAVE_PATH)
             ->shouldBeCalledTimes(1)
             ->willReturn($this->collection->reveal());
+        $this->firestore->runTransaction(Argument::any())
+            ->shouldBeCalledTimes(1)
+            ->willThrow(new ServiceException(''));
         $this->collection->document($id)
             ->shouldBeCalledTimes(1)
             ->willReturn($docRef);
@@ -247,7 +252,7 @@ public function testDestroyWithException()
         $docRef = $this->prophesize(DocumentReference::class);
         $docRef->delete()
             ->shouldBeCalledTimes(1)
-            ->willThrow(new Exception());
+            ->willThrow(new ServiceException(''));
         $this->firestore->collection(self::SESSION_SAVE_PATH)
             ->shouldBeCalledTimes(1)
             ->willReturn($this->collection->reveal());
@@ -345,7 +350,7 @@ public function testGcWithException()
         $docRef = $this->prophesize(DocumentReference::class);
         $docRef->delete()
             ->shouldBeCalledTimes(1)
-            ->willThrow(new Exception());
+            ->willThrow(new ServiceException(''));
         $snapshot = $this->prophesize(DocumentSnapshot::class);
         $snapshot->reference()
             ->shouldBeCalledTimes(1)

From d4249e666ce9cf1d0811ef240d33ce1cd22ff667 Mon Sep 17 00:00:00 2001
From: John Pedrie <john@pedrie.com>
Date: Wed, 28 Aug 2019 13:26:18 -0400
Subject: [PATCH 06/20] remove extraneous doc lines

---
 Firestore/src/FirestoreSessionHandler.php | 2 --
 1 file changed, 2 deletions(-)

diff --git a/Firestore/src/FirestoreSessionHandler.php b/Firestore/src/FirestoreSessionHandler.php
index 60afa934aa02..784c9ccc032d 100644
--- a/Firestore/src/FirestoreSessionHandler.php
+++ b/Firestore/src/FirestoreSessionHandler.php
@@ -128,8 +128,6 @@ class FirestoreSessionHandler implements SessionHandlerInterface
      *        garbage collection. Values larger than 1000 will be limited to
      *        1000. **Defaults to** `0`, indicating garbage collection is
      *        disabled by default.
-     *        garbage collection.  Defaults to 0 which means it does nothing.
-     *        The value larger than 1000 will be cut down to 1000.
      */
     public function __construct(
         FirestoreClient $firestore,

From dc26d834d05592efde49394838bf6c0c5b0c0985 Mon Sep 17 00:00:00 2001
From: John Pedrie <john@pedrie.com>
Date: Wed, 28 Aug 2019 13:40:34 -0400
Subject: [PATCH 07/20] Skip tests if grpc missing

---
 Firestore/tests/Snippet/FirestoreSessionHandlerTest.php | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/Firestore/tests/Snippet/FirestoreSessionHandlerTest.php b/Firestore/tests/Snippet/FirestoreSessionHandlerTest.php
index 78436570caee..10ce2fd2f953 100644
--- a/Firestore/tests/Snippet/FirestoreSessionHandlerTest.php
+++ b/Firestore/tests/Snippet/FirestoreSessionHandlerTest.php
@@ -17,6 +17,7 @@
 
 namespace Google\Cloud\Firestore\Tests\Snippet;
 
+use Google\Cloud\Core\Testing\GrpcTestTrait;
 use Google\Cloud\Core\Testing\Snippet\SnippetTestCase;
 use Google\Cloud\Core\Testing\TestHelpers;
 use Google\Cloud\Firestore\Connection\ConnectionInterface;
@@ -31,6 +32,8 @@
  */
 class FirestoreSessionHandlerTest extends SnippetTestCase
 {
+    use GrpcTestTrait;
+
     const TRANSACTION = 'transaction-id';
 
     private $connection;
@@ -50,6 +53,8 @@ public static function setUpBeforeClass()
 
     public function setUp()
     {
+        $this->checkAndSkipGrpcTests();
+
         $this->connection = $this->prophesize(ConnectionInterface::class);
         $this->client = TestHelpers::stub(FirestoreClient::class);
     }

From 216b9641ac50f1056e41530258173a88fdf17546 Mon Sep 17 00:00:00 2001
From: Brent Shaffer <betterbrent@google.com>
Date: Wed, 18 Sep 2019 15:44:08 -0700
Subject: [PATCH 08/20] updates sessionhandler to use transactions

---
 Firestore/src/FirestoreClient.php             |  39 +++-
 Firestore/src/FirestoreSessionHandler.php     | 181 ++++++++++++++----
 .../Snippet/FirestoreSessionHandlerTest.php   |  21 +-
 .../System/FirestoreSessionHandlerTest.php    |   4 +-
 4 files changed, 188 insertions(+), 57 deletions(-)

diff --git a/Firestore/src/FirestoreClient.php b/Firestore/src/FirestoreClient.php
index a6f003b70a63..6839c0b0f0eb 100644
--- a/Firestore/src/FirestoreClient.php
+++ b/Firestore/src/FirestoreClient.php
@@ -443,7 +443,7 @@ public function runTransaction(callable $callable, array $options = [])
             'maxRetries' => self::MAX_RETRIES,
             'begin' => [],
             'commit' => [],
-            'rollback' => []
+            'rollback' => [],
         ];
 
         $retryableErrors = [
@@ -573,4 +573,41 @@ public function fieldPath(array $fieldNames)
     {
         return new FieldPath($fieldNames);
     }
+
+    /**
+     * Returns a FirestoreSessionHandler.
+     *
+     * Example:
+     * ```
+     * use Google\Cloud\Firestore\FirestoreClient;
+     *
+     * $firestore = new FirestoreClient(['projectId' => $projectId]);
+     *
+     * $handler = $firestore->sessionHandler();
+     * session_set_save_handler($handler, true);
+     * session_save_path('sessions');
+     * session_start();
+     * ```
+     *
+     * @param array $options {
+     *     Configuration Options.
+     *
+     *     @type int $gcLimit [optional] The number of entities to delete in the
+     *        garbage collection. Values larger than 1000 will be limited to
+     *        1000. **Defaults to** `0`, indicating garbage collection is
+     *        disabled by default.
+     * @return FirestoreSessionHandler
+     */
+    public function sessionHandler(array $options = [])
+    {
+        return new FirestoreSessionHandler(
+            $this->connection,
+            $this->valueMapper,
+            $this->databaseName(
+                $this->projectId,
+                $this->database
+            ),
+            $options
+        );
+    }
 }
diff --git a/Firestore/src/FirestoreSessionHandler.php b/Firestore/src/FirestoreSessionHandler.php
index 784c9ccc032d..5d240419a9ae 100644
--- a/Firestore/src/FirestoreSessionHandler.php
+++ b/Firestore/src/FirestoreSessionHandler.php
@@ -18,6 +18,7 @@
 
 use Google\Cloud\Core\Exception\ServiceException;
 use SessionHandlerInterface;
+use Google\Cloud\Firestore\Connection\ConnectionInterface;
 
 /**
  * Custom session handler backed by Cloud Firestore.
@@ -62,11 +63,10 @@
  * Example:
  * ```
  * use Google\Cloud\Firestore\FirestoreClient;
- * use Google\Cloud\Firestore\FirestoreSessionHandler;
  *
  * $firestore = new FirestoreClient();
  *
- * $handler = new FirestoreSessionHandler($firestore);
+ * $handler = $firestore->sessionHandler();
  *
  * session_set_save_handler($handler, true);
  * session_save_path('sessions');
@@ -80,11 +80,10 @@
  * ```
  * // Session handler with error handling:
  * use Google\Cloud\Firestore\FirestoreClient;
- * use Google\Cloud\Firestore\FirestoreSessionHandler;
  *
  * $firestore = new FirestoreClient();
  *
- * $handler = new FirestoreSessionHandler($firestore);
+ * $handler = $firestore->sessionHandler();
  * session_set_save_handler($handler, true);
  * session_save_path('sessions');
  * session_start();
@@ -113,29 +112,62 @@
  */
 class FirestoreSessionHandler implements SessionHandlerInterface
 {
-    /* @var int */
-    private $gcLimit;
-    /* @var FirestoreClient */
-    private $firestore;
-    /* @var CollectionReference */
-    private $collection;
+    /**
+     * @var ConnectionInterface
+     */
+    private $connection;
+    /**
+     * @var ValueMapper
+     */
+    private $valueMapper;
+    /**
+     * @var string
+     */
+    private $database;
+    /**
+     * @var array
+     */
+    private $options;
+    /**
+     * @var string
+     */
+    private $savePath;
+    /**
+     * @var string
+     */
+    private $sessionName;
+    /**
+     * @var Transaction
+     */
+    private $transaction;
 
     /**
      * Create a custom session handler backed by Cloud Firestore.
      *
-     * @param FirestoreClient $firestore Firestore client.
-     * @param int $gcLimit [optional] The number of entities to delete in the
-     *        garbage collection. Values larger than 1000 will be limited to
-     *        1000. **Defaults to** `0`, indicating garbage collection is
-     *        disabled by default.
+     * @param ConnectionInterface $connection A Connection to Cloud Firestore.
+     * @param ValueMapper $valueMapper A Firestore Value Mapper.
+     * @param string $database The current database
+     * @param array $options [optional]
      */
     public function __construct(
-        FirestoreClient $firestore,
-        $gcLimit = 0
+        ConnectionInterface $connection,
+        ValueMapper $valueMapper,
+        $database,
+        array $options = []
     ) {
-        $this->firestore = $firestore;
-        // Cut down to 1000
-        $this->gcLimit = min($gcLimit, 1000);
+        $this->connection = $connection;
+        $this->valueMapper = $valueMapper;
+        $this->database = $database;
+        $this->options = $options + [
+            'begin' => [],
+            'commit' => [],
+            'rollback' => [],
+            'delete' => [],
+            'gcLimit' => 0,
+        ];
+
+        // Cut down gcLimit to 1000
+        $this->options['gcLimit'] = min($this->options['gcLimit'], 1000);
     }
 
     /**
@@ -149,8 +181,20 @@ public function __construct(
      */
     public function open($savePath, $sessionName)
     {
+        $this->savePath = $savePath;
         $this->sessionName = $sessionName;
-        $this->collection = $this->firestore->collection($savePath);
+
+        $beginTransaction = $this->connection->beginTransaction([
+            'database' => $this->database,
+        ] + $this->options['begin']);
+
+        $this->transaction = new Transaction(
+            $this->connection,
+            $this->valueMapper,
+            $this->database,
+            $beginTransaction['transaction']
+        );
+
         return true;
     }
 
@@ -171,8 +215,7 @@ public function close()
     public function read($id)
     {
         try {
-            $docRef = $this->collection->document($this->formatId($id));
-            $snapshot = $docRef->snapshot();
+            $snapshot = $this->transaction->snapshot($this->docRef($id));
             if ($snapshot->exists() && isset($snapshot['data'])) {
                 return $snapshot->get('data');
             }
@@ -195,15 +238,11 @@ public function read($id)
     public function write($id, $data)
     {
         try {
-            $docRef = $this->collection->document($this->formatId($id));
-            $this->firestore->runTransaction(
-                function (Transaction $transaction) use ($docRef, $data) {
-                    $transaction->set($docRef, [
-                        'data' => $data,
-                        't' => time()
-                    ]);
-                }
-            );
+            $this->transaction->set($this->docRef($id), [
+                'data' => $data,
+                't' => time()
+            ]);
+            $this->commitTransaction();
         } catch (ServiceException $e) {
             trigger_error(
                 sprintf('Firestore upsert failed: %s', $e->getMessage()),
@@ -223,7 +262,11 @@ function (Transaction $transaction) use ($docRef, $data) {
     public function destroy($id)
     {
         try {
-            $this->collection->document($this->formatId($id))->delete();
+            $this->transaction->delete(
+                $this->docRef($id),
+                $this->options['delete']
+            );
+            $this->commitTransaction();
         } catch (ServiceException $e) {
             trigger_error(
                 sprintf('Firestore delete failed: %s', $e->getMessage()),
@@ -247,13 +290,20 @@ public function gc($maxlifetime)
             return true;
         }
         try {
-            $query = $this->collection
+            $query = $this->collectionRef()
                 ->limit($this->gcLimit)
                 ->orderBy('t')
-                ->where('t', '<', time() - $maxlifetime);
-            foreach ($query->documents() as $snapshot) {
-                $snapshot->reference()->delete();
+                ->where('t', '<', time() - $maxlifetime)
+                ->where('id', '>=', $this->sessionName . ':')
+                ->where('id', '<', $this->sessionName . chr(ord(':') + 1));
+            $querySnapshot = $this->transaction->runQuery($query);
+            foreach ($querySnapshot as $snapshot) {
+                $this->transaction->delete(
+                    $snapshot->reference(),
+                    $this->options['delete']
+                );
             }
+            $this->commitTransaction();
         } catch (ServiceException $e) {
             trigger_error(
                 sprintf('Session gc failed: %s', $e->getMessage()),
@@ -265,14 +315,61 @@ public function gc($maxlifetime)
     }
 
     /**
-     * Format the Firebase document ID from the PHP session ID and session name.
-     * ex: PHPSESSID:abcdef
+     * Returns a Firestore document reference for the provided PHP session ID.
      *
      * @param string $id Identifier used for the session
-     * @return string
+     * @return DocumentReference
+     */
+    private function docRef($id)
+    {
+        // The Firebase document name is derived from the session ID and session
+        // path, ex: "PHPSESSID:abcdef".
+        $collectionRef = $this->collectionRef();
+        $parent = $collectionRef->name();
+        $name = sprintf('%s/%s:%s', $parent, $this->sessionName, $id);
+        return new DocumentReference(
+            $this->connection,
+            $this->valueMapper,
+            $collectionRef,
+            $name
+        );
+    }
+
+    /**
+     * Returns a Firestore collection reference for the provided PHP session ID.
+     *
+     * @return CollectionReference
+     */
+    private function collectionRef()
+    {
+        // The Firebase collection path is derived from the save path.
+        $name = sprintf('%s/documents/%s', $this->database, $this->savePath);
+        return new CollectionReference(
+            $this->connection,
+            $this->valueMapper,
+            $name
+        );
+    }
+
+    /**
+     * Commit a transaction if changes exist, otherwise rollback the
+     * transaction. Also rollback if an exception is thrown.
+     *
+     * @throws \Exception
      */
-    private function formatId($id)
+    private function commitTransaction()
     {
-        return sprintf('%s:%s', $this->sessionName, $id);
+        try {
+            if (!$this->transaction->writer()->isEmpty()) {
+                $this->transaction->writer()->commit($this->options['commit']);
+            } else {
+                // trigger rollback if no writes exist.
+                $this->transaction->writer()->rollback($this->options['rollback']);
+            }
+        } catch (\Exception $e) {
+            $this->transaction->writer()->rollback($this->options['rollback']);
+
+            throw $e;
+        }
     }
 }
diff --git a/Firestore/tests/Snippet/FirestoreSessionHandlerTest.php b/Firestore/tests/Snippet/FirestoreSessionHandlerTest.php
index 10ce2fd2f953..da7610db129f 100644
--- a/Firestore/tests/Snippet/FirestoreSessionHandlerTest.php
+++ b/Firestore/tests/Snippet/FirestoreSessionHandlerTest.php
@@ -39,24 +39,19 @@ class FirestoreSessionHandlerTest extends SnippetTestCase
     private $connection;
     private $client;
 
-    public static function setUpBeforeClass()
-    {
-        parent::setUpBeforeClass();
-
-        // Since the tests in this class must run in isolation, they won't be
-        // recognized as having been covered, and will cause a CI error.
-        // We can call `snippetFromClass` in the parent process to mark the
-        // snippets as having been covered.
-        self::snippetFromClass(FirestoreSessionHandler::class);
-        self::snippetFromClass(FirestoreSessionHandler::class, 1);
-    }
-
     public function setUp()
     {
         $this->checkAndSkipGrpcTests();
 
         $this->connection = $this->prophesize(ConnectionInterface::class);
         $this->client = TestHelpers::stub(FirestoreClient::class);
+
+        // Since the tests in this class must run in isolation, they won't be
+        // recognized as having been covered, and will cause a CI error.
+        // We can call `snippetFromClass` in the parent process to mark the
+        // snippets as having been covered.
+        $this->snippetFromClass(FirestoreSessionHandler::class);
+        $this->snippetFromClass(FirestoreSessionHandler::class, 1);
     }
 
     public function testClass()
@@ -137,5 +132,7 @@ public function testClassErrorHandler()
         $snippet->addLocal('firestore', $this->client);
 
         $res = $snippet->invoke();
+
+        $this->assertEquals('Bob', $res->output());
     }
 }
diff --git a/Firestore/tests/System/FirestoreSessionHandlerTest.php b/Firestore/tests/System/FirestoreSessionHandlerTest.php
index a37cde387170..bd5e6ac4542b 100644
--- a/Firestore/tests/System/FirestoreSessionHandlerTest.php
+++ b/Firestore/tests/System/FirestoreSessionHandlerTest.php
@@ -35,7 +35,7 @@ public function testSessionHandler()
         $content = 'foo';
         $storedValue = 'name|' . serialize($content);
 
-        $handler = new FirestoreSessionHandler($client);
+        $handler = $client->sessionHandler();
 
         session_set_save_handler($handler, true);
         session_save_path($namespace);
@@ -50,7 +50,7 @@ public function testSessionHandler()
         $hasDocument = false;
         $query = $client->collection($namespace);
         foreach ($query->documents() as $snapshot) {
-            self::$deletionQueue->add($snapshot->reference());
+            self::$localDeletionQueue->add($snapshot->reference());
             if (!$hasDocument) {
                 $hasDocument = $snapshot['data'] === $storedValue;
             }

From 2ab3eb08661a09cb661d698dfb770786e128e7f9 Mon Sep 17 00:00:00 2001
From: Brent Shaffer <betterbrent@google.com>
Date: Wed, 18 Sep 2019 18:21:24 -0700
Subject: [PATCH 09/20] first round of test fixes

---
 Firestore/src/FirestoreClient.php             |   6 +-
 Firestore/src/FirestoreSessionHandler.php     | 102 +--
 .../Unit/FirestoreSessionHandlerTest.php      | 613 +++++++++---------
 3 files changed, 379 insertions(+), 342 deletions(-)

diff --git a/Firestore/src/FirestoreClient.php b/Firestore/src/FirestoreClient.php
index 6839c0b0f0eb..27bd7dc53000 100644
--- a/Firestore/src/FirestoreClient.php
+++ b/Firestore/src/FirestoreClient.php
@@ -603,10 +603,8 @@ public function sessionHandler(array $options = [])
         return new FirestoreSessionHandler(
             $this->connection,
             $this->valueMapper,
-            $this->databaseName(
-                $this->projectId,
-                $this->database
-            ),
+            $this->projectId,
+            $this->database
             $options
         );
     }
diff --git a/Firestore/src/FirestoreSessionHandler.php b/Firestore/src/FirestoreSessionHandler.php
index 5d240419a9ae..114daae6089d 100644
--- a/Firestore/src/FirestoreSessionHandler.php
+++ b/Firestore/src/FirestoreSessionHandler.php
@@ -112,6 +112,8 @@
  */
 class FirestoreSessionHandler implements SessionHandlerInterface
 {
+    use SnapshotTrait;
+
     /**
      * @var ConnectionInterface
      */
@@ -120,6 +122,10 @@ class FirestoreSessionHandler implements SessionHandlerInterface
      * @var ValueMapper
      */
     private $valueMapper;
+    /**
+     * @var string
+     */
+    private $projectId;
     /**
      * @var string
      */
@@ -146,17 +152,20 @@ class FirestoreSessionHandler implements SessionHandlerInterface
      *
      * @param ConnectionInterface $connection A Connection to Cloud Firestore.
      * @param ValueMapper $valueMapper A Firestore Value Mapper.
-     * @param string $database The current database
+     * @param string $projectId The current project id.
+     * @param string $database The database id.
      * @param array $options [optional]
      */
     public function __construct(
         ConnectionInterface $connection,
         ValueMapper $valueMapper,
+        $projectId,
         $database,
         array $options = []
     ) {
         $this->connection = $connection;
         $this->valueMapper = $valueMapper;
+        $this->projectId = $projectId;
         $this->database = $database;
         $this->options = $options + [
             'begin' => [],
@@ -183,15 +192,16 @@ public function open($savePath, $sessionName)
     {
         $this->savePath = $savePath;
         $this->sessionName = $sessionName;
+        $database = $this->databaseName($this->projectId, $this->database);
 
         $beginTransaction = $this->connection->beginTransaction([
-            'database' => $this->database,
+            'database' => $database
         ] + $this->options['begin']);
 
         $this->transaction = new Transaction(
             $this->connection,
             $this->valueMapper,
-            $this->database,
+            $database,
             $beginTransaction['transaction']
         );
 
@@ -215,7 +225,14 @@ public function close()
     public function read($id)
     {
         try {
-            $snapshot = $this->transaction->snapshot($this->docRef($id));
+            $docRef = $this->getDocumentReference(
+                $this->connection,
+                $this->valueMapper,
+                $this->projectId,
+                $this->database,
+                $this->formatId($id)
+            );
+            $snapshot = $this->transaction->snapshot($docRef);
             if ($snapshot->exists() && isset($snapshot['data'])) {
                 return $snapshot->get('data');
             }
@@ -238,7 +255,14 @@ public function read($id)
     public function write($id, $data)
     {
         try {
-            $this->transaction->set($this->docRef($id), [
+            $docRef = $this->getDocumentReference(
+                $this->connection,
+                $this->valueMapper,
+                $this->projectId,
+                $this->database,
+                $this->formatId($id)
+            );
+            $this->transaction->set($docRef, [
                 'data' => $data,
                 't' => time()
             ]);
@@ -262,10 +286,14 @@ public function write($id, $data)
     public function destroy($id)
     {
         try {
-            $this->transaction->delete(
-                $this->docRef($id),
-                $this->options['delete']
+            $docRef = $this->getDocumentReference(
+                $this->connection,
+                $this->valueMapper,
+                $this->projectId,
+                $this->database,
+                $this->formatId($id)
             );
+            $this->transaction->delete($docRef, $this->options['delete']);
             $this->commitTransaction();
         } catch (ServiceException $e) {
             trigger_error(
@@ -290,7 +318,14 @@ public function gc($maxlifetime)
             return true;
         }
         try {
-            $query = $this->collectionRef()
+            $collectionRef = $this->getCollectionReference(
+                $this->connection,
+                $this->valueMapper,
+                $this->projectId,
+                $this->database,
+                $this->savePath
+            );
+            $query = $collectionRef
                 ->limit($this->gcLimit)
                 ->orderBy('t')
                 ->where('t', '<', time() - $maxlifetime)
@@ -314,43 +349,6 @@ public function gc($maxlifetime)
         return true;
     }
 
-    /**
-     * Returns a Firestore document reference for the provided PHP session ID.
-     *
-     * @param string $id Identifier used for the session
-     * @return DocumentReference
-     */
-    private function docRef($id)
-    {
-        // The Firebase document name is derived from the session ID and session
-        // path, ex: "PHPSESSID:abcdef".
-        $collectionRef = $this->collectionRef();
-        $parent = $collectionRef->name();
-        $name = sprintf('%s/%s:%s', $parent, $this->sessionName, $id);
-        return new DocumentReference(
-            $this->connection,
-            $this->valueMapper,
-            $collectionRef,
-            $name
-        );
-    }
-
-    /**
-     * Returns a Firestore collection reference for the provided PHP session ID.
-     *
-     * @return CollectionReference
-     */
-    private function collectionRef()
-    {
-        // The Firebase collection path is derived from the save path.
-        $name = sprintf('%s/documents/%s', $this->database, $this->savePath);
-        return new CollectionReference(
-            $this->connection,
-            $this->valueMapper,
-            $name
-        );
-    }
-
     /**
      * Commit a transaction if changes exist, otherwise rollback the
      * transaction. Also rollback if an exception is thrown.
@@ -372,4 +370,16 @@ private function commitTransaction()
             throw $e;
         }
     }
+
+    /**
+     * Format the Firebase document ID from the PHP session ID and session name.
+     * ex: PHPSESSID:abcdef
+     *
+     * @param string $id Identifier used for the session
+     * @return string
+     */
+    private function formatId($id)
+    {
+        return sprintf('%s/%s:%s', $this->savePath, $this->sessionName, $id);
+    }
 }
diff --git a/Firestore/tests/Unit/FirestoreSessionHandlerTest.php b/Firestore/tests/Unit/FirestoreSessionHandlerTest.php
index 42d97bcc1f1c..f24c1dd7b64a 100644
--- a/Firestore/tests/Unit/FirestoreSessionHandlerTest.php
+++ b/Firestore/tests/Unit/FirestoreSessionHandlerTest.php
@@ -22,12 +22,14 @@
 use Google\Cloud\Firestore\CollectionReference;
 use Google\Cloud\Firestore\DocumentReference;
 use Google\Cloud\Firestore\DocumentSnapshot;
-use Google\Cloud\Firestore\FirestoreClient;
+use Google\Cloud\Firestore\Connection\ConnectionInterface;
+use Google\Cloud\Firestore\ValueMapper;
 use Google\Cloud\Firestore\FirestoreSessionHandler;
 use Google\Cloud\Firestore\Transaction;
 use InvalidArgumentException;
 use PHPUnit\Framework\TestCase;
 use Prophecy\Argument;
+use Iterator;
 
 /**
  * @group firestore
@@ -37,23 +39,30 @@ class FirestoreSessionHandlerTest extends TestCase
 {
     const SESSION_SAVE_PATH = 'sessions';
     const SESSION_NAME = 'PHPSESID';
+    const PROJECT = 'example_project';
+    const DATABASE = '(default)';
 
-    private $firestore;
-    private $collection;
+    private $connection;
+    private $valueMapper;
+    private $documents;
 
     public function setUp()
     {
-        $this->firestore = $this->prophesize(FirestoreClient::class);
-        $this->collection = $this->prophesize(CollectionReference::class);
+        $this->connection = $this->prophesize(ConnectionInterface::class);
+        $this->valueMapper = $this->prophesize(ValueMapper::class);
+        $this->documents = $this->prophesize(Iterator::class);
     }
 
     public function testOpen()
     {
-        $this->firestore->collection(self::SESSION_SAVE_PATH)
-            ->shouldBeCalledTimes(1)
-            ->willReturn($this->collection->reveal());
+        $db = sprintf('projects/%s/databases/%s', self::PROJECT, self::DATABASE);
+        $this->connection->beginTransaction(['database' => $db])
+            ->shouldBeCalledTimes(1);
         $firestoreSessionHandler = new FirestoreSessionHandler(
-            $this->firestore->reveal()
+            $this->connection->reveal(),
+            $this->valueMapper->reveal(),
+            self::PROJECT,
+            self::DATABASE
         );
         $ret = $firestoreSessionHandler->open(self::SESSION_SAVE_PATH, self::SESSION_NAME);
         $this->assertTrue($ret);
@@ -62,22 +71,25 @@ public function testOpen()
     /**
      * @expectedException InvalidArgumentException
      */
-    public function testOpenNotAllowed()
+    public function testReadNotAllowed()
     {
-        $this->firestore->collection('invalid/savepath')
-            ->shouldBeCalledTimes(1)
-            ->willThrow(new InvalidArgumentException());
         $firestoreSessionHandler = new FirestoreSessionHandler(
-            $this->firestore->reveal()
+            $this->connection->reveal(),
+            $this->valueMapper->reveal(),
+            self::PROJECT,
+            self::DATABASE
         );
         $firestoreSessionHandler->open('invalid/savepath', self::SESSION_NAME);
+        $firestoreSessionHandler->read('sessionid');
     }
 
-
     public function testClose()
     {
         $firestoreSessionHandler = new FirestoreSessionHandler(
-            $this->firestore->reveal()
+            $this->connection->reveal(),
+            $this->valueMapper->reveal(),
+            self::PROJECT,
+            self::DATABASE
         );
         $ret = $firestoreSessionHandler->close();
         $this->assertTrue($ret);
@@ -85,22 +97,25 @@ public function testClose()
 
     public function testReadNothing()
     {
-        $this->firestore->collection(self::SESSION_SAVE_PATH)
+        $db = sprintf('projects/%s/databases/%s', self::PROJECT, self::DATABASE);
+        $doc = sprintf('%s/documents/%s/%s:sessionid', $db, self::SESSION_SAVE_PATH, self::SESSION_NAME);
+        $this->documents->current()
             ->shouldBeCalledTimes(1)
-            ->willReturn($this->collection->reveal());
-        $snapshot = $this->prophesize(DocumentSnapshot::class);
-        $snapshot->exists()
-            ->shouldBeCalledTimes(1)
-            ->willReturn(false);
-        $docRef = $this->prophesize(DocumentReference::class);
-        $docRef->snapshot()
-            ->shouldBeCalledTimes(1)
-            ->willReturn($snapshot);
-        $this->collection->document(self::SESSION_NAME . ':sessionid')
+            ->willReturn(null);
+        $this->connection->beginTransaction(['database' => $db])
+            ->shouldBeCalledTimes(1);
+        $this->connection->batchGetDocuments([
+            'database' => $db,
+            'documents' => [$doc],
+            'transaction' => null,
+        ])
             ->shouldBeCalledTimes(1)
-            ->willReturn($docRef);
+            ->willReturn($this->documents->reveal());
         $firestoreSessionHandler = new FirestoreSessionHandler(
-            $this->firestore->reveal()
+            $this->connection->reveal(),
+            $this->valueMapper->reveal(),
+            self::PROJECT,
+            self::DATABASE
         );
         $firestoreSessionHandler->open(self::SESSION_SAVE_PATH, self::SESSION_NAME);
         $ret = $firestoreSessionHandler->read('sessionid');
@@ -113,16 +128,23 @@ public function testReadNothing()
      */
     public function testReadWithException()
     {
-        $this->firestore->collection(self::SESSION_SAVE_PATH)
+        $db = sprintf('projects/%s/databases/%s', self::PROJECT, self::DATABASE);
+        $doc = sprintf('%s/documents/%s/%s:sessionid', $db, self::SESSION_SAVE_PATH, self::SESSION_NAME);
+        $this->connection->beginTransaction(['database' => $db])
+            ->shouldBeCalledTimes(1);
+        $this->connection->batchGetDocuments([
+            'database' => $db,
+            'documents' => [$doc],
+            'transaction' => null,
+        ])
             ->shouldBeCalledTimes(1)
-            ->willReturn($this->collection->reveal());
+            ->willThrow((new ServiceException('')));
         $firestoreSessionHandler = new FirestoreSessionHandler(
-            $this->firestore->reveal()
+            $this->connection->reveal(),
+            $this->valueMapper->reveal(),
+            self::PROJECT,
+            self::DATABASE
         );
-        $this->collection->document(self::SESSION_NAME . ':sessionid')
-            ->shouldBeCalledTimes(1)
-            ->willThrow((new ServiceException('')));
-
         $firestoreSessionHandler->open(self::SESSION_SAVE_PATH, self::SESSION_NAME);
         $ret = $firestoreSessionHandler->read('sessionid');
 
@@ -131,269 +153,276 @@ public function testReadWithException()
 
     public function testReadEntity()
     {
-        $id = self::SESSION_NAME . ':sessionid';
-        $snapshot = $this->prophesize(DocumentSnapshot::class);
-        $snapshot->exists()
-            ->shouldBeCalledTimes(1)
-            ->willReturn(true);
-        $snapshot->offsetExists('data')
-            ->shouldBeCalledTimes(1)
-            ->willReturn(true);
-        $snapshot->get('data')
-            ->shouldBeCalledTimes(1)
-            ->willReturn('sessiondata');
-        $docRef = $this->prophesize(DocumentReference::class);
-        $docRef->snapshot()
-            ->shouldBeCalledTimes(1)
-            ->willReturn($snapshot);
-        $this->collection->document($id)
-            ->shouldBeCalledTimes(1)
-            ->willReturn($docRef);
-        $this->firestore->collection(self::SESSION_SAVE_PATH)
-            ->shouldBeCalledTimes(1)
-            ->willReturn($this->collection->reveal());
-        $firestoreSessionHandler = new FirestoreSessionHandler(
-            $this->firestore->reveal()
-        );
-        $firestoreSessionHandler->open(self::SESSION_SAVE_PATH, self::SESSION_NAME);
-        $ret = $firestoreSessionHandler->read('sessionid');
-
-        $this->assertEquals('sessiondata', $ret);
-    }
-
-    public function testWrite()
-    {
-        $id = self::SESSION_NAME . ':sessionid';
-        $docRef = $this->prophesize(DocumentReference::class);
-        $this->firestore->collection(self::SESSION_SAVE_PATH)
-            ->shouldBeCalledTimes(1)
-            ->willReturn($this->collection->reveal());
-        $phpunit = $this;
-        $this->firestore->runTransaction(Argument::type('closure'))
-            ->shouldBeCalledTimes(1)
-            ->will(function ($args) use ($phpunit, $docRef) {
-                $transaction = $phpunit->prophesize(Transaction::class);
-                $transaction->set($docRef, Argument::type('array'))
-                    ->will(function ($args) use ($phpunit) {
-                        $phpunit->assertEquals('sessiondata', $args[1]['data']);
-                        $phpunit->assertInternalType('int', $args[1]['t']);
-                        $phpunit->assertGreaterThanOrEqual($args[1]['t'], time());
-                        // 2 seconds grace period should be enough
-                        $phpunit->assertLessThanOrEqual(2, time() - $args[1]['t']);
-                    });
-                $args[0]($transaction->reveal());
-            });
-        $this->collection->document($id)
-            ->shouldBeCalledTimes(1)
-            ->willReturn($docRef);
-        $firestoreSessionHandler = new FirestoreSessionHandler(
-            $this->firestore->reveal()
-        );
-        $firestoreSessionHandler->open(self::SESSION_SAVE_PATH, self::SESSION_NAME);
-        $ret = $firestoreSessionHandler->write('sessionid', 'sessiondata');
-
-        $this->assertTrue($ret);
-    }
-
-    /**
-     * @expectedException PHPUnit_Framework_Error_Warning
-     */
-    public function testWriteWithException()
-    {
-        $id = self::SESSION_NAME . ':sessionid';
-        $transaction = $this->prophesize(Transaction::class);
-        $docRef = $this->prophesize(DocumentReference::class);
-        $this->firestore->collection(self::SESSION_SAVE_PATH)
-            ->shouldBeCalledTimes(1)
-            ->willReturn($this->collection->reveal());
-        $this->firestore->runTransaction(Argument::any())
-            ->shouldBeCalledTimes(1)
-            ->willThrow(new ServiceException(''));
-        $this->collection->document($id)
-            ->shouldBeCalledTimes(1)
-            ->willReturn($docRef);
-
-        $firestoreSessionHandler = new FirestoreSessionHandler(
-            $this->firestore->reveal()
-        );
-        $firestoreSessionHandler->open(self::SESSION_SAVE_PATH, self::SESSION_NAME);
-        $ret = $firestoreSessionHandler->write('sessionid', 'sessiondata');
-
-        $this->assertFalse($ret);
-    }
-
-    public function testDestroy()
-    {
-        $id = self::SESSION_NAME . ':sessionid';
-        $docRef = $this->prophesize(DocumentReference::class);
-        $docRef->delete()
+        $db = sprintf('projects/%s/databases/%s', self::PROJECT, self::DATABASE);
+        $doc = sprintf('%s/documents/%s/%s:sessionid', $db, self::SESSION_SAVE_PATH, self::SESSION_NAME);
+        $this->documents->current()
+            ->shouldBeCalledTimes(1)
+            ->willReturn([
+                'found' => [
+                    'createTime' => date('Y-m-d'),
+                    'updateTime' => date('Y-m-d'),
+                    'readTime' => date('Y-m-d'),
+                    'fields' => ['data' => 'sessiondata']
+                ]
+            ]);
+        $this->valueMapper->decodeValues(['data' => 'sessiondata'])
+            ->shouldBeCalledTimes(1)
+            ->willReturn(['data' => 'sessiondata']);
+        $this->connection->beginTransaction(['database' => $db])
             ->shouldBeCalledTimes(1);
-        $this->firestore->collection(self::SESSION_SAVE_PATH)
+        $this->connection->batchGetDocuments([
+            'database' => $db,
+            'documents' => [$doc],
+            'transaction' => null,
+        ])
             ->shouldBeCalledTimes(1)
-            ->willReturn($this->collection->reveal());
-        $this->collection->document($id)
-            ->shouldBeCalledTimes(1)
-            ->willReturn($docRef);
+            ->willReturn($this->documents->reveal());
         $firestoreSessionHandler = new FirestoreSessionHandler(
-            $this->firestore->reveal()
-        );
-        $firestoreSessionHandler->open(self::SESSION_SAVE_PATH, self::SESSION_NAME);
-        $ret = $firestoreSessionHandler->destroy('sessionid');
-
-        $this->assertTrue($ret);
-    }
-
-    /**
-     * @expectedException PHPUnit_Framework_Error_Warning
-     */
-    public function testDestroyWithException()
-    {
-        $id = self::SESSION_NAME . ':sessionid';
-        $docRef = $this->prophesize(DocumentReference::class);
-        $docRef->delete()
-            ->shouldBeCalledTimes(1)
-            ->willThrow(new ServiceException(''));
-        $this->firestore->collection(self::SESSION_SAVE_PATH)
-            ->shouldBeCalledTimes(1)
-            ->willReturn($this->collection->reveal());
-        $this->collection->document($id)
-            ->shouldBeCalledTimes(1)
-            ->willReturn($docRef);
-        $firestoreSessionHandler = new FirestoreSessionHandler(
-            $this->firestore->reveal()
-        );
-        $firestoreSessionHandler->open(self::SESSION_SAVE_PATH, self::SESSION_NAME);
-        $ret = $firestoreSessionHandler->destroy('sessionid');
-
-        $this->assertFalse($ret);
-    }
-
-    public function testDefaultGcDoesNothing()
-    {
-        $this->firestore->collection(self::SESSION_SAVE_PATH)
-            ->shouldBeCalledTimes(1)
-            ->willReturn($this->collection->reveal());
-        $this->collection->limit()->shouldNotBeCalled();
-        $firestoreSessionHandler = new FirestoreSessionHandler(
-            $this->firestore->reveal()
-        );
-        $firestoreSessionHandler->open(self::SESSION_SAVE_PATH, self::SESSION_NAME);
-        $ret = $firestoreSessionHandler->gc(100);
-
-        $this->assertTrue($ret);
-    }
-
-    public function testGc()
-    {
-        $docRef1 = $this->prophesize(DocumentReference::class);
-        $docRef1->delete()
-            ->shouldBeCalledTimes(1);
-        $snapshot1 = $this->prophesize(DocumentSnapshot::class);
-        $snapshot1->reference()
-            ->shouldBeCalledTimes(1)
-            ->willReturn($docRef1);
-        $docRef2 = $this->prophesize(DocumentReference::class);
-        $docRef2->delete()
-            ->shouldBeCalledTimes(1);
-        $snapshot2 = $this->prophesize(DocumentSnapshot::class);
-        $snapshot2->reference()
-            ->shouldBeCalledTimes(1)
-            ->willReturn($docRef2);
-        $phpunit = $this;
-        $collection = $this->collection;
-        $collection->where(
-            Argument::type('string'),
-            Argument::type('string'),
-            Argument::type('int')
-        )
-            ->shouldBeCalledTimes(1)
-            ->will(function ($args) use ($phpunit, $collection) {
-                $phpunit->assertEquals('t', $args[0]);
-                $phpunit->assertEquals('<', $args[1]);
-                $phpunit->assertInternalType('int', $args[2]);
-                $diff = time() - $args[2];
-                // 2 seconds grace period should be enough
-                $phpunit->assertLessThanOrEqual(102, $diff);
-                $phpunit->assertGreaterThanOrEqual(100, $diff);
-                return $collection->reveal();
-            });
-
-        $collection->orderBy('t')
-            ->shouldBeCalledTimes(1)
-            ->willReturn($collection->reveal());
-        $collection->limit(1000)
-            ->shouldBeCalledTimes(1)
-            ->willReturn($collection->reveal());
-        $collection->documents()
-            ->shouldBeCalledTimes(1)
-            ->willReturn([$snapshot1, $snapshot2]);
-        $this->firestore->collection(self::SESSION_SAVE_PATH)
-            ->shouldBeCalledTimes(1)
-            ->willReturn($collection->reveal());
-
-        $firestoreSessionHandler = new FirestoreSessionHandler(
-            $this->firestore->reveal(),
-            1000
+            $this->connection->reveal(),
+            $this->valueMapper->reveal(),
+            self::PROJECT,
+            self::DATABASE
         );
 
         $firestoreSessionHandler->open(self::SESSION_SAVE_PATH, self::SESSION_NAME);
-        $ret = $firestoreSessionHandler->gc(100);
+        $ret = $firestoreSessionHandler->read('sessionid');
 
-        $this->assertTrue($ret);
+        $this->assertEquals('sessiondata', $ret);
     }
 
-    /**
-     * @expectedException PHPUnit_Framework_Error_Warning
-     */
-    public function testGcWithException()
-    {
-        $docRef = $this->prophesize(DocumentReference::class);
-        $docRef->delete()
-            ->shouldBeCalledTimes(1)
-            ->willThrow(new ServiceException(''));
-        $snapshot = $this->prophesize(DocumentSnapshot::class);
-        $snapshot->reference()
-            ->shouldBeCalledTimes(1)
-            ->willReturn($docRef);
-        $phpunit = $this;
-        $collection = $this->collection;
-        $collection->where(
-            Argument::type('string'),
-            Argument::type('string'),
-            Argument::type('int')
-        )
-            ->shouldBeCalledTimes(1)
-            ->will(function ($args) use ($phpunit, $collection) {
-                $phpunit->assertEquals('t', $args[0]);
-                $phpunit->assertEquals('<', $args[1]);
-                $phpunit->assertInternalType('int', $args[2]);
-                $diff = time() - $args[2];
-                // 2 seconds grace period should be enough
-                $phpunit->assertLessThanOrEqual(102, $diff);
-                $phpunit->assertGreaterThanOrEqual(100, $diff);
-                return $collection->reveal();
-            });
-
-        $collection->orderBy('t')
-            ->shouldBeCalledTimes(1)
-            ->willReturn($collection->reveal());
-        $collection->limit(1000)
-            ->shouldBeCalledTimes(1)
-            ->willReturn($collection->reveal());
-        $collection->documents()
-            ->shouldBeCalledTimes(1)
-            ->willReturn([$snapshot]);
-        $this->firestore->collection(self::SESSION_SAVE_PATH)
-            ->shouldBeCalledTimes(1)
-            ->willReturn($collection->reveal());
-        $firestoreSessionHandler = new FirestoreSessionHandler(
-            $this->firestore->reveal(),
-            1000
-        );
-
-        $firestoreSessionHandler->open(self::SESSION_SAVE_PATH, self::SESSION_NAME);
-        $ret = $firestoreSessionHandler->gc(100);
-
-        $this->assertFalse($ret);
-    }
+    // public function testWrite()
+    // {
+    //     $id = self::SESSION_NAME . ':sessionid';
+    //     $docRef = $this->prophesize(DocumentReference::class);
+    //     $this->firestore->collection(self::SESSION_SAVE_PATH)
+    //         ->shouldBeCalledTimes(1)
+    //         ->willReturn($this->collection->reveal());
+    //     $phpunit = $this;
+    //     $this->firestore->runTransaction(Argument::type('closure'))
+    //         ->shouldBeCalledTimes(1)
+    //         ->will(function ($args) use ($phpunit, $docRef) {
+    //             $transaction = $phpunit->prophesize(Transaction::class);
+    //             $transaction->set($docRef, Argument::type('array'))
+    //                 ->will(function ($args) use ($phpunit) {
+    //                     $phpunit->assertEquals('sessiondata', $args[1]['data']);
+    //                     $phpunit->assertInternalType('int', $args[1]['t']);
+    //                     $phpunit->assertGreaterThanOrEqual($args[1]['t'], time());
+    //                     // 2 seconds grace period should be enough
+    //                     $phpunit->assertLessThanOrEqual(2, time() - $args[1]['t']);
+    //                 });
+    //             $args[0]($transaction->reveal());
+    //         });
+    //     $this->collection->document($id)
+    //         ->shouldBeCalledTimes(1)
+    //         ->willReturn($docRef);
+    //     $firestoreSessionHandler = new FirestoreSessionHandler(
+    //         $this->firestore->reveal()
+    //     );
+    //     $firestoreSessionHandler->open(self::SESSION_SAVE_PATH, self::SESSION_NAME);
+    //     $ret = $firestoreSessionHandler->write('sessionid', 'sessiondata');
+
+    //     $this->assertTrue($ret);
+    // }
+
+    // /**
+    //  * @expectedException PHPUnit_Framework_Error_Warning
+    //  */
+    // public function testWriteWithException()
+    // {
+    //     $id = self::SESSION_NAME . ':sessionid';
+    //     $transaction = $this->prophesize(Transaction::class);
+    //     $docRef = $this->prophesize(DocumentReference::class);
+    //     $this->firestore->collection(self::SESSION_SAVE_PATH)
+    //         ->shouldBeCalledTimes(1)
+    //         ->willReturn($this->collection->reveal());
+    //     $this->firestore->runTransaction(Argument::any())
+    //         ->shouldBeCalledTimes(1)
+    //         ->willThrow(new ServiceException(''));
+    //     $this->collection->document($id)
+    //         ->shouldBeCalledTimes(1)
+    //         ->willReturn($docRef);
+
+    //     $firestoreSessionHandler = new FirestoreSessionHandler(
+    //         $this->firestore->reveal()
+    //     );
+    //     $firestoreSessionHandler->open(self::SESSION_SAVE_PATH, self::SESSION_NAME);
+    //     $ret = $firestoreSessionHandler->write('sessionid', 'sessiondata');
+
+    //     $this->assertFalse($ret);
+    // }
+
+    // public function testDestroy()
+    // {
+    //     $id = self::SESSION_NAME . ':sessionid';
+    //     $docRef = $this->prophesize(DocumentReference::class);
+    //     $docRef->delete()
+    //         ->shouldBeCalledTimes(1);
+    //     $this->firestore->collection(self::SESSION_SAVE_PATH)
+    //         ->shouldBeCalledTimes(1)
+    //         ->willReturn($this->collection->reveal());
+    //     $this->collection->document($id)
+    //         ->shouldBeCalledTimes(1)
+    //         ->willReturn($docRef);
+    //     $firestoreSessionHandler = new FirestoreSessionHandler(
+    //         $this->firestore->reveal()
+    //     );
+    //     $firestoreSessionHandler->open(self::SESSION_SAVE_PATH, self::SESSION_NAME);
+    //     $ret = $firestoreSessionHandler->destroy('sessionid');
+
+    //     $this->assertTrue($ret);
+    // }
+
+    // /**
+    //  * @expectedException PHPUnit_Framework_Error_Warning
+    //  */
+    // public function testDestroyWithException()
+    // {
+    //     $id = self::SESSION_NAME . ':sessionid';
+    //     $docRef = $this->prophesize(DocumentReference::class);
+    //     $docRef->delete()
+    //         ->shouldBeCalledTimes(1)
+    //         ->willThrow(new ServiceException(''));
+    //     $this->firestore->collection(self::SESSION_SAVE_PATH)
+    //         ->shouldBeCalledTimes(1)
+    //         ->willReturn($this->collection->reveal());
+    //     $this->collection->document($id)
+    //         ->shouldBeCalledTimes(1)
+    //         ->willReturn($docRef);
+    //     $firestoreSessionHandler = new FirestoreSessionHandler(
+    //         $this->firestore->reveal()
+    //     );
+    //     $firestoreSessionHandler->open(self::SESSION_SAVE_PATH, self::SESSION_NAME);
+    //     $ret = $firestoreSessionHandler->destroy('sessionid');
+
+    //     $this->assertFalse($ret);
+    // }
+
+    // public function testDefaultGcDoesNothing()
+    // {
+    //     $this->firestore->collection(self::SESSION_SAVE_PATH)
+    //         ->shouldBeCalledTimes(1)
+    //         ->willReturn($this->collection->reveal());
+    //     $this->collection->limit()->shouldNotBeCalled();
+    //     $firestoreSessionHandler = new FirestoreSessionHandler(
+    //         $this->firestore->reveal()
+    //     );
+    //     $firestoreSessionHandler->open(self::SESSION_SAVE_PATH, self::SESSION_NAME);
+    //     $ret = $firestoreSessionHandler->gc(100);
+
+    //     $this->assertTrue($ret);
+    // }
+
+    // public function testGc()
+    // {
+    //     $docRef1 = $this->prophesize(DocumentReference::class);
+    //     $docRef1->delete()
+    //         ->shouldBeCalledTimes(1);
+    //     $snapshot1 = $this->prophesize(DocumentSnapshot::class);
+    //     $snapshot1->reference()
+    //         ->shouldBeCalledTimes(1)
+    //         ->willReturn($docRef1);
+    //     $docRef2 = $this->prophesize(DocumentReference::class);
+    //     $docRef2->delete()
+    //         ->shouldBeCalledTimes(1);
+    //     $snapshot2 = $this->prophesize(DocumentSnapshot::class);
+    //     $snapshot2->reference()
+    //         ->shouldBeCalledTimes(1)
+    //         ->willReturn($docRef2);
+    //     $phpunit = $this;
+    //     $collection = $this->collection;
+    //     $collection->where(
+    //         Argument::type('string'),
+    //         Argument::type('string'),
+    //         Argument::type('int')
+    //     )
+    //         ->shouldBeCalledTimes(1)
+    //         ->will(function ($args) use ($phpunit, $collection) {
+    //             $phpunit->assertEquals('t', $args[0]);
+    //             $phpunit->assertEquals('<', $args[1]);
+    //             $phpunit->assertInternalType('int', $args[2]);
+    //             $diff = time() - $args[2];
+    //             // 2 seconds grace period should be enough
+    //             $phpunit->assertLessThanOrEqual(102, $diff);
+    //             $phpunit->assertGreaterThanOrEqual(100, $diff);
+    //             return $collection->reveal();
+    //         });
+
+    //     $collection->orderBy('t')
+    //         ->shouldBeCalledTimes(1)
+    //         ->willReturn($collection->reveal());
+    //     $collection->limit(1000)
+    //         ->shouldBeCalledTimes(1)
+    //         ->willReturn($collection->reveal());
+    //     $collection->documents()
+    //         ->shouldBeCalledTimes(1)
+    //         ->willReturn([$snapshot1, $snapshot2]);
+    //     $this->firestore->collection(self::SESSION_SAVE_PATH)
+    //         ->shouldBeCalledTimes(1)
+    //         ->willReturn($collection->reveal());
+
+    //     $firestoreSessionHandler = new FirestoreSessionHandler(
+    //         $this->firestore->reveal(),
+    //         1000
+    //     );
+
+    //     $firestoreSessionHandler->open(self::SESSION_SAVE_PATH, self::SESSION_NAME);
+    //     $ret = $firestoreSessionHandler->gc(100);
+
+    //     $this->assertTrue($ret);
+    // }
+
+    // /**
+    //  * @expectedException PHPUnit_Framework_Error_Warning
+    //  */
+    // public function testGcWithException()
+    // {
+    //     $docRef = $this->prophesize(DocumentReference::class);
+    //     $docRef->delete()
+    //         ->shouldBeCalledTimes(1)
+    //         ->willThrow(new ServiceException(''));
+    //     $snapshot = $this->prophesize(DocumentSnapshot::class);
+    //     $snapshot->reference()
+    //         ->shouldBeCalledTimes(1)
+    //         ->willReturn($docRef);
+    //     $phpunit = $this;
+    //     $collection = $this->collection;
+    //     $collection->where(
+    //         Argument::type('string'),
+    //         Argument::type('string'),
+    //         Argument::type('int')
+    //     )
+    //         ->shouldBeCalledTimes(1)
+    //         ->will(function ($args) use ($phpunit, $collection) {
+    //             $phpunit->assertEquals('t', $args[0]);
+    //             $phpunit->assertEquals('<', $args[1]);
+    //             $phpunit->assertInternalType('int', $args[2]);
+    //             $diff = time() - $args[2];
+    //             // 2 seconds grace period should be enough
+    //             $phpunit->assertLessThanOrEqual(102, $diff);
+    //             $phpunit->assertGreaterThanOrEqual(100, $diff);
+    //             return $collection->reveal();
+    //         });
+
+    //     $collection->orderBy('t')
+    //         ->shouldBeCalledTimes(1)
+    //         ->willReturn($collection->reveal());
+    //     $collection->limit(1000)
+    //         ->shouldBeCalledTimes(1)
+    //         ->willReturn($collection->reveal());
+    //     $collection->documents()
+    //         ->shouldBeCalledTimes(1)
+    //         ->willReturn([$snapshot]);
+    //     $this->firestore->collection(self::SESSION_SAVE_PATH)
+    //         ->shouldBeCalledTimes(1)
+    //         ->willReturn($collection->reveal());
+    //     $firestoreSessionHandler = new FirestoreSessionHandler(
+    //         $this->firestore->reveal(),
+    //         1000
+    //     );
+
+    //     $firestoreSessionHandler->open(self::SESSION_SAVE_PATH, self::SESSION_NAME);
+    //     $ret = $firestoreSessionHandler->gc(100);
+
+    //     $this->assertFalse($ret);
+    // }
 }

From 919bb8e2b44a80d290fd06a32884b8226ff92965 Mon Sep 17 00:00:00 2001
From: Brent Shaffer <betterbrent@google.com>
Date: Thu, 19 Sep 2019 11:02:52 -0700
Subject: [PATCH 10/20] fixes syntax error

---
 Firestore/src/FirestoreClient.php | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Firestore/src/FirestoreClient.php b/Firestore/src/FirestoreClient.php
index 27bd7dc53000..85796da8506f 100644
--- a/Firestore/src/FirestoreClient.php
+++ b/Firestore/src/FirestoreClient.php
@@ -604,7 +604,7 @@ public function sessionHandler(array $options = [])
             $this->connection,
             $this->valueMapper,
             $this->projectId,
-            $this->database
+            $this->database,
             $options
         );
     }

From ab7a5b508a182706df806388a0cbebb1d5efa7fb Mon Sep 17 00:00:00 2001
From: Brent Shaffer <betterbrent@google.com>
Date: Fri, 20 Sep 2019 09:32:55 -0700
Subject: [PATCH 11/20] adds GC test, fixes remaining tests

---
 Firestore/src/FirestoreSessionHandler.php     |  42 +-
 Firestore/src/Query.php                       |   7 +-
 .../Snippet/FirestoreSessionHandlerTest.php   |   2 +-
 .../System/FirestoreSessionHandlerTest.php    |  28 +-
 .../Unit/FirestoreSessionHandlerTest.php      | 515 +++++++++---------
 5 files changed, 323 insertions(+), 271 deletions(-)

diff --git a/Firestore/src/FirestoreSessionHandler.php b/Firestore/src/FirestoreSessionHandler.php
index 114daae6089d..31fa2df01f34 100644
--- a/Firestore/src/FirestoreSessionHandler.php
+++ b/Firestore/src/FirestoreSessionHandler.php
@@ -172,6 +172,7 @@ public function __construct(
             'commit' => [],
             'rollback' => [],
             'delete' => [],
+            'query' => [],
             'gcLimit' => 0,
         ];
 
@@ -230,7 +231,7 @@ public function read($id)
                 $this->valueMapper,
                 $this->projectId,
                 $this->database,
-                $this->formatId($id)
+                $this->docId($id)
             );
             $snapshot = $this->transaction->snapshot($docRef);
             if ($snapshot->exists() && isset($snapshot['data'])) {
@@ -260,7 +261,7 @@ public function write($id, $data)
                 $this->valueMapper,
                 $this->projectId,
                 $this->database,
-                $this->formatId($id)
+                $this->docId($id)
             );
             $this->transaction->set($docRef, [
                 'data' => $data,
@@ -291,7 +292,7 @@ public function destroy($id)
                 $this->valueMapper,
                 $this->projectId,
                 $this->database,
-                $this->formatId($id)
+                $this->docId($id)
             );
             $this->transaction->delete($docRef, $this->options['delete']);
             $this->commitTransaction();
@@ -314,7 +315,7 @@ public function destroy($id)
      */
     public function gc($maxlifetime)
     {
-        if (0 === $this->gcLimit) {
+        if (0 === $this->options['gcLimit']) {
             return true;
         }
         try {
@@ -323,15 +324,16 @@ public function gc($maxlifetime)
                 $this->valueMapper,
                 $this->projectId,
                 $this->database,
-                $this->savePath
+                $this->collectionId()
             );
             $query = $collectionRef
-                ->limit($this->gcLimit)
-                ->orderBy('t')
+                ->limit($this->options['gcLimit'])
                 ->where('t', '<', time() - $maxlifetime)
-                ->where('id', '>=', $this->sessionName . ':')
-                ->where('id', '<', $this->sessionName . chr(ord(':') + 1));
-            $querySnapshot = $this->transaction->runQuery($query);
+                ->orderBy('t');
+            $querySnapshot = $this->transaction->runQuery(
+                $query,
+                $this->options['query']
+            );
             foreach ($querySnapshot as $snapshot) {
                 $this->transaction->delete(
                     $snapshot->reference(),
@@ -364,7 +366,7 @@ private function commitTransaction()
                 // trigger rollback if no writes exist.
                 $this->transaction->writer()->rollback($this->options['rollback']);
             }
-        } catch (\Exception $e) {
+        } catch (ServiceException $e) {
             $this->transaction->writer()->rollback($this->options['rollback']);
 
             throw $e;
@@ -373,13 +375,25 @@ private function commitTransaction()
 
     /**
      * Format the Firebase document ID from the PHP session ID and session name.
-     * ex: PHPSESSID:abcdef
+     * ex: sessions:PHPSESSID/abcdef
+     *
+     * @param string $id Identifier used for the session
+     * @return string
+     */
+    private function collectionId()
+    {
+        return sprintf('%s:%s', $this->savePath, $this->sessionName);
+    }
+
+    /**
+     * Format the Firebase document ID from the PHP session ID and session name.
+     * ex: sessions:PHPSESSID/abcdef
      *
      * @param string $id Identifier used for the session
      * @return string
      */
-    private function formatId($id)
+    private function docId($id)
     {
-        return sprintf('%s/%s:%s', $this->savePath, $this->sessionName, $id);
+        return sprintf('%s/%s', $this->collectionId(), $id);
     }
 }
diff --git a/Firestore/src/Query.php b/Firestore/src/Query.php
index 01823a484cb9..b77623de799c 100644
--- a/Firestore/src/Query.php
+++ b/Firestore/src/Query.php
@@ -169,11 +169,13 @@ public function documents(array $options = [])
 
         $rows = (new ExponentialBackoff($maxRetries))->execute(function () use ($options) {
             $query = $this->finalQueryPrepare($this->query);
-            $generator = $this->connection->runQuery($this->arrayFilterRemoveNull([
+            $options2 = $this->arrayFilterRemoveNull([
                 'parent' => $this->parent,
                 'structuredQuery' => $query,
                 'retries' => 0
-            ]) + $options);
+            ]) + $options;
+
+            $generator = $this->connection->runQuery($options2);
 
             // cache collection references
             $collections = [];
@@ -207,7 +209,6 @@ public function documents(array $options = [])
 
                 $generator->next();
             }
-
             return $out;
         });
 
diff --git a/Firestore/tests/Snippet/FirestoreSessionHandlerTest.php b/Firestore/tests/Snippet/FirestoreSessionHandlerTest.php
index da7610db129f..dc66d78988f0 100644
--- a/Firestore/tests/Snippet/FirestoreSessionHandlerTest.php
+++ b/Firestore/tests/Snippet/FirestoreSessionHandlerTest.php
@@ -79,7 +79,7 @@ public function testClass()
         $value = 'name|' . serialize('Bob');
         $this->connection->commit(Argument::allOf(
             Argument::that(function ($args) use ($value) {
-                return strpos($args['writes'][0]['update']['name'], 'PHPSESSID:') !== false
+                return strpos($args['writes'][0]['update']['name'], ':PHPSESSID') !== false
                     && $args['writes'][0]['update']['fields']['data']['stringValue'] === $value
                     && isset($args['writes'][0]['update']['fields']['t']['integerValue']);
             }),
diff --git a/Firestore/tests/System/FirestoreSessionHandlerTest.php b/Firestore/tests/System/FirestoreSessionHandlerTest.php
index bd5e6ac4542b..f389a49a5b07 100644
--- a/Firestore/tests/System/FirestoreSessionHandlerTest.php
+++ b/Firestore/tests/System/FirestoreSessionHandlerTest.php
@@ -48,7 +48,7 @@ public function testSessionHandler()
         sleep(1);
 
         $hasDocument = false;
-        $query = $client->collection($namespace);
+        $query = $client->collection($namespace . ':' . session_name());
         foreach ($query->documents() as $snapshot) {
             self::$localDeletionQueue->add($snapshot->reference());
             if (!$hasDocument) {
@@ -58,4 +58,30 @@ public function testSessionHandler()
 
         $this->assertTrue($hasDocument);
     }
+
+    public function testSessionHandlerGarbageCollection()
+    {
+        $client = self::$client;
+
+        $namespace = uniqid('sess-' . self::COLLECTION_NAME);
+        $sessionName = 'PHPSESSID';
+        $collection = $client->collection($namespace . ':' . $sessionName);
+        $collection->document('foo1')->set(['data' => 'foo1', 't' => time() - 1]);
+        $collection->document('foo2')->set(['data' => 'foo2', 't' => time() - 1]);
+
+        $count = 0;
+        foreach ($collection->documents() as $doc) $count++;
+        $this->assertEquals(2, $count);
+
+        $handler = $client->sessionHandler([
+            'gcLimit' => 1000,
+            'query' => ['maxRetries' => 0]
+        ]);
+        $handler->open($namespace, $sessionName);
+        $handler->gc(0);
+
+        $count = 0;
+        foreach ($collection->documents() as $doc) $count++;
+        $this->assertEquals(0, $count);
+    }
 }
diff --git a/Firestore/tests/Unit/FirestoreSessionHandlerTest.php b/Firestore/tests/Unit/FirestoreSessionHandlerTest.php
index f24c1dd7b64a..098866de2634 100644
--- a/Firestore/tests/Unit/FirestoreSessionHandlerTest.php
+++ b/Firestore/tests/Unit/FirestoreSessionHandlerTest.php
@@ -55,8 +55,7 @@ public function setUp()
 
     public function testOpen()
     {
-        $db = sprintf('projects/%s/databases/%s', self::PROJECT, self::DATABASE);
-        $this->connection->beginTransaction(['database' => $db])
+        $this->connection->beginTransaction(['database' => $this->dbName()])
             ->shouldBeCalledTimes(1);
         $firestoreSessionHandler = new FirestoreSessionHandler(
             $this->connection->reveal(),
@@ -97,16 +96,14 @@ public function testClose()
 
     public function testReadNothing()
     {
-        $db = sprintf('projects/%s/databases/%s', self::PROJECT, self::DATABASE);
-        $doc = sprintf('%s/documents/%s/%s:sessionid', $db, self::SESSION_SAVE_PATH, self::SESSION_NAME);
         $this->documents->current()
             ->shouldBeCalledTimes(1)
             ->willReturn(null);
-        $this->connection->beginTransaction(['database' => $db])
+        $this->connection->beginTransaction(['database' => $this->dbName()])
             ->shouldBeCalledTimes(1);
         $this->connection->batchGetDocuments([
-            'database' => $db,
-            'documents' => [$doc],
+            'database' => $this->dbName(),
+            'documents' => [$this->documentName()],
             'transaction' => null,
         ])
             ->shouldBeCalledTimes(1)
@@ -128,13 +125,11 @@ public function testReadNothing()
      */
     public function testReadWithException()
     {
-        $db = sprintf('projects/%s/databases/%s', self::PROJECT, self::DATABASE);
-        $doc = sprintf('%s/documents/%s/%s:sessionid', $db, self::SESSION_SAVE_PATH, self::SESSION_NAME);
-        $this->connection->beginTransaction(['database' => $db])
+        $this->connection->beginTransaction(['database' => $this->dbName()])
             ->shouldBeCalledTimes(1);
         $this->connection->batchGetDocuments([
-            'database' => $db,
-            'documents' => [$doc],
+            'database' => $this->dbName(),
+            'documents' => [$this->documentName()],
             'transaction' => null,
         ])
             ->shouldBeCalledTimes(1)
@@ -153,8 +148,6 @@ public function testReadWithException()
 
     public function testReadEntity()
     {
-        $db = sprintf('projects/%s/databases/%s', self::PROJECT, self::DATABASE);
-        $doc = sprintf('%s/documents/%s/%s:sessionid', $db, self::SESSION_SAVE_PATH, self::SESSION_NAME);
         $this->documents->current()
             ->shouldBeCalledTimes(1)
             ->willReturn([
@@ -168,11 +161,11 @@ public function testReadEntity()
         $this->valueMapper->decodeValues(['data' => 'sessiondata'])
             ->shouldBeCalledTimes(1)
             ->willReturn(['data' => 'sessiondata']);
-        $this->connection->beginTransaction(['database' => $db])
+        $this->connection->beginTransaction(['database' => $this->dbName()])
             ->shouldBeCalledTimes(1);
         $this->connection->batchGetDocuments([
-            'database' => $db,
-            'documents' => [$doc],
+            'database' => $this->dbName(),
+            'documents' => [$this->documentName()],
             'transaction' => null,
         ])
             ->shouldBeCalledTimes(1)
@@ -190,239 +183,257 @@ public function testReadEntity()
         $this->assertEquals('sessiondata', $ret);
     }
 
-    // public function testWrite()
-    // {
-    //     $id = self::SESSION_NAME . ':sessionid';
-    //     $docRef = $this->prophesize(DocumentReference::class);
-    //     $this->firestore->collection(self::SESSION_SAVE_PATH)
-    //         ->shouldBeCalledTimes(1)
-    //         ->willReturn($this->collection->reveal());
-    //     $phpunit = $this;
-    //     $this->firestore->runTransaction(Argument::type('closure'))
-    //         ->shouldBeCalledTimes(1)
-    //         ->will(function ($args) use ($phpunit, $docRef) {
-    //             $transaction = $phpunit->prophesize(Transaction::class);
-    //             $transaction->set($docRef, Argument::type('array'))
-    //                 ->will(function ($args) use ($phpunit) {
-    //                     $phpunit->assertEquals('sessiondata', $args[1]['data']);
-    //                     $phpunit->assertInternalType('int', $args[1]['t']);
-    //                     $phpunit->assertGreaterThanOrEqual($args[1]['t'], time());
-    //                     // 2 seconds grace period should be enough
-    //                     $phpunit->assertLessThanOrEqual(2, time() - $args[1]['t']);
-    //                 });
-    //             $args[0]($transaction->reveal());
-    //         });
-    //     $this->collection->document($id)
-    //         ->shouldBeCalledTimes(1)
-    //         ->willReturn($docRef);
-    //     $firestoreSessionHandler = new FirestoreSessionHandler(
-    //         $this->firestore->reveal()
-    //     );
-    //     $firestoreSessionHandler->open(self::SESSION_SAVE_PATH, self::SESSION_NAME);
-    //     $ret = $firestoreSessionHandler->write('sessionid', 'sessiondata');
-
-    //     $this->assertTrue($ret);
-    // }
-
-    // /**
-    //  * @expectedException PHPUnit_Framework_Error_Warning
-    //  */
-    // public function testWriteWithException()
-    // {
-    //     $id = self::SESSION_NAME . ':sessionid';
-    //     $transaction = $this->prophesize(Transaction::class);
-    //     $docRef = $this->prophesize(DocumentReference::class);
-    //     $this->firestore->collection(self::SESSION_SAVE_PATH)
-    //         ->shouldBeCalledTimes(1)
-    //         ->willReturn($this->collection->reveal());
-    //     $this->firestore->runTransaction(Argument::any())
-    //         ->shouldBeCalledTimes(1)
-    //         ->willThrow(new ServiceException(''));
-    //     $this->collection->document($id)
-    //         ->shouldBeCalledTimes(1)
-    //         ->willReturn($docRef);
-
-    //     $firestoreSessionHandler = new FirestoreSessionHandler(
-    //         $this->firestore->reveal()
-    //     );
-    //     $firestoreSessionHandler->open(self::SESSION_SAVE_PATH, self::SESSION_NAME);
-    //     $ret = $firestoreSessionHandler->write('sessionid', 'sessiondata');
-
-    //     $this->assertFalse($ret);
-    // }
-
-    // public function testDestroy()
-    // {
-    //     $id = self::SESSION_NAME . ':sessionid';
-    //     $docRef = $this->prophesize(DocumentReference::class);
-    //     $docRef->delete()
-    //         ->shouldBeCalledTimes(1);
-    //     $this->firestore->collection(self::SESSION_SAVE_PATH)
-    //         ->shouldBeCalledTimes(1)
-    //         ->willReturn($this->collection->reveal());
-    //     $this->collection->document($id)
-    //         ->shouldBeCalledTimes(1)
-    //         ->willReturn($docRef);
-    //     $firestoreSessionHandler = new FirestoreSessionHandler(
-    //         $this->firestore->reveal()
-    //     );
-    //     $firestoreSessionHandler->open(self::SESSION_SAVE_PATH, self::SESSION_NAME);
-    //     $ret = $firestoreSessionHandler->destroy('sessionid');
-
-    //     $this->assertTrue($ret);
-    // }
-
-    // /**
-    //  * @expectedException PHPUnit_Framework_Error_Warning
-    //  */
-    // public function testDestroyWithException()
-    // {
-    //     $id = self::SESSION_NAME . ':sessionid';
-    //     $docRef = $this->prophesize(DocumentReference::class);
-    //     $docRef->delete()
-    //         ->shouldBeCalledTimes(1)
-    //         ->willThrow(new ServiceException(''));
-    //     $this->firestore->collection(self::SESSION_SAVE_PATH)
-    //         ->shouldBeCalledTimes(1)
-    //         ->willReturn($this->collection->reveal());
-    //     $this->collection->document($id)
-    //         ->shouldBeCalledTimes(1)
-    //         ->willReturn($docRef);
-    //     $firestoreSessionHandler = new FirestoreSessionHandler(
-    //         $this->firestore->reveal()
-    //     );
-    //     $firestoreSessionHandler->open(self::SESSION_SAVE_PATH, self::SESSION_NAME);
-    //     $ret = $firestoreSessionHandler->destroy('sessionid');
-
-    //     $this->assertFalse($ret);
-    // }
-
-    // public function testDefaultGcDoesNothing()
-    // {
-    //     $this->firestore->collection(self::SESSION_SAVE_PATH)
-    //         ->shouldBeCalledTimes(1)
-    //         ->willReturn($this->collection->reveal());
-    //     $this->collection->limit()->shouldNotBeCalled();
-    //     $firestoreSessionHandler = new FirestoreSessionHandler(
-    //         $this->firestore->reveal()
-    //     );
-    //     $firestoreSessionHandler->open(self::SESSION_SAVE_PATH, self::SESSION_NAME);
-    //     $ret = $firestoreSessionHandler->gc(100);
-
-    //     $this->assertTrue($ret);
-    // }
-
-    // public function testGc()
-    // {
-    //     $docRef1 = $this->prophesize(DocumentReference::class);
-    //     $docRef1->delete()
-    //         ->shouldBeCalledTimes(1);
-    //     $snapshot1 = $this->prophesize(DocumentSnapshot::class);
-    //     $snapshot1->reference()
-    //         ->shouldBeCalledTimes(1)
-    //         ->willReturn($docRef1);
-    //     $docRef2 = $this->prophesize(DocumentReference::class);
-    //     $docRef2->delete()
-    //         ->shouldBeCalledTimes(1);
-    //     $snapshot2 = $this->prophesize(DocumentSnapshot::class);
-    //     $snapshot2->reference()
-    //         ->shouldBeCalledTimes(1)
-    //         ->willReturn($docRef2);
-    //     $phpunit = $this;
-    //     $collection = $this->collection;
-    //     $collection->where(
-    //         Argument::type('string'),
-    //         Argument::type('string'),
-    //         Argument::type('int')
-    //     )
-    //         ->shouldBeCalledTimes(1)
-    //         ->will(function ($args) use ($phpunit, $collection) {
-    //             $phpunit->assertEquals('t', $args[0]);
-    //             $phpunit->assertEquals('<', $args[1]);
-    //             $phpunit->assertInternalType('int', $args[2]);
-    //             $diff = time() - $args[2];
-    //             // 2 seconds grace period should be enough
-    //             $phpunit->assertLessThanOrEqual(102, $diff);
-    //             $phpunit->assertGreaterThanOrEqual(100, $diff);
-    //             return $collection->reveal();
-    //         });
-
-    //     $collection->orderBy('t')
-    //         ->shouldBeCalledTimes(1)
-    //         ->willReturn($collection->reveal());
-    //     $collection->limit(1000)
-    //         ->shouldBeCalledTimes(1)
-    //         ->willReturn($collection->reveal());
-    //     $collection->documents()
-    //         ->shouldBeCalledTimes(1)
-    //         ->willReturn([$snapshot1, $snapshot2]);
-    //     $this->firestore->collection(self::SESSION_SAVE_PATH)
-    //         ->shouldBeCalledTimes(1)
-    //         ->willReturn($collection->reveal());
-
-    //     $firestoreSessionHandler = new FirestoreSessionHandler(
-    //         $this->firestore->reveal(),
-    //         1000
-    //     );
-
-    //     $firestoreSessionHandler->open(self::SESSION_SAVE_PATH, self::SESSION_NAME);
-    //     $ret = $firestoreSessionHandler->gc(100);
-
-    //     $this->assertTrue($ret);
-    // }
-
-    // /**
-    //  * @expectedException PHPUnit_Framework_Error_Warning
-    //  */
-    // public function testGcWithException()
-    // {
-    //     $docRef = $this->prophesize(DocumentReference::class);
-    //     $docRef->delete()
-    //         ->shouldBeCalledTimes(1)
-    //         ->willThrow(new ServiceException(''));
-    //     $snapshot = $this->prophesize(DocumentSnapshot::class);
-    //     $snapshot->reference()
-    //         ->shouldBeCalledTimes(1)
-    //         ->willReturn($docRef);
-    //     $phpunit = $this;
-    //     $collection = $this->collection;
-    //     $collection->where(
-    //         Argument::type('string'),
-    //         Argument::type('string'),
-    //         Argument::type('int')
-    //     )
-    //         ->shouldBeCalledTimes(1)
-    //         ->will(function ($args) use ($phpunit, $collection) {
-    //             $phpunit->assertEquals('t', $args[0]);
-    //             $phpunit->assertEquals('<', $args[1]);
-    //             $phpunit->assertInternalType('int', $args[2]);
-    //             $diff = time() - $args[2];
-    //             // 2 seconds grace period should be enough
-    //             $phpunit->assertLessThanOrEqual(102, $diff);
-    //             $phpunit->assertGreaterThanOrEqual(100, $diff);
-    //             return $collection->reveal();
-    //         });
-
-    //     $collection->orderBy('t')
-    //         ->shouldBeCalledTimes(1)
-    //         ->willReturn($collection->reveal());
-    //     $collection->limit(1000)
-    //         ->shouldBeCalledTimes(1)
-    //         ->willReturn($collection->reveal());
-    //     $collection->documents()
-    //         ->shouldBeCalledTimes(1)
-    //         ->willReturn([$snapshot]);
-    //     $this->firestore->collection(self::SESSION_SAVE_PATH)
-    //         ->shouldBeCalledTimes(1)
-    //         ->willReturn($collection->reveal());
-    //     $firestoreSessionHandler = new FirestoreSessionHandler(
-    //         $this->firestore->reveal(),
-    //         1000
-    //     );
-
-    //     $firestoreSessionHandler->open(self::SESSION_SAVE_PATH, self::SESSION_NAME);
-    //     $ret = $firestoreSessionHandler->gc(100);
-
-    //     $this->assertFalse($ret);
-    // }
+    public function testWrite()
+    {
+        $phpunit = $this;
+        $this->valueMapper->encodeValues(Argument::type('array'))
+            ->will(function ($args) use ($phpunit) {
+                $phpunit->assertEquals('sessiondata', $args[0]['data']);
+                $phpunit->assertTrue(is_int($args[0]['t']));
+                return ['data' => ['stringValue' => 'sessiondata']];
+            });
+        $this->connection->beginTransaction(['database' => $this->dbName()])
+            ->shouldBeCalledTimes(1);
+        $this->connection->commit([
+            'database' => $this->dbName(),
+            'writes' => [
+                [
+                    'update' => [
+                        'name' => $this->documentName(),
+                        'fields' => [
+                            'data' => ['stringValue' => 'sessiondata']
+                        ]
+                    ]
+                ]
+            ]
+        ])
+            ->shouldBeCalledTimes(1);
+        $firestoreSessionHandler = new FirestoreSessionHandler(
+            $this->connection->reveal(),
+            $this->valueMapper->reveal(),
+            self::PROJECT,
+            self::DATABASE
+        );
+        $firestoreSessionHandler->open(self::SESSION_SAVE_PATH, self::SESSION_NAME);
+        $ret = $firestoreSessionHandler->write('sessionid', 'sessiondata');
+
+        $this->assertTrue($ret);
+    }
+
+    /**
+     * @expectedException PHPUnit_Framework_Error_Warning
+     */
+    public function testWriteWithException()
+    {
+        $phpunit = $this;
+        $this->valueMapper->encodeValues(Argument::type('array'))
+            ->will(function ($args) use ($phpunit) {
+                $phpunit->assertEquals('sessiondata', $args[0]['data']);
+                $phpunit->assertTrue(is_int($args[0]['t']));
+                return ['data' => ['stringValue' => 'sessiondata']];
+            });
+        $this->connection->beginTransaction(['database' => $this->dbName()])
+            ->shouldBeCalledTimes(1)
+            ->willReturn(['transaction' => 123]);
+        $this->connection->rollback([
+            'database' => $this->dbName(),
+            'transaction' => 123
+        ])
+            ->shouldBeCalledTimes(1);
+        $this->connection->commit(Argument::any())
+            ->shouldBeCalledTimes(1)
+            ->willThrow((new ServiceException('')));
+        $firestoreSessionHandler = new FirestoreSessionHandler(
+            $this->connection->reveal(),
+            $this->valueMapper->reveal(),
+            self::PROJECT,
+            self::DATABASE
+        );
+        $firestoreSessionHandler->open(self::SESSION_SAVE_PATH, self::SESSION_NAME);
+        $ret = $firestoreSessionHandler->write('sessionid', 'sessiondata');
+
+        $this->assertFalse($ret);
+    }
+
+    public function testDestroy()
+    {
+        $this->connection->beginTransaction(['database' => $this->dbName()])
+            ->shouldBeCalledTimes(1)
+            ->willReturn(['transaction' => 123]);
+        $this->connection->commit([
+            'database' => $this->dbName(),
+            'writes' => [
+                [
+                    'delete' => $this->documentName()
+                ]
+            ],
+            'transaction' => 123
+        ])
+            ->shouldBeCalledTimes(1);
+        $firestoreSessionHandler = new FirestoreSessionHandler(
+            $this->connection->reveal(),
+            $this->valueMapper->reveal(),
+            self::PROJECT,
+            self::DATABASE
+        );
+        $firestoreSessionHandler->open(self::SESSION_SAVE_PATH, self::SESSION_NAME);
+        $ret = $firestoreSessionHandler->destroy('sessionid');
+
+        $this->assertTrue($ret);
+    }
+
+    /**
+     * @expectedException PHPUnit_Framework_Error_Warning
+     */
+    public function testDestroyWithException()
+    {
+        $this->connection->beginTransaction(['database' => $this->dbName()])
+            ->shouldBeCalledTimes(1)
+            ->willReturn(['transaction' => 123]);
+        $this->connection->commit(Argument::any())
+            ->shouldBeCalledTimes(1)
+            ->willThrow(new ServiceException(''));
+        $this->connection->rollback([
+            'database' => $this->dbName(),
+            'transaction' => 123
+        ])
+            ->shouldBeCalledTimes(1);
+        $firestoreSessionHandler = new FirestoreSessionHandler(
+            $this->connection->reveal(),
+            $this->valueMapper->reveal(),
+            self::PROJECT,
+            self::DATABASE
+        );
+        $firestoreSessionHandler->open(self::SESSION_SAVE_PATH, self::SESSION_NAME);
+        $ret = $firestoreSessionHandler->destroy('sessionid');
+
+        $this->assertFalse($ret);
+    }
+
+    public function testDefaultGcDoesNothing()
+    {
+        $this->connection->beginTransaction(['database' => $this->dbName()])
+            ->shouldBeCalledTimes(1)
+            ->willReturn(['transaction' => 123]);
+        $this->connection->commit()->shouldNotBeCalled();
+        $firestoreSessionHandler = new FirestoreSessionHandler(
+            $this->connection->reveal(),
+            $this->valueMapper->reveal(),
+            self::PROJECT,
+            self::DATABASE
+        );
+        $firestoreSessionHandler->open(self::SESSION_SAVE_PATH, self::SESSION_NAME);
+        $ret = $firestoreSessionHandler->gc(100);
+
+        $this->assertTrue($ret);
+    }
+
+    public function testGc()
+    {
+        $phpunit = $this;
+        $this->documents->valid()
+            ->shouldBeCalledTimes(2)
+            ->willReturn(true, false);
+        $this->documents->current()
+            ->shouldBeCalledTimes(1)
+            ->willReturn([
+                'document' => [
+                    'name' => $this->documentName(),
+                    'fields' => [],
+                    'createTime' => date('Y-m-d'),
+                    'updateTime' => date('Y-m-d'),
+                ],
+                'readTime' => date('Y-m-d'),
+            ]);
+        $this->documents->next()
+            ->shouldBeCalledTimes(1);
+        $this->connection->beginTransaction(['database' => $this->dbName()])
+            ->shouldBeCalledTimes(1)
+            ->willReturn(['transaction' => 123]);
+        $this->connection->runQuery(Argument::any())
+            ->shouldBeCalledTimes(1)
+            ->will(function ($args) use ($phpunit) {
+                $options = $args[0];
+                $phpunit->assertEquals(
+                    $phpunit->dbName() . '/documents',
+                    $options['parent']
+                );
+                $phpunit->assertEquals(999, $options['structuredQuery']['limit']);
+                $phpunit->assertEquals(
+                    self::SESSION_SAVE_PATH . ':' . self::SESSION_NAME,
+                    $options['structuredQuery']['from'][0]['collectionId']
+                );
+                $phpunit->assertEquals(123, $options['transaction']);
+                return $phpunit->documents->reveal();
+            });
+        $this->valueMapper->decodeValues([])
+            ->shouldBeCalledTimes(1)
+            ->willReturn(['data' => 'sessiondata']);
+        $this->valueMapper->encodeValue(Argument::type('integer'))
+            ->shouldBeCalledTimes(1);
+        $this->connection->commit([
+            'database' => $this->dbName(),
+            'writes' => [
+                [
+                    'delete' => $this->documentName()
+                ]
+            ],
+            'transaction' => 123
+        ])
+            ->shouldBeCalledTimes(1);
+        $firestoreSessionHandler = new FirestoreSessionHandler(
+            $this->connection->reveal(),
+            $this->valueMapper->reveal(),
+            self::PROJECT,
+            self::DATABASE,
+            ['gcLimit' => 999, 'query' => ['maxRetries' => 0]]
+        );
+
+        $firestoreSessionHandler->open(self::SESSION_SAVE_PATH, self::SESSION_NAME);
+        $ret = $firestoreSessionHandler->gc(100);
+
+        $this->assertTrue($ret);
+    }
+
+    /**
+     * @expectedException PHPUnit_Framework_Error_Warning
+     */
+    public function testGcWithException()
+    {
+        $this->connection->beginTransaction(['database' => $this->dbName()])
+            ->shouldBeCalledTimes(1)
+            ->willReturn(['transaction' => 123]);
+        $this->connection->runQuery(Argument::any())
+            ->shouldBeCalledTimes(1)
+            ->willThrow(new ServiceException(''));
+        $firestoreSessionHandler = new FirestoreSessionHandler(
+            $this->connection->reveal(),
+            $this->valueMapper->reveal(),
+            self::PROJECT,
+            self::DATABASE,
+            ['gcLimit' => 1000, 'query' => ['maxRetries' => 0]]
+        );
+
+        $firestoreSessionHandler->open(self::SESSION_SAVE_PATH, self::SESSION_NAME);
+        $ret = $firestoreSessionHandler->gc(100);
+
+        $this->assertFalse($ret);
+    }
+
+    private function dbName()
+    {
+        return sprintf('projects/%s/databases/%s',
+            self::PROJECT,
+            self::DATABASE
+        );
+    }
+
+    private function documentName()
+    {
+        return sprintf('%s/documents/%s:%s/sessionid',
+            $this->dbName(),
+            self::SESSION_SAVE_PATH,
+            self::SESSION_NAME
+        );
+    }
 }

From 62c874c0d50e63be926c55a0709bd6f554f4241a Mon Sep 17 00:00:00 2001
From: Brent Shaffer <betterbrent@google.com>
Date: Fri, 20 Sep 2019 09:57:16 -0700
Subject: [PATCH 12/20] fixes cs

---
 Firestore/tests/System/FirestoreSessionHandlerTest.php | 8 ++++++--
 Firestore/tests/Unit/FirestoreSessionHandlerTest.php   | 6 ++++--
 2 files changed, 10 insertions(+), 4 deletions(-)

diff --git a/Firestore/tests/System/FirestoreSessionHandlerTest.php b/Firestore/tests/System/FirestoreSessionHandlerTest.php
index f389a49a5b07..cd20b144e980 100644
--- a/Firestore/tests/System/FirestoreSessionHandlerTest.php
+++ b/Firestore/tests/System/FirestoreSessionHandlerTest.php
@@ -70,7 +70,9 @@ public function testSessionHandlerGarbageCollection()
         $collection->document('foo2')->set(['data' => 'foo2', 't' => time() - 1]);
 
         $count = 0;
-        foreach ($collection->documents() as $doc) $count++;
+        foreach ($collection->documents() as $doc) {
+            $count++;
+        }
         $this->assertEquals(2, $count);
 
         $handler = $client->sessionHandler([
@@ -81,7 +83,9 @@ public function testSessionHandlerGarbageCollection()
         $handler->gc(0);
 
         $count = 0;
-        foreach ($collection->documents() as $doc) $count++;
+        foreach ($collection->documents() as $doc) {
+            $count++;
+        }
         $this->assertEquals(0, $count);
     }
 }
diff --git a/Firestore/tests/Unit/FirestoreSessionHandlerTest.php b/Firestore/tests/Unit/FirestoreSessionHandlerTest.php
index 098866de2634..8294ef8f047f 100644
--- a/Firestore/tests/Unit/FirestoreSessionHandlerTest.php
+++ b/Firestore/tests/Unit/FirestoreSessionHandlerTest.php
@@ -422,7 +422,8 @@ public function testGcWithException()
 
     private function dbName()
     {
-        return sprintf('projects/%s/databases/%s',
+        return sprintf(
+            'projects/%s/databases/%s',
             self::PROJECT,
             self::DATABASE
         );
@@ -430,7 +431,8 @@ private function dbName()
 
     private function documentName()
     {
-        return sprintf('%s/documents/%s:%s/sessionid',
+        return sprintf(
+            '%s/documents/%s:%s/sessionid',
             $this->dbName(),
             self::SESSION_SAVE_PATH,
             self::SESSION_NAME

From 909c7dd50643db0ccbdbd537b946e165e0df8a37 Mon Sep 17 00:00:00 2001
From: Brent Shaffer <betterbrent@google.com>
Date: Fri, 20 Sep 2019 16:00:24 -0700
Subject: [PATCH 13/20] fixes test snippets

---
 Firestore/src/FirestoreClient.php             | 10 +--
 .../tests/Snippet/FirestoreClientTest.php     |  1 +
 .../Snippet/FirestoreSessionHandlerTest.php   | 62 ++++++++++++++++---
 3 files changed, 62 insertions(+), 11 deletions(-)

diff --git a/Firestore/src/FirestoreClient.php b/Firestore/src/FirestoreClient.php
index 85796da8506f..a5d6d9245e80 100644
--- a/Firestore/src/FirestoreClient.php
+++ b/Firestore/src/FirestoreClient.php
@@ -579,14 +579,16 @@ public function fieldPath(array $fieldNames)
      *
      * Example:
      * ```
-     * use Google\Cloud\Firestore\FirestoreClient;
-     *
-     * $firestore = new FirestoreClient(['projectId' => $projectId]);
-     *
      * $handler = $firestore->sessionHandler();
+     *
+     * // Configure PHP to use the Firestore session handler.
      * session_set_save_handler($handler, true);
      * session_save_path('sessions');
      * session_start();
+     *
+     * // Then write and read the $_SESSION array.
+     * $_SESSION['name'] = 'Bob';
+     * echo $_SESSION['name'];
      * ```
      *
      * @param array $options {
diff --git a/Firestore/tests/Snippet/FirestoreClientTest.php b/Firestore/tests/Snippet/FirestoreClientTest.php
index ac3899fe0a5a..f17361af6cf5 100644
--- a/Firestore/tests/Snippet/FirestoreClientTest.php
+++ b/Firestore/tests/Snippet/FirestoreClientTest.php
@@ -43,6 +43,7 @@ class FirestoreClientTest extends SnippetTestCase
 
     const PROJECT = 'example_project';
     const DATABASE = '(default)';
+    const TRANSACTION = 'transaction-id';
 
     private $connection;
     private $client;
diff --git a/Firestore/tests/Snippet/FirestoreSessionHandlerTest.php b/Firestore/tests/Snippet/FirestoreSessionHandlerTest.php
index dc66d78988f0..c4049392e3e1 100644
--- a/Firestore/tests/Snippet/FirestoreSessionHandlerTest.php
+++ b/Firestore/tests/Snippet/FirestoreSessionHandlerTest.php
@@ -39,19 +39,25 @@ class FirestoreSessionHandlerTest extends SnippetTestCase
     private $connection;
     private $client;
 
-    public function setUp()
+    public static function setUpBeforeClass()
     {
-        $this->checkAndSkipGrpcTests();
-
-        $this->connection = $this->prophesize(ConnectionInterface::class);
-        $this->client = TestHelpers::stub(FirestoreClient::class);
+        parent::setUpBeforeClass();
 
         // Since the tests in this class must run in isolation, they won't be
         // recognized as having been covered, and will cause a CI error.
         // We can call `snippetFromClass` in the parent process to mark the
         // snippets as having been covered.
-        $this->snippetFromClass(FirestoreSessionHandler::class);
-        $this->snippetFromClass(FirestoreSessionHandler::class, 1);
+        self::snippetFromClass(FirestoreSessionHandler::class);
+        self::snippetFromClass(FirestoreSessionHandler::class, 1);
+        self::snippetFromMethod(FirestoreClient::class, 'sessionHandler');
+    }
+
+    public function setUp()
+    {
+        $this->checkAndSkipGrpcTests();
+
+        $this->connection = $this->prophesize(ConnectionInterface::class);
+        $this->client = TestHelpers::stub(FirestoreClient::class);
     }
 
     public function testClass()
@@ -97,6 +103,48 @@ public function testClass()
         $this->assertEquals('Bob', $res->output());
     }
 
+    public function testSessionHandlerMethod()
+    {
+        $snippet = $this->snippetFromMethod(FirestoreClient::class, 'sessionHandler');
+
+        $this->connection->batchGetDocuments(Argument::withEntry('documents', Argument::type('array')))
+            ->shouldBeCalled()
+            ->willReturn(new \ArrayIterator([
+                'found' => [
+                    [
+                        'name' => '',
+                        'fields' => []
+                    ]
+                ]
+            ]));
+
+        $this->connection->beginTransaction(Argument::any())
+            ->shouldBeCalled()
+            ->willReturn([
+                'transaction' => self::TRANSACTION
+            ]);
+
+        $value = 'name|' . serialize('Bob');
+        $this->connection->commit(Argument::allOf(
+            Argument::that(function ($args) use ($value) {
+                return strpos($args['writes'][0]['update']['name'], ':PHPSESSID') !== false
+                    && $args['writes'][0]['update']['fields']['data']['stringValue'] === $value
+                    && isset($args['writes'][0]['update']['fields']['t']['integerValue']);
+            }),
+            Argument::withEntry('transaction', self::TRANSACTION)
+        ))->shouldBeCalled()->willReturn([
+            'writeResults' => []
+        ]);
+
+        $this->client->___setProperty('connection', $this->connection->reveal());
+        $snippet->addLocal('firestore', $this->client);
+
+        $res = $snippet->invoke();
+        session_write_close();
+
+        $this->assertEquals('Bob', $res->output());
+    }
+
     /**
      * @expectedException \RuntimeException
      */

From 5ceb459e604d6fe41be7c32c742c49090f71f242 Mon Sep 17 00:00:00 2001
From: Brent Shaffer <betterbrent@google.com>
Date: Fri, 20 Sep 2019 18:33:30 -0700
Subject: [PATCH 14/20] removes unnecessary changes and adds test for coverage

---
 Firestore/src/FirestoreClient.php               | 2 +-
 Firestore/src/Query.php                         | 8 ++++----
 Firestore/tests/Snippet/FirestoreClientTest.php | 1 -
 Firestore/tests/Unit/FirestoreClientTest.php    | 7 +++++++
 4 files changed, 12 insertions(+), 6 deletions(-)

diff --git a/Firestore/src/FirestoreClient.php b/Firestore/src/FirestoreClient.php
index a5d6d9245e80..62ab658023ec 100644
--- a/Firestore/src/FirestoreClient.php
+++ b/Firestore/src/FirestoreClient.php
@@ -443,7 +443,7 @@ public function runTransaction(callable $callable, array $options = [])
             'maxRetries' => self::MAX_RETRIES,
             'begin' => [],
             'commit' => [],
-            'rollback' => [],
+            'rollback' => []
         ];
 
         $retryableErrors = [
diff --git a/Firestore/src/Query.php b/Firestore/src/Query.php
index b77623de799c..b7a4a1115e54 100644
--- a/Firestore/src/Query.php
+++ b/Firestore/src/Query.php
@@ -169,13 +169,12 @@ public function documents(array $options = [])
 
         $rows = (new ExponentialBackoff($maxRetries))->execute(function () use ($options) {
             $query = $this->finalQueryPrepare($this->query);
-            $options2 = $this->arrayFilterRemoveNull([
+
+            $generator = $this->connection->runQuery($this->arrayFilterRemoveNull([
                 'parent' => $this->parent,
                 'structuredQuery' => $query,
                 'retries' => 0
-            ]) + $options;
-
-            $generator = $this->connection->runQuery($options2);
+            ]) + $options);
 
             // cache collection references
             $collections = [];
@@ -209,6 +208,7 @@ public function documents(array $options = [])
 
                 $generator->next();
             }
+
             return $out;
         });
 
diff --git a/Firestore/tests/Snippet/FirestoreClientTest.php b/Firestore/tests/Snippet/FirestoreClientTest.php
index f17361af6cf5..ac3899fe0a5a 100644
--- a/Firestore/tests/Snippet/FirestoreClientTest.php
+++ b/Firestore/tests/Snippet/FirestoreClientTest.php
@@ -43,7 +43,6 @@ class FirestoreClientTest extends SnippetTestCase
 
     const PROJECT = 'example_project';
     const DATABASE = '(default)';
-    const TRANSACTION = 'transaction-id';
 
     private $connection;
     private $client;
diff --git a/Firestore/tests/Unit/FirestoreClientTest.php b/Firestore/tests/Unit/FirestoreClientTest.php
index 45b8b1a7ecfb..2295d2451e37 100644
--- a/Firestore/tests/Unit/FirestoreClientTest.php
+++ b/Firestore/tests/Unit/FirestoreClientTest.php
@@ -29,6 +29,7 @@
 use Google\Cloud\Firestore\DocumentReference;
 use Google\Cloud\Firestore\FieldPath;
 use Google\Cloud\Firestore\FirestoreClient;
+use Google\Cloud\Firestore\FirestoreSessionHandler;
 use Google\Cloud\Firestore\Query;
 use Google\Cloud\Firestore\WriteBatch;
 use PHPUnit\Framework\TestCase;
@@ -555,6 +556,12 @@ public function testFieldPath()
         $this->assertEquals($parts, $path->path());
     }
 
+    public function testSessionHandler()
+    {
+        $sessionHandler = $this->client->sessionHandler();
+        $this->assertInstanceOf(FirestoreSessionHandler::class, $sessionHandler);
+    }
+
     private function noop()
     {
         return function () {

From 9065fc2f8b3453222b6b473c18c56ffb3a2bc16a Mon Sep 17 00:00:00 2001
From: Brent Shaffer <betterbrent@google.com>
Date: Mon, 23 Sep 2019 12:38:02 -0700
Subject: [PATCH 15/20] adds options comment, adds collectionNameTemplate
 option

---
 Firestore/src/FirestoreClient.php         |  9 +++++++++
 Firestore/src/FirestoreSessionHandler.php | 13 +++++++++----
 2 files changed, 18 insertions(+), 4 deletions(-)

diff --git a/Firestore/src/FirestoreClient.php b/Firestore/src/FirestoreClient.php
index 62ab658023ec..e0032868ff15 100644
--- a/Firestore/src/FirestoreClient.php
+++ b/Firestore/src/FirestoreClient.php
@@ -598,6 +598,15 @@ public function fieldPath(array $fieldNames)
      *        garbage collection. Values larger than 1000 will be limited to
      *        1000. **Defaults to** `0`, indicating garbage collection is
      *        disabled by default.
+     *     @type string $collectionNameTemplate [optional] A sprintf compatible
+     *        template for formatting the collection name where sessions will be
+     *        stored. The template receives two values, the first being the save
+     *        path and the latter being the session name.
+     *     @type array $begin [optional] Configuration options for beginTransaction.
+     *     @type array $commit [optional] Configuration options for commit.
+     *     @type array $rollback [optional] Configuration options for rollback.
+     *     @type array $delete [optional] Configuration options for delete.
+     *     @type array $query [optional] Configuration options for runQuery.
      * @return FirestoreSessionHandler
      */
     public function sessionHandler(array $options = [])
diff --git a/Firestore/src/FirestoreSessionHandler.php b/Firestore/src/FirestoreSessionHandler.php
index 31fa2df01f34..1d8b556dac1d 100644
--- a/Firestore/src/FirestoreSessionHandler.php
+++ b/Firestore/src/FirestoreSessionHandler.php
@@ -174,6 +174,7 @@ public function __construct(
             'delete' => [],
             'query' => [],
             'gcLimit' => 0,
+            'collectionNameTemplate' => '%1$s:%2$s',
         ];
 
         // Cut down gcLimit to 1000
@@ -374,19 +375,23 @@ private function commitTransaction()
     }
 
     /**
-     * Format the Firebase document ID from the PHP session ID and session name.
-     * ex: sessions:PHPSESSID/abcdef
+     * Format the Firebase collection ID from the PHP session ID and session name.
+     * ex: sessions:PHPSESSID
      *
      * @param string $id Identifier used for the session
      * @return string
      */
     private function collectionId()
     {
-        return sprintf('%s:%s', $this->savePath, $this->sessionName);
+        return sprintf(
+            $this->options['collectionNameTemplate'],
+            $this->savePath,
+            $this->sessionName
+        );
     }
 
     /**
-     * Format the Firebase document ID from the PHP session ID and session name.
+     * Format the Firebase document ID from the collection ID.
      * ex: sessions:PHPSESSID/abcdef
      *
      * @param string $id Identifier used for the session

From e730412bd500dfa16b0e60a0259371e8cbf94134 Mon Sep 17 00:00:00 2001
From: Brent Shaffer <betterbrent@google.com>
Date: Mon, 23 Sep 2019 12:40:35 -0700
Subject: [PATCH 16/20] removes redundant optional identifier

---
 Firestore/src/FirestoreClient.php | 28 ++++++++++++++--------------
 1 file changed, 14 insertions(+), 14 deletions(-)

diff --git a/Firestore/src/FirestoreClient.php b/Firestore/src/FirestoreClient.php
index e0032868ff15..342d2332ddc2 100644
--- a/Firestore/src/FirestoreClient.php
+++ b/Firestore/src/FirestoreClient.php
@@ -591,22 +591,22 @@ public function fieldPath(array $fieldNames)
      * echo $_SESSION['name'];
      * ```
      *
-     * @param array $options {
+     * @param array $options [optional] {
      *     Configuration Options.
      *
-     *     @type int $gcLimit [optional] The number of entities to delete in the
-     *        garbage collection. Values larger than 1000 will be limited to
-     *        1000. **Defaults to** `0`, indicating garbage collection is
-     *        disabled by default.
-     *     @type string $collectionNameTemplate [optional] A sprintf compatible
-     *        template for formatting the collection name where sessions will be
-     *        stored. The template receives two values, the first being the save
-     *        path and the latter being the session name.
-     *     @type array $begin [optional] Configuration options for beginTransaction.
-     *     @type array $commit [optional] Configuration options for commit.
-     *     @type array $rollback [optional] Configuration options for rollback.
-     *     @type array $delete [optional] Configuration options for delete.
-     *     @type array $query [optional] Configuration options for runQuery.
+     *     @type int $gcLimit The number of entities to delete in the garbage
+     *        collection. Values larger than 1000 will be limited to 1000.
+     *        **Defaults to** `0`, indicating garbage collection is disabled by
+     *        default.
+     *     @type string $collectionNameTemplate A sprintf compatible template
+     *        for formatting the collection name where sessions will be stored.
+     *        The template receives two values, the first being the save path
+     *        and the latter being the session name.
+     *     @type array $begin Configuration options for beginTransaction.
+     *     @type array $commit Configuration options for commit.
+     *     @type array $rollback Configuration options for rollback.
+     *     @type array $delete Configuration options for delete.
+     *     @type array $query Configuration options for runQuery.
      * @return FirestoreSessionHandler
      */
     public function sessionHandler(array $options = [])

From 36de91a5fe5784a0835044925be530028c06b77f Mon Sep 17 00:00:00 2001
From: Brent Shaffer <betterbrent@google.com>
Date: Mon, 23 Sep 2019 22:21:43 -0700
Subject: [PATCH 17/20] addresses review comments

---
 Firestore/src/FirestoreClient.php         |  3 +-
 Firestore/src/FirestoreSessionHandler.php | 45 ++++++++++++++++-------
 2 files changed, 33 insertions(+), 15 deletions(-)

diff --git a/Firestore/src/FirestoreClient.php b/Firestore/src/FirestoreClient.php
index 342d2332ddc2..77d1c85fef40 100644
--- a/Firestore/src/FirestoreClient.php
+++ b/Firestore/src/FirestoreClient.php
@@ -605,8 +605,9 @@ public function fieldPath(array $fieldNames)
      *     @type array $begin Configuration options for beginTransaction.
      *     @type array $commit Configuration options for commit.
      *     @type array $rollback Configuration options for rollback.
-     *     @type array $delete Configuration options for delete.
+     *     @type array $read Configuration options for read.
      *     @type array $query Configuration options for runQuery.
+     * }
      * @return FirestoreSessionHandler
      */
     public function sessionHandler(array $options = [])
diff --git a/Firestore/src/FirestoreSessionHandler.php b/Firestore/src/FirestoreSessionHandler.php
index 1d8b556dac1d..cd15f0e22d95 100644
--- a/Firestore/src/FirestoreSessionHandler.php
+++ b/Firestore/src/FirestoreSessionHandler.php
@@ -46,12 +46,13 @@
  * possible data contentions. Also please see the 2nd example below for how to
  * properly handle errors on `write` operations.
  *
- * The handler stores data in a collection provided by the value of
- * session.save_path, isolating the session data from your application data. It
- * creates documents in the specified collection where the session name and ID
- * are concatenated. By default, it does nothing on gc for reducing the cost.
- * Pass a positive value up to 1000 for $gcLimit parameter to delete entities in
- * gc.
+ * The handler stores data in a collection formatted from the values of
+ * session.save_path and session.name, which can be customized using the
+ * $collectionNameTemplate option. This isolates the session data from your
+ * application data. It creates documents in the specified collection where the
+ * ID is the session ID. By default, it does nothing on gc for reducing the
+ * cost. Pass a positive value up to 1000 for $gcLimit option to delete entities
+ * in gc.
  *
  * The first example automatically writes the session data. It's handy, but
  * the code doesn't stop even if it fails to write the session data, because
@@ -154,7 +155,23 @@ class FirestoreSessionHandler implements SessionHandlerInterface
      * @param ValueMapper $valueMapper A Firestore Value Mapper.
      * @param string $projectId The current project id.
      * @param string $database The database id.
-     * @param array $options [optional]
+     * @param array $options [optional] {
+     *     Configuration Options.
+     *
+     *     @type int $gcLimit The number of entities to delete in the garbage
+     *        collection. Values larger than 1000 will be limited to 1000.
+     *        **Defaults to** `0`, indicating garbage collection is disabled by
+     *        default.
+     *     @type string $collectionNameTemplate A sprintf compatible template
+     *        for formatting the collection name where sessions will be stored.
+     *        The template receives two values, the first being the save path
+     *        and the latter being the session name.
+     *     @type array $begin Configuration options for beginTransaction.
+     *     @type array $commit Configuration options for commit.
+     *     @type array $rollback Configuration options for rollback.
+     *     @type array $read Configuration options for read.
+     *     @type array $query Configuration options for runQuery.
+     * }
      */
     public function __construct(
         ConnectionInterface $connection,
@@ -171,8 +188,8 @@ public function __construct(
             'begin' => [],
             'commit' => [],
             'rollback' => [],
-            'delete' => [],
             'query' => [],
+            'read' => [],
             'gcLimit' => 0,
             'collectionNameTemplate' => '%1$s:%2$s',
         ];
@@ -234,7 +251,10 @@ public function read($id)
                 $this->database,
                 $this->docId($id)
             );
-            $snapshot = $this->transaction->snapshot($docRef);
+            $snapshot = $this->transaction->snapshot(
+                $docRef,
+                $this->options['read']
+            );
             if ($snapshot->exists() && isset($snapshot['data'])) {
                 return $snapshot->get('data');
             }
@@ -295,7 +315,7 @@ public function destroy($id)
                 $this->database,
                 $this->docId($id)
             );
-            $this->transaction->delete($docRef, $this->options['delete']);
+            $this->transaction->delete($docRef);
             $this->commitTransaction();
         } catch (ServiceException $e) {
             trigger_error(
@@ -336,10 +356,7 @@ public function gc($maxlifetime)
                 $this->options['query']
             );
             foreach ($querySnapshot as $snapshot) {
-                $this->transaction->delete(
-                    $snapshot->reference(),
-                    $this->options['delete']
-                );
+                $this->transaction->delete($snapshot->reference());
             }
             $this->commitTransaction();
         } catch (ServiceException $e) {

From d9190ba1903d72915d75faee929bbb87654ff369 Mon Sep 17 00:00:00 2001
From: Brent Shaffer <betterbrent@google.com>
Date: Tue, 24 Sep 2019 08:32:17 -0700
Subject: [PATCH 18/20] addresses more review comments

---
 .../System/FirestoreSessionHandlerTest.php    | 12 ++-----
 .../Unit/FirestoreSessionHandlerTest.php      | 31 +++++++++----------
 2 files changed, 16 insertions(+), 27 deletions(-)

diff --git a/Firestore/tests/System/FirestoreSessionHandlerTest.php b/Firestore/tests/System/FirestoreSessionHandlerTest.php
index cd20b144e980..07b19f3e5aea 100644
--- a/Firestore/tests/System/FirestoreSessionHandlerTest.php
+++ b/Firestore/tests/System/FirestoreSessionHandlerTest.php
@@ -69,11 +69,7 @@ public function testSessionHandlerGarbageCollection()
         $collection->document('foo1')->set(['data' => 'foo1', 't' => time() - 1]);
         $collection->document('foo2')->set(['data' => 'foo2', 't' => time() - 1]);
 
-        $count = 0;
-        foreach ($collection->documents() as $doc) {
-            $count++;
-        }
-        $this->assertEquals(2, $count);
+        $this->assertCount(2, $collection->documents());
 
         $handler = $client->sessionHandler([
             'gcLimit' => 1000,
@@ -82,10 +78,6 @@ public function testSessionHandlerGarbageCollection()
         $handler->open($namespace, $sessionName);
         $handler->gc(0);
 
-        $count = 0;
-        foreach ($collection->documents() as $doc) {
-            $count++;
-        }
-        $this->assertEquals(0, $count);
+        $this->assertCount(0, $collection->documents());
     }
 }
diff --git a/Firestore/tests/Unit/FirestoreSessionHandlerTest.php b/Firestore/tests/Unit/FirestoreSessionHandlerTest.php
index 8294ef8f047f..47e5afd046e8 100644
--- a/Firestore/tests/Unit/FirestoreSessionHandlerTest.php
+++ b/Firestore/tests/Unit/FirestoreSessionHandlerTest.php
@@ -38,7 +38,7 @@
 class FirestoreSessionHandlerTest extends TestCase
 {
     const SESSION_SAVE_PATH = 'sessions';
-    const SESSION_NAME = 'PHPSESID';
+    const SESSION_NAME = 'PHPSESSID';
     const PROJECT = 'example_project';
     const DATABASE = '(default)';
 
@@ -185,11 +185,10 @@ public function testReadEntity()
 
     public function testWrite()
     {
-        $phpunit = $this;
         $this->valueMapper->encodeValues(Argument::type('array'))
-            ->will(function ($args) use ($phpunit) {
-                $phpunit->assertEquals('sessiondata', $args[0]['data']);
-                $phpunit->assertTrue(is_int($args[0]['t']));
+            ->will(function ($args) {
+                $this->assertEquals('sessiondata', $args[0]['data']);
+                $this->assertTrue(is_int($args[0]['t']));
                 return ['data' => ['stringValue' => 'sessiondata']];
             });
         $this->connection->beginTransaction(['database' => $this->dbName()])
@@ -225,11 +224,10 @@ public function testWrite()
      */
     public function testWriteWithException()
     {
-        $phpunit = $this;
         $this->valueMapper->encodeValues(Argument::type('array'))
-            ->will(function ($args) use ($phpunit) {
-                $phpunit->assertEquals('sessiondata', $args[0]['data']);
-                $phpunit->assertTrue(is_int($args[0]['t']));
+            ->will(function ($args) {
+                $this->assertEquals('sessiondata', $args[0]['data']);
+                $this->assertTrue(is_int($args[0]['t']));
                 return ['data' => ['stringValue' => 'sessiondata']];
             });
         $this->connection->beginTransaction(['database' => $this->dbName()])
@@ -330,7 +328,6 @@ public function testDefaultGcDoesNothing()
 
     public function testGc()
     {
-        $phpunit = $this;
         $this->documents->valid()
             ->shouldBeCalledTimes(2)
             ->willReturn(true, false);
@@ -352,19 +349,19 @@ public function testGc()
             ->willReturn(['transaction' => 123]);
         $this->connection->runQuery(Argument::any())
             ->shouldBeCalledTimes(1)
-            ->will(function ($args) use ($phpunit) {
+            ->will(function ($args) {
                 $options = $args[0];
-                $phpunit->assertEquals(
-                    $phpunit->dbName() . '/documents',
+                $this->assertEquals(
+                    $this->dbName() . '/documents',
                     $options['parent']
                 );
-                $phpunit->assertEquals(999, $options['structuredQuery']['limit']);
-                $phpunit->assertEquals(
+                $this->assertEquals(999, $options['structuredQuery']['limit']);
+                $this->assertEquals(
                     self::SESSION_SAVE_PATH . ':' . self::SESSION_NAME,
                     $options['structuredQuery']['from'][0]['collectionId']
                 );
-                $phpunit->assertEquals(123, $options['transaction']);
-                return $phpunit->documents->reveal();
+                $this->assertEquals(123, $options['transaction']);
+                return $this->documents->reveal();
             });
         $this->valueMapper->decodeValues([])
             ->shouldBeCalledTimes(1)

From b5cff2dc4f7834ea99a9626df45b4ac5056c7952 Mon Sep 17 00:00:00 2001
From: Brent Shaffer <betterbrent@google.com>
Date: Tue, 24 Sep 2019 08:46:35 -0700
Subject: [PATCH 19/20] adds back  variable to callables

---
 .../Unit/FirestoreSessionHandlerTest.php      | 29 ++++++++++---------
 1 file changed, 16 insertions(+), 13 deletions(-)

diff --git a/Firestore/tests/Unit/FirestoreSessionHandlerTest.php b/Firestore/tests/Unit/FirestoreSessionHandlerTest.php
index 47e5afd046e8..879d28fa48c4 100644
--- a/Firestore/tests/Unit/FirestoreSessionHandlerTest.php
+++ b/Firestore/tests/Unit/FirestoreSessionHandlerTest.php
@@ -185,10 +185,11 @@ public function testReadEntity()
 
     public function testWrite()
     {
+        $phpunit = $this;
         $this->valueMapper->encodeValues(Argument::type('array'))
-            ->will(function ($args) {
-                $this->assertEquals('sessiondata', $args[0]['data']);
-                $this->assertTrue(is_int($args[0]['t']));
+            ->will(function ($args) use ($phpunit) {
+                $phpunit->assertEquals('sessiondata', $args[0]['data']);
+                $phpunit->assertTrue(is_int($args[0]['t']));
                 return ['data' => ['stringValue' => 'sessiondata']];
             });
         $this->connection->beginTransaction(['database' => $this->dbName()])
@@ -224,10 +225,11 @@ public function testWrite()
      */
     public function testWriteWithException()
     {
+        $phpunit = $this;
         $this->valueMapper->encodeValues(Argument::type('array'))
-            ->will(function ($args) {
-                $this->assertEquals('sessiondata', $args[0]['data']);
-                $this->assertTrue(is_int($args[0]['t']));
+            ->will(function ($args) use ($phpunit) {
+                $phpunit->assertEquals('sessiondata', $args[0]['data']);
+                $phpunit->assertTrue(is_int($args[0]['t']));
                 return ['data' => ['stringValue' => 'sessiondata']];
             });
         $this->connection->beginTransaction(['database' => $this->dbName()])
@@ -328,6 +330,7 @@ public function testDefaultGcDoesNothing()
 
     public function testGc()
     {
+        $phpunit = $this;
         $this->documents->valid()
             ->shouldBeCalledTimes(2)
             ->willReturn(true, false);
@@ -349,19 +352,19 @@ public function testGc()
             ->willReturn(['transaction' => 123]);
         $this->connection->runQuery(Argument::any())
             ->shouldBeCalledTimes(1)
-            ->will(function ($args) {
+            ->will(function ($args) use ($phpunit) {
                 $options = $args[0];
-                $this->assertEquals(
-                    $this->dbName() . '/documents',
+                $phpunit->assertEquals(
+                    $phpunit->dbName() . '/documents',
                     $options['parent']
                 );
-                $this->assertEquals(999, $options['structuredQuery']['limit']);
-                $this->assertEquals(
+                $phpunit->assertEquals(999, $options['structuredQuery']['limit']);
+                $phpunit->assertEquals(
                     self::SESSION_SAVE_PATH . ':' . self::SESSION_NAME,
                     $options['structuredQuery']['from'][0]['collectionId']
                 );
-                $this->assertEquals(123, $options['transaction']);
-                return $this->documents->reveal();
+                $phpunit->assertEquals(123, $options['transaction']);
+                return $phpunit->documents->reveal();
             });
         $this->valueMapper->decodeValues([])
             ->shouldBeCalledTimes(1)

From da89a0a0ca184d33e9d440acf0ff56721d278e44 Mon Sep 17 00:00:00 2001
From: Brent Shaffer <betterbrent@google.com>
Date: Tue, 24 Sep 2019 12:14:22 -0700
Subject: [PATCH 20/20] adds try/catch around beginTransaction

---
 Firestore/src/FirestoreSessionHandler.php     | 20 +++++++++++++------
 .../Unit/FirestoreSessionHandlerTest.php      | 18 +++++++++++++++++
 2 files changed, 32 insertions(+), 6 deletions(-)

diff --git a/Firestore/src/FirestoreSessionHandler.php b/Firestore/src/FirestoreSessionHandler.php
index cd15f0e22d95..609e53bd51b4 100644
--- a/Firestore/src/FirestoreSessionHandler.php
+++ b/Firestore/src/FirestoreSessionHandler.php
@@ -202,9 +202,9 @@ public function __construct(
      * Start a session, by getting the Firebase collection from $savePath.
      *
      * @param string $savePath The value of `session.save_path` setting will be
-     *        used here as the Firestore collection ID.
+     *        used in the Firestore collection ID.
      * @param string $sessionName The value of `session.name` setting will be
-     *        used here as the Firestore document ID prefix (ex: "PHPSESSID:").
+     *        used in the Firestore collection ID.
      * @return bool
      */
     public function open($savePath, $sessionName)
@@ -213,9 +213,16 @@ public function open($savePath, $sessionName)
         $this->sessionName = $sessionName;
         $database = $this->databaseName($this->projectId, $this->database);
 
-        $beginTransaction = $this->connection->beginTransaction([
-            'database' => $database
-        ] + $this->options['begin']);
+        try {
+            $beginTransaction = $this->connection->beginTransaction([
+                'database' => $database
+            ] + $this->options['begin']);
+        } catch (ServiceException $e) {
+            trigger_error(
+                sprintf('Firestore beginTransaction failed: %s', $e->getMessage()),
+                E_USER_WARNING
+            );
+        }
 
         $this->transaction = new Transaction(
             $this->connection,
@@ -392,7 +399,8 @@ private function commitTransaction()
     }
 
     /**
-     * Format the Firebase collection ID from the PHP session ID and session name.
+     * Format the Firebase collection ID from the PHP session ID and session
+     * name according to the $collectionNameTemplate option.
      * ex: sessions:PHPSESSID
      *
      * @param string $id Identifier used for the session
diff --git a/Firestore/tests/Unit/FirestoreSessionHandlerTest.php b/Firestore/tests/Unit/FirestoreSessionHandlerTest.php
index 879d28fa48c4..f39761631776 100644
--- a/Firestore/tests/Unit/FirestoreSessionHandlerTest.php
+++ b/Firestore/tests/Unit/FirestoreSessionHandlerTest.php
@@ -67,6 +67,24 @@ public function testOpen()
         $this->assertTrue($ret);
     }
 
+    /**
+     * @expectedException PHPUnit_Framework_Error_Warning
+     */
+    public function testOpenWithException()
+    {
+        $this->connection->beginTransaction(['database' => $this->dbName()])
+            ->shouldBeCalledTimes(1)
+            ->willThrow(new ServiceException(''));
+        $firestoreSessionHandler = new FirestoreSessionHandler(
+            $this->connection->reveal(),
+            $this->valueMapper->reveal(),
+            self::PROJECT,
+            self::DATABASE
+        );
+        $ret = $firestoreSessionHandler->open(self::SESSION_SAVE_PATH, self::SESSION_NAME);
+        $this->assertFalse($ret);
+    }
+
     /**
      * @expectedException InvalidArgumentException
      */