-
Notifications
You must be signed in to change notification settings - Fork 837
setcookie: Add note regarding SameSite=None and disabled Secure behavior #4916
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
base: master
Are you sure you want to change the base?
Conversation
TimWolla
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Girgias might have markup opinions here?
Girgias
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed I have some markup notes, the wrapping para is very much unnecessary and please use <simpara> and fix the above note at the same time :)
|
In this case fixing this instance would have created an inconsistency with the markup across rest of this file, so I've reviewed the whole file to make it consistent. |
Girgias
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for the delay, but going throught the file just brings to light more issues :/
|
I've fixed all the review issues, including CDATA indents and trailing whitespace. Additionally, I changed the suggestion to consider explode() for setting multiple values in a single cookie to json_encode() as I think this is easier / less error prone to use correctly. |
Fixes #4873