Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

[HttpFoundation] Precalculate expiry timestamp#21423

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Conversation

@bcremer
Copy link
Contributor

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

Fix#13955.

QA
Branch?master
Bug fix?no
New feature?no
BC breaks?yes
Deprecations?no
Tests pass?yes
Fixed tickets#13955
LicenseMIT
Doc PRTBD

Removed timestamp and lifetime columns in favor of expiry column.Based on the work done insymfony#14625.Fixsymfony#13955.
@nicolas-grekas
Copy link
Member

nicolas-grekas commentedJan 29, 2017•
edited
Loading

But what about:

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

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

robfrawley reacted with thumbs up emoji

@nicolas-grekasnicolas-grekas added this to the3.x milestoneJan 29, 2017
@bcremerbcremerforce-pushed thesession-precalculate-expiry-timestamp branch from4833d08 to5342d67CompareJanuary 30, 2017 10:05
$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);
Copy link
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Setting thelifetimeCol to0 was inlined so we don't need different bind params here.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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

@bcremerbcremerforce-pushed thesession-precalculate-expiry-timestamp branch from5342d67 tocc4e4a5CompareJanuary 30, 2017 12:03
@bcremer
Copy link
ContributorAuthor

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

@nicolas-grekas
Copy link
Member

LGTM.@Tobion WDYT here?

* added the`Cookie::fromString()` method that allows to create a cookie from a
raw header string

* PdoSessionHandler: Deprecated the the`lifetime` column
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Extra "the".

$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) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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

$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);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

yoda

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

duplicate comment can be removed IMHO

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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


// 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";

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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

$sessionRows =$selectStmt->fetchAll(\PDO::FETCH_NUM);

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

true, the logic must be kept for old session data

"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) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

yoda

{
$storage =newPdoSessionHandler($this->getMemorySqlitePdo());
$storage =newPdoSessionHandler($this->getMemorySqlitePdo(),array('db_lifetime_col' =>false));
$storage->open('','sid');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Added by mistake?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Removed in#21857

@Tobion
Copy link
Contributor

Tobion commentedFeb 9, 2017•
edited
Loading

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
Copy link
Member

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

@bcremer
Copy link
ContributorAuthor

@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
Copy link
Contributor

robfrawley commentedFeb 18, 2017•
edited
Loading

@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
Copy link
Member

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

@robfrawley
Copy link
Contributor

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

@robfrawley
Copy link
Contributor

robfrawley commentedMar 3, 2017•
edited
Loading

@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
Copy link
Member

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

robfrawley reacted with thumbs up emoji

@xabbuh
Copy link
Member

closing in favour of#21857

@xabbuhxabbuh closed thisMar 4, 2017
@nicolas-grekasnicolas-grekas removed this from the3.x milestoneMar 24, 2017
@nicolas-grekasnicolas-grekas modified the milestones:3.3,3.xMar 24, 2017
nicolas-grekas added a commit that referenced this pull requestAug 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       | MITContinued work from the original PR#21423 /#21857Commits-------066000a [HttpFoundation] Precalculate session expiry timestamp
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@nicolas-grekasnicolas-grekasnicolas-grekas left review comments

+3 more reviewers

@TobionTobionTobion left review comments

@julienfalquejulienfalquejulienfalque left review comments

@robfrawleyrobfrawleyrobfrawley left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Projects

None yet

Milestone

3.3

Development

Successfully merging this pull request may close these issues.

[Session][3.0] PdoSessionHandler: precalculate expiry timestamp

7 participants

@bcremer@nicolas-grekas@Tobion@robfrawley@xabbuh@julienfalque@carsonbot
[8]ページ先頭
©2009-2025 Movatter.jp