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

Add a check to make sure the session column can hold the data#14647

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

Closed
gonzalovilaseca wants to merge2 commits intosymfony:2.7fromgonzalovilaseca:patch-1
Closed

Add a check to make sure the session column can hold the data#14647

gonzalovilaseca wants to merge2 commits intosymfony:2.7fromgonzalovilaseca:patch-1

Conversation

@gonzalovilaseca
Copy link
Contributor

QA
Bug fix?no
New feature?no
BC breaks?no
Deprecations?no
Tests pass?yes
Fixed tickets
LicenseMIT
Doc PR

We faced a problem in our app where the session data was being reset without any reason.
We traced it down to the data being truncated when saving, so when it was read, being corrupt, it was reset.
There was no error anywhere, so we thought this check would be nice to have.

…savedWe faced a problem in our app where the session data was being reset without any reason. We traced it down to the data being truncated when saving, so when it was read, being corrupt, it was reset.There was no error anywhere, so we thought this check would be nice to have.
@gonzalovilasecagonzalovilaseca changed the titleAdd a check to make sure the session can hold the dataAdd a check to make sure the session column can hold the dataMay 15, 2015
@keradus
Copy link
Member

no need for new test that covers this new exception throw ?

@gonzalovilaseca
Copy link
ContributorAuthor

You're right, will update PR with test

@Tobion
Copy link
Contributor

This cannot be merged becausegetColumnMeta doesn't work with all drivers. Also this slows down every session with an additional query. And the additional query even selects every row and thus blocks every session inside the transaction.

@Tobion
Copy link
Contributor

If strict SQL mode is not enabled and you assign a value to a BLOB or TEXT column that exceeds the column's maximum length, the value is truncated to fit and a warning is generated. For truncation of nonspace characters, you can cause an error to occur (rather than a warning) and suppress insertion of the value by using strict SQL mode.

https://dev.mysql.com/doc/refman/5.0/en/blob.html

So for mysql enabling strict sql mode should be sufficient.
Also 64kbyte session storage is possible with BLOB.@gonzalovilaseca what is the use-case to store more data in the session?

Copy link
Member

Choose a reason for hiding this comment

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

This notation does not work on PHP 5.3.

@gonzalovilaseca
Copy link
ContributorAuthor

@Tobion We store session in a Mysql in memory table therefore we cannot use blob or text, in our scenario the column can't be bigger than 21.000 chars.
Anyway we'll find a workaround for that, I don't see any neat solution for this in the PDO handler class.

@Tobion
Copy link
Contributor

So do you usevarbinary for the data column? It would give you 65,535 bytes, which is shared among all columns. Why only 21.000 bytes in your case?

@gonzalovilaseca
Copy link
ContributorAuthor

@Tobion We are using varchar. Don't know exactly why, but it doesn't allow a value greater than 21k. Anyway we can solve the issue by writing fewer data to session, but the background problem still remains, allthough it seems that nobody has had it before. Should we just close thir PR?

@Tobion
Copy link
Contributor

Closed in favor ofsymfony/symfony-docs#5269 as the only thing we can do is documentation

@TobionTobion closed thisMay 19, 2015
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@gonzalovilaseca@keradus@Tobion@fabpot

[8]ページ先頭

©2009-2025 Movatter.jp