[HttpFoundation] Precalculate expiry timestamp #21423

bcremer · 2017-01-26T17:40:00Z

Removed timestamp and lifetime columns in favor of expiry column.
Based on the work done in #14625.

Q	A
Branch?	master
Bug fix?	no
New feature?	no
BC breaks?	yes
Deprecations?	no
Tests pass?	yes
Fixed tickets	#13955
License	MIT
Doc PR	TBD

Removed timestamp and lifetime columns in favor of expiry column. Based on the work done in symfony#14625. Fix symfony#13955.

nicolas-grekas · 2017-01-29T19:40:24Z

But what about:

keeping the sess_time column asis, but put the expiry inside,
fill the lifetime column with zeros
trigger a deprecation when false !== $options['db_lifetime_col']
when $options['db_lifetime_col'] === false, don't use the lifetime_col at all

In 4.0, we could then just drop the lifetime_col, and keep the sess_time one as is (we don't need to rename it - and btw the DbalSessionHandler already calls it sess_time and puts the expiry inside).

bcremer · 2017-01-30T10:07:43Z

src/Symfony/Component/HttpFoundation/Session/Storage/Handler/PdoSessionHandler.php

@@ -689,16 +762,13 @@ private function getMergeStatement($sessionId, $data, $maxlifetime)
                $mergeStmt->bindParam(1, $sessionId, \PDO::PARAM_STR);
                $mergeStmt->bindParam(2, $sessionId, \PDO::PARAM_STR);
                $mergeStmt->bindParam(3, $data, \PDO::PARAM_LOB);
-                $mergeStmt->bindParam(4, $maxlifetime, \PDO::PARAM_INT);


Setting the lifetimeCol to 0 was inlined so we don't need different bind params here.

bcremer · 2017-01-30T10:08:49Z

src/Symfony/Component/HttpFoundation/Session/Storage/Handler/PdoSessionHandler.php

@@ -97,6 +97,7 @@ class PdoSessionHandler implements \SessionHandlerInterface

    /**
     * @var string Column for lifetime
+     * @deprecated since version 3.3, to be removed in 4.0
     */
    private $lifetimeCol = 'sess_lifetime';


Should this be changed to false? Right now the new behavior it opt-in.

it must be opt-in, to keep BC by default

bcremer · 2017-02-09T09:54:08Z

@nicolas-grekas Anything still todo for me here to get this merged?

nicolas-grekas · 2017-02-09T15:32:40Z

LGTM. @Tobion WDYT here?

julienfalque · 2017-02-09T15:37:09Z

src/Symfony/Component/HttpFoundation/CHANGELOG.md

@@ -8,7 +8,8 @@ CHANGELOG
   disabling `Range` and `Content-Length` handling, switching to chunked encoding instead
 * added the `Cookie::fromString()` method that allows to create a cookie from a
   raw header string
-
+ * PdoSessionHandler: Deprecated the the `lifetime` column


Extra "the".

nicolas-grekas · 2017-02-09T15:33:32Z

src/Symfony/Component/HttpFoundation/Session/Storage/Handler/PdoSessionHandler.php

@@ -193,6 +195,10 @@ public function __construct($pdoOrDsn = null, array $options = array())
        $this->password = isset($options['db_password']) ? $options['db_password'] : $this->password;
        $this->connectionOptions = isset($options['db_connection_options']) ? $options['db_connection_options'] : $this->connectionOptions;
        $this->lockMode = isset($options['lock_mode']) ? $options['lock_mode'] : $this->lockMode;
+
+        if ($this->lifetimeCol !== false) {


yoda style (false !== ...) same below

nicolas-grekas · 2017-02-09T15:38:00Z

src/Symfony/Component/HttpFoundation/Session/Storage/Handler/PdoSessionHandler.php

@@ -193,6 +195,10 @@ public function __construct($pdoOrDsn = null, array $options = array())
        $this->password = isset($options['db_password']) ? $options['db_password'] : $this->password;
        $this->connectionOptions = isset($options['db_connection_options']) ? $options['db_connection_options'] : $this->connectionOptions;
        $this->lockMode = isset($options['lock_mode']) ? $options['lock_mode'] : $this->lockMode;
+
+        if ($this->lifetimeCol !== false) {
+            @trigger_error('Using the "db_lifetime_col" option is deprecated since version 3.3 as it will be removed in 4.0.', E_USER_DEPRECATED);


Should give more insights I think, eg
sprintf('The "%s" column is deprecated since version 3.3 and won't be used anymore in 4.0. Migrate your session database then set the "db_lifetime_col" option to false to opt-in for the new behavior.', $this->lifetimeCol);

I think this deserves an entry in the UPGRADE-3.3+4.0 files, with extended migration info/SQL.

nicolas-grekas · 2017-02-09T15:38:30Z

src/Symfony/Component/HttpFoundation/Session/Storage/Handler/PdoSessionHandler.php

-                break;
-            default:
-                throw new \DomainException(sprintf('Creating the session table is currently not implemented for PDO driver "%s".', $this->driver));
+        if ($this->lifetimeCol === false) {


nicolas-grekas · 2017-02-09T15:38:59Z

src/Symfony/Component/HttpFoundation/Session/Storage/Handler/PdoSessionHandler.php

+        } else {
+            switch ($this->driver) {
+                case 'mysql':
+                    // We use varbinary for the ID column because it prevents unwanted conversions:


duplicate comment can be removed IMHO

nicolas-grekas · 2017-02-09T15:40:39Z

src/Symfony/Component/HttpFoundation/Session/Storage/Handler/PdoSessionHandler.php

            );
            $updateStmt->bindParam(':id', $sessionId, \PDO::PARAM_STR);
            $updateStmt->bindParam(':data', $data, \PDO::PARAM_LOB);
-            $updateStmt->bindParam(':lifetime', $maxlifetime, \PDO::PARAM_INT);


lifetime should be set to zero so that migration is always possible ("time += lifetime")

Can you expand on what you mean here: where should it to 0?

nicolas-grekas · 2017-02-09T15:41:38Z

src/Symfony/Component/HttpFoundation/Session/Storage/Handler/PdoSessionHandler.php

@@ -389,7 +426,7 @@ public function close()
            $this->gcCalled = false;

            // delete the session records that have expired
-            $sql = "DELETE FROM $this->table WHERE $this->lifetimeCol + $this->timeCol < :time";
+            $sql = "DELETE FROM $this->table WHERE $this->timeCol < :time";


for BC, we should still consider lifetime when the colum is used, isn't it?

nicolas-grekas · 2017-02-09T15:42:14Z

src/Symfony/Component/HttpFoundation/Session/Storage/Handler/PdoSessionHandler.php

@@ -510,7 +547,7 @@ private function doRead($sessionId)
            $sessionRows = $selectStmt->fetchAll(\PDO::FETCH_NUM);

            if ($sessionRows) {
-                if ($sessionRows[0][1] + $sessionRows[0][2] < time()) {


same here, for BC, we should still use any values in lifetime?

true, the logic must be kept for old session data

nicolas-grekas · 2017-02-09T15:43:02Z

src/Symfony/Component/HttpFoundation/Session/Storage/Handler/PdoSessionHandler.php

-                    "ON CONFLICT ($this->idCol) DO UPDATE SET ($this->dataCol, $this->lifetimeCol, $this->timeCol) = (EXCLUDED.$this->dataCol, EXCLUDED.$this->lifetimeCol, EXCLUDED.$this->timeCol)";
-                break;
+
+        if ($this->lifetimeCol === false) {


Tobion · 2017-02-09T16:15:48Z

src/Symfony/Component/HttpFoundation/Tests/Session/Storage/Handler/PdoSessionHandlerTest.php

@@ -215,7 +215,8 @@ public function testReadingRequiresExactlySameId()
     */
    public function testWriteDifferentSessionIdThanRead()
    {
-        $storage = new PdoSessionHandler($this->getMemorySqlitePdo());
+        $storage = new PdoSessionHandler($this->getMemorySqlitePdo(), array('db_lifetime_col' => false));
+        $storage->open('', 'sid');


Added by mistake?

Removed in #21857

Tobion · 2017-02-09T16:26:58Z

Approach seems ok but it's not BC yet.
Also there is no index on the time column yet. So I don't think there is any gain as the DB still needs to do a full table scan for gc.

nicolas-grekas · 2017-02-18T08:46:36Z

@bcremer do you need help - or just a bit more time? please advise :)

bcremer · 2017-02-18T10:34:17Z

@nicolas-grekas It's not my top priority right know. If anybody likes to pickup please do :)

Will get back onto it otherwise in a few days or so.

robfrawley · 2017-02-18T10:53:07Z

@bcremer I'm happy to help get this finished up; should I clone your branch, make the required changes, and open a new PR, or would you prefer (and do you have the time) to accept pull requests against your branch (this pull request).

nicolas-grekas · 2017-03-01T16:15:33Z

@robfrawley please go with a new PR on your own, keeping the first commit from @bcremer, OK?

robfrawley · 2017-03-01T18:32:50Z

@nicolas-grekas Sounds good, will take a look and get something together for tomorrow.

robfrawley · 2017-03-03T05:40:59Z

@nicolas-grekas Nevermind my original comment; it was my mistake, though there are some BC breaks in this PR. I've already taken care of the style changes (like the yoda-style comments and others) requested. Will be opening up a new PR shortly, but before doing so, I wanted to ask if you guys would like BC unit tests created with the proper @group and @expectedDeprecation annotations, like:

/**
 * @group legacy
 * @expectedDeprecation The "%s" column is deprecated since version 3.3 and won't be used anymore in 4.0. Migrate your session database then set the "db_lifetime_col" option to false to opt-in for the new behavior.
 */

nicolas-grekas · 2017-03-03T05:59:57Z

BC is a requirement yes, and legacy and non-legacy tests would be great yes.

xabbuh · 2017-03-04T19:13:11Z

closing in favour of #21857

… (azjezz) This PR was merged into the 4.4 branch. Discussion ---------- [HttpFoundation] Precalculate session expiry timestamp | Q | A | ------------- | --- | Branch? | 4.4 | Bug fix? | no | New feature? | yes | BC breaks? | no | Deprecations? | yes | Tests pass? | yes | Fixed tickets | #13955, #21423, #14625 | License | MIT Continued work from the original PR #21423 / #21857 Commits ------- 066000a [HttpFoundation] Precalculate session expiry timestamp

[HttpFoundation] Precalculate expiry timestamp

5780201

Removed timestamp and lifetime columns in favor of expiry column. Based on the work done in symfony#14625. Fix symfony#13955.

carsonbot added Status: Needs Review HttpFoundation BC Break labels Jan 26, 2017

nicolas-grekas added this to the 3.x milestone Jan 29, 2017

bcremer force-pushed the session-precalculate-expiry-timestamp branch from 4833d08 to 5342d67 Compare January 30, 2017 10:05

bcremer commented Jan 30, 2017

View reviewed changes

Implement CR suggestions from nicolas-grekas

cc4e4a5

bcremer force-pushed the session-precalculate-expiry-timestamp branch from 5342d67 to cc4e4a5 Compare January 30, 2017 12:03

julienfalque reviewed Feb 9, 2017

View reviewed changes

nicolas-grekas reviewed Feb 9, 2017

View reviewed changes

Tobion reviewed Feb 9, 2017

View reviewed changes

nicolas-grekas added Status: Needs Work and removed Status: Needs Review labels Feb 14, 2017

robfrawley mentioned this pull request Mar 3, 2017

[HttpFoundation] Precalculate session expiry timestamp #21857

Closed

6 tasks

xabbuh closed this Mar 4, 2017

nicolas-grekas removed this from the 3.x milestone Mar 24, 2017

nicolas-grekas modified the milestones: 3.3, 3.x Mar 24, 2017

azjezz mentioned this pull request Aug 14, 2019

[HttpFoundation] Precalculate session expiry timestamp #33169

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[HttpFoundation] Precalculate expiry timestamp #21423

[HttpFoundation] Precalculate expiry timestamp #21423

bcremer commented Jan 26, 2017

nicolas-grekas commented Jan 29, 2017 •

edited

Loading

bcremer Jan 30, 2017

bcremer Jan 30, 2017

nicolas-grekas Jan 30, 2017

bcremer commented Feb 9, 2017

nicolas-grekas commented Feb 9, 2017

julienfalque Feb 9, 2017

nicolas-grekas Feb 9, 2017

nicolas-grekas Feb 9, 2017

nicolas-grekas Feb 9, 2017

nicolas-grekas Feb 9, 2017

nicolas-grekas Feb 9, 2017

robfrawley Mar 4, 2017

nicolas-grekas Feb 9, 2017

nicolas-grekas Feb 9, 2017

Tobion Feb 9, 2017

nicolas-grekas Feb 9, 2017

Tobion Feb 9, 2017

robfrawley Mar 4, 2017

Tobion commented Feb 9, 2017 •

edited

Loading

nicolas-grekas commented Feb 18, 2017

bcremer commented Feb 18, 2017

robfrawley commented Feb 18, 2017 •

edited

Loading

nicolas-grekas commented Mar 1, 2017

robfrawley commented Mar 1, 2017

robfrawley commented Mar 3, 2017 •

edited

Loading

nicolas-grekas commented Mar 3, 2017

xabbuh commented Mar 4, 2017

[HttpFoundation] Precalculate expiry timestamp #21423

[HttpFoundation] Precalculate expiry timestamp #21423

Conversation

bcremer commented Jan 26, 2017

nicolas-grekas commented Jan 29, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bcremer commented Feb 9, 2017

nicolas-grekas commented Feb 9, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Tobion commented Feb 9, 2017 • edited Loading

nicolas-grekas commented Feb 18, 2017

bcremer commented Feb 18, 2017

robfrawley commented Feb 18, 2017 • edited Loading

nicolas-grekas commented Mar 1, 2017

robfrawley commented Mar 1, 2017

robfrawley commented Mar 3, 2017 • edited Loading

nicolas-grekas commented Mar 3, 2017

xabbuh commented Mar 4, 2017

nicolas-grekas commented Jan 29, 2017 •

edited

Loading

Tobion commented Feb 9, 2017 •

edited

Loading

robfrawley commented Feb 18, 2017 •

edited

Loading

robfrawley commented Mar 3, 2017 •

edited

Loading