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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix tld_length documentation in ActionDispatch::Cookies #31195

Merged
merged 1 commit into from Nov 24, 2017

Conversation

mltsy
Copy link
Contributor

@mltsy mltsy commented Nov 21, 2017

Summary

Change recommendation for tld_length (for sharing cookies across subdomains of a 2-token TLD), to 2 instead of 1 (TLD isn't exactly the right name here, but since that's what the option is called, that's what I'm calling it). This option actually specifies the number of tokens in the root domain across whose subdomains you want to share the cookie. In the example given, it is 2 (lvh and me), so recommending 1 is confusing. (calling it tld_length is also confusing, but c'est la vie) 馃槈

With a setting of 2, the domain for the cookie is set to: .lvh.me which is the desired result.

Change recommendation for tld_length (for sharing cookies across subdomains of a 2-token TLD), to 2 instead of 1.
@rails-bot
Copy link

Thanks for the pull request, and welcome! The Rails team is excited to review your changes, and you should hear from @pixeltrix (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

This repository is being automatically checked for code quality issues using Code Climate. You can see results for this analysis in the PR status below. Newly introduced issues should be fixed before a Pull Request is considered ready to review.

Please see the contribution instructions for more information.

@mltsy
Copy link
Contributor Author

mltsy commented Nov 21, 2017

I suppose the rest of the text for this description could probably be updated too, since it seems to indicate that tld_length is for setting the length of the TLD. Alternately, if this option is actually supposed set the length of the TLD (rather than the root domain for cookie sharing), and domain: :all is supposed to add one more token to that TLD to determine the root domain for cookie sharing, then the implementation is incorrect... not sure which is the case!

I'd be happy to update this PR in either case, given direction :)

@vipulnsward vipulnsward merged commit 65ff01e into rails:master Nov 24, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants