-
Notifications
You must be signed in to change notification settings - Fork 29
Deprecate pointless cookie expiry facades #45
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
Deprecate pointless cookie expiry facades #45
Conversation
| ; | ||
|
|
||
| $response = FigResponseCookies::expire($response, 'session_cookie'); | ||
| FigResponseCookies::set($response, $setCookie->expire()); |
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.
One alternative I see:
FigResponseCookies::expire($response, $setCookie);Hardly more elegant and slightly harder in terms of BC. Your choice. 😃
|
Tests failures:
I can only assume that |
|
@franzliedke Would you be able to try playing with
However, based on the Based on this stackoverflow answer it might be that we need to run the following additional command right before the - run: vendor/bin/phpunit --coverage-text --coverage-clover=coverage.clover
if: ${{ matrix.coverage != 'none' }}
# new thing to unshallow our checkout as `ocular` needs parent commits
- run: git fetch --unshallow
- run: php vendor/bin/ocular code-coverage:upload --format=php-clover coverage.clover
if: ${{ matrix.coverage != 'none' }} |
|
Sure, I would be happy to. Thanks for the additional research. I will do it in an additional MR, so that
|
|
I will rebase once #46 has been merged. |
b87d904 to
168d37b
Compare
|
Rebased, and tests are green. 🙂 |
|
@franzliedke Thanks! |
As outlined in #23 (the problem also came up in #22), some of the "utilities" I introduced in #4 / #10 don't make sense. To expire cookies, the
Set-Cookieheader needs to have the same parameters that were used when creating them.Therefore, this PR:
Sorry for not thinking this through properly in the original PR - and for taking a while to fix this. 😉
Thanks for the great library! 🙌🏼
Fixes #22, #23.