Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[HttpFoundation] Precalculate session expiry timestamp #33169

Merged

Conversation

azjezz
Copy link
Contributor

@azjezz azjezz commented Aug 14, 2019

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

@azjezz azjezz force-pushed the precalculate-session-expiry-timestamp branch from 0aacdcd to cc3dc65 Compare August 14, 2019 15:00
@azjezz azjezz marked this pull request as ready for review August 14, 2019 15:00
@azjezz azjezz force-pushed the precalculate-session-expiry-timestamp branch 12 times, most recently from 3ac6cf7 to 1d7737c Compare August 14, 2019 17:01
@nicolas-grekas
Copy link
Member

nicolas-grekas commented Aug 15, 2019

The migration path doesn't work IMHO.
Instead, I propose to keep the name of the column and of the property as is.
Then, on table creation, the column should be indexed, and we should use it to store absolute expiries. We should tell in UPGRADE files which query ppl need to run to add the index to their existing DB.
To have smooth compat with current sessions, to decide when a value in the column is a lifetime or an expiry, we would compare values to 315576000 (10 years): when the number is higher, it's an absolute date, when it's ower, it's a relative date.
GC would be done in two DELETE queries, for >315576000 and <=315576000.
In v6, we would simply drop the <=315576000 one, because in two years we'll be able to consider that all relevant sessions have been migrated to absolute dates.

@nicolas-grekas nicolas-grekas added this to the next milestone Aug 15, 2019
@azjezz azjezz force-pushed the precalculate-session-expiry-timestamp branch from 1d7737c to e4f4a71 Compare August 15, 2019 09:02
@azjezz azjezz force-pushed the precalculate-session-expiry-timestamp branch 7 times, most recently from 37a924c to 1e5586d Compare August 15, 2019 15:23
@azjezz azjezz force-pushed the precalculate-session-expiry-timestamp branch from 1e5586d to 9dd3b93 Compare August 16, 2019 08:58
@azjezz azjezz force-pushed the precalculate-session-expiry-timestamp branch 4 times, most recently from e849cbb to 0d57343 Compare August 19, 2019 09:00
Co-authored-by: Benjamin Cremer <b.cremer@shopware.com>
Co-authored-by: Rob Frawley 2nd <rmf@src.run>
@azjezz azjezz force-pushed the precalculate-session-expiry-timestamp branch from 0d57343 to 066000a Compare August 19, 2019 09:02
@nicolas-grekas
Copy link
Member

Thank you @azjezz.

nicolas-grekas added a commit that referenced this pull request Aug 20, 2019
… (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
@nicolas-grekas nicolas-grekas merged commit 066000a into symfony:4.4 Aug 20, 2019
@nicolas-grekas nicolas-grekas modified the milestones: next, 4.4 Oct 27, 2019
This was referenced Nov 12, 2019
@tseho
Copy link
Contributor

tseho commented Nov 21, 2019

Hello @nicolas-grekas, I think this is a BC break. On mysql, the lifetime column has been for the last few years a MEDIUMINT column.

The new version with time() + $maxlifetime instead of $maxlifetime is higher than the MEDIUMINT mysql limit.

When upgrading from symfony 4.3 to 4.4 with an existing database, we have the error SQLSTATE[22003]: Numeric value out of range: 1264 Out of range value for column 'sess_lifetime' at row 1

I saw the creation of the session table has been fixed 5 days ago in #34410 but that does not fix existing databases.

Is it normal we need to create manually a sql migration for a breaking database schema change, which is not really mentioned in UPGRADE-4.4.md ?

@nicolas-grekas
Copy link
Member

That's an interesting feedback @tseho but please create an issue: nobody is tracking comments on closed PRs.

@azjezz azjezz deleted the precalculate-session-expiry-timestamp branch November 21, 2019 13:30
@tseho
Copy link
Contributor

tseho commented Nov 21, 2019

Thanks for your response and sorry about that, I just created a new issue : #34491

fabpot added a commit that referenced this pull request Sep 21, 2021
… session (IonBazan)

This PR was squashed before being merged into the 6.0 branch.

Discussion
----------

[HttpFoundation] [PDO] Don't fetch time when reading the session

| Q             | A
| ------------- | ---
| Branch?       | 6.0
| Bug fix?      | no
| New feature?  | no
| Deprecations? | no
| Tickets       |
| License       | MIT
| Doc PR        |

Following up #33169 (comment)

BTW is there a reason we are using `FETCH_NUM` instead of `FETCH_ASSOC` for more readability?

Commits
-------

fb9508f [HttpFoundation] [PDO] Don't fetch time when reading the session
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants