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

Fix Cookie Prefix #6024

Closed

Conversation

colethorsen
Copy link
Contributor

Fixes #6009

This allows the cookie_helper to enforce the default prefix if one exists, and also allows the user to set their own prefix including none at all to overwrite the prefix if necessary. This way there is no longer a collision issue if there are cookies that are prefixed and un-prefixed.

There is also an issue with the session setup where it would inconsistently set the name of the session cookie with and without the cookie prefix leading to multiple session cookies and also effectively creating the same prefixed vs un-prefixed issue.

Checklist:

  • Securely signed commits
  • Component(s) with PHPDoc blocks, only if necessary or adds value
  • Unit testing, with >80% coverage
  • User guide updated
  • Conforms to style guide

# Conflicts:
#	system/Helpers/cookie_helper.php
-There was a naming convention issue which could happen sometimes where adding a prefix would create 2 cookies for the session a properly prefixed session cookie and a un-prefixed session cookie
@kenjis
Copy link
Member

kenjis commented May 24, 2022

Thank you for sending a PR.

There is also an issue with the session setup where it would inconsistently set the name of the session cookie with and without the cookie prefix leading to multiple session cookies and also effectively creating the same prefixed vs un-prefixed issue.

I don't know this issue. Could you make it another PR?
We don't change the Session cookie name.
See #5951 (comment)

@kenjis kenjis added tests needed Pull requests that need tests breaking change Pull requests that may break existing functionalities enhancement PRs that improve existing functionalities labels May 24, 2022
@kenjis
Copy link
Member

kenjis commented May 24, 2022

This is my simple question.

We expect all contributions to conform to our style guide, be commented (inside the PHP source files), be documented (in the user guide), and unit tested (in the test folder).
https://github.com/codeigniter4/CodeIgniter4/blob/develop/contributing/pull_request.md#contributions

It is clearly stated that we need unit tests.
But almost (especially new) contributors don't write test code.
Do you know why?

@datamweb
Copy link
Contributor

Do you know why?

I understand why! 🥇

@colethorsen
Copy link
Contributor Author

colethorsen commented May 24, 2022

Well if it’s not supposed to set the prefix on the session cookie then it’s broken occasionally in the opposite way.

@kenjis
Copy link
Member

kenjis commented May 25, 2022

@colethorsen I don't know why the session is broken occasionally, and why this PR fixes it.

Anyway, the session cookie name is another issue.
Would you please remove it from this PR?
Otherwise this PR won't go ahead.

@kenjis kenjis changed the base branch from develop to 4.3 June 4, 2022 21:26
@kenjis kenjis added the 4.3 label Jun 7, 2022
@kenjis kenjis added the stale Pull requests with conflicts label Jun 10, 2022
@kenjis
Copy link
Member

kenjis commented Jun 16, 2022

#6082 was merged.

@kenjis kenjis closed this Jun 16, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4.3 breaking change Pull requests that may break existing functionalities enhancement PRs that improve existing functionalities stale Pull requests with conflicts tests needed Pull requests that need tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bug: get_cookie() cookie prefix behavior
3 participants