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

Add a note to clarify header.Set behavior on cookies #1864

Merged
merged 1 commit into from
Sep 25, 2024

Conversation

sigmundxia
Copy link
Contributor

Hi Developers,

As described in this issue, currently header.Set does not reset but adds cookies. This pull request attempts to fix this so that header.Set resets cookies instead of adding them.

Feel free to let me know if there are any further changes you would like me to make.

Best regards!

header.go Show resolved Hide resolved
@erikdubbelboer
Copy link
Collaborator

This would be a backwards incompatible change for anyone using header.Set with Cookie. I don't think we should just change it because of that. I think a better solution would be to really make this clear in the documentation that you need to delete the 'Cookie' header before setting it if you don't want to add.

@sigmundxia
Copy link
Contributor Author

Indeed, changing the behavior of this method may cause compatibility issues for downstream software. I will try to make direct improvements to gofiber downstream. So, should this pull request be closed? Or should I undo all changes here and add a note about cookies in the code comments?

@erikdubbelboer
Copy link
Collaborator

Yeah sorry lets change this pull to only add a note to the documentation to clarify this behavior. Thanks!

@sigmundxia sigmundxia reopened this Sep 24, 2024
@sigmundxia sigmundxia changed the title Make header.Set reset cookies instead of adding them Add a note to clarify header.Set behavior on cookies Sep 24, 2024
@sigmundxia
Copy link
Contributor Author

Yeah sorry lets change this pull to only add a note to the documentation to clarify this behavior. Thanks!

No problem, I also think a change to the description would be ideal 😊 I've made the changes, let me know if there are any further changes needed!

@erikdubbelboer erikdubbelboer merged commit 441750d into valyala:master Sep 25, 2024
30 checks passed
@erikdubbelboer
Copy link
Collaborator

Thanks, it's good!

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.

3 participants