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

Switch DynamoDb session data type to binary (B) #1835

Closed
wants to merge 1 commit into from

Conversation

dnsl48
Copy link

@dnsl48 dnsl48 commented Jul 1, 2019

Fixes #1831

  • change DynamoDb session data type from S to B so PHP strings are handled in a binary safe manner

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@codecov-io
Copy link

codecov-io commented Jul 1, 2019

Codecov Report

Merging #1835 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master    #1835   +/-   ##
=========================================
  Coverage     94.11%   94.11%           
  Complexity     3125     3125           
=========================================
  Files           179      179           
  Lines          8341     8341           
=========================================
  Hits           7850     7850           
  Misses          491      491
Impacted Files Coverage Δ Complexity Δ
src/DynamoDb/StandardSessionConnection.php 100% <100%> (ø) 15 <0> (ø) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update be04607...6eed90d. Read the comment docs.

@srchase
Copy link
Contributor

srchase commented Jul 1, 2019

@dnsl48

Thanks for submitting this PR!

Switching the data type from S to B makes sense, but would be a breaking change for existing customers that expect session data to be stored as a string.

Would you be interested in expanding this PR to adding a config option for data_attribute_type that would default to string, but allow binary to be used for the data_attribute? That configuration is handled in SessionConnectionConfigTrait.

@srchase srchase self-assigned this Jul 1, 2019
@dnsl48
Copy link
Author

dnsl48 commented Jul 1, 2019

@srchase
Yes, sure, makes sense. I will add the config option, thank you for the advice :)

@srchase
Copy link
Contributor

srchase commented Jul 3, 2019

@dnsl48

I went ahead and opened a PR to add this as an option: #1838

Thanks for raising this issue!

@dnsl48
Copy link
Author

dnsl48 commented Jul 3, 2019

@srchase
Your PR looks much more solid :)
Thank you for taking care of that!

@dnsl48 dnsl48 closed this Jul 3, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

DynamoDB StandardSessionConnection is not binary safe
3 participants