Skip to content

Change in table creation SQL for SQL Server #14823

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

Merged
merged 1 commit into from
Jan 13, 2021
Merged

Conversation

0fca
Copy link
Contributor

@0fca 0fca commented Jan 12, 2021

I propose to change VARBINARY to NVARCHAR, because for the newest version of drivers and SQL Server 17 or newer this script creates a table which is not valid: PdoSessionHandler actually tries to insert string in sess_data not binary data.
What's more: VARBINARY(max) is about 8kB long at most whereas NVARCHAR can contain up to 2GB of data.
There is of course another way, suggested by ODBC error while trying to insert data to sessions created like shown: use convert during inserting, however I would not consider it a way.

I propose to change VARBINARY to NVARCHAR, because for the newest version of drivers and SQL Server 17 or newer this script creates a table which is not valid: PdoSessionHandler actually tries to insert string in sess_data not binary data. 
What's more: VARBINARY(max) is about 8kB long at most whereas NVARCHAR can contain up to 2GB of data. 
There is of course another way, suggested by ODBC error while trying to insert data to sessions created like shown: use convert during inserting, however I would not consider it a way.
@carsonbot carsonbot added this to the 4.4 milestone Jan 12, 2021
@carsonbot
Copy link
Collaborator

Hey!

I see that more good work is coming your way.

I think @greg0ire has recently worked with this code. Maybe they can help review this?

Cheers!

Carsonbot

Copy link
Contributor

@greg0ire greg0ire left a comment

Choose a reason for hiding this comment

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

Great commit message!

@OskarStark
Copy link
Contributor

Thank you Lukas.

@OskarStark OskarStark merged commit 93169c7 into symfony:4.4 Jan 13, 2021
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.

4 participants