Skip to content

Update expires format for session cookie #9304

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

Merged
merged 2 commits into from
Aug 12, 2022

Conversation

TimWolla
Copy link
Member

Even if DATE_FORMAT_COOKIE is not updated, at the very least regular cookies and sessions cookies should be consistent.


see GH-9200
see 15e3fcb

Copy link
Member

@derickr derickr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good. I hadn't realised that this didn't go through the normal php_setcookie API. Should it?

Do see if you can add a test though.

@TimWolla
Copy link
Member Author

Should it?

All things being equal, it should. I might look into this as a follow-up improvement.

Do see if you can add a test though.

I had a look at creating a test, but to my understanding this is non-trivial, unless I want to create a flaky test. As I can't fixate the current time, I can't control the expiry time. Even just adding some time amount X to the current time, might cause false failures if the second rolls over.

@cmb69
Copy link
Member

cmb69 commented Aug 12, 2022

I had a look at creating a test, but to my understanding this is non-trivial,

I think it would be sufficient to test the format, not any particular date.

@TimWolla
Copy link
Member Author

I think it would be sufficient to test the format, not any particular date.

Yes, but this is also going to get ugly quickly: EXPECTHEADERS doesn't support regular expressions and while I could technically use headers_list(), this would require me to find the set-cookie header in there and then use preg_match() to verify the format. However based on my understanding preg_match() is not even guaranteed to exist, because pcre might not be compiled.

@cmb69
Copy link
Member

cmb69 commented Aug 12, 2022

I have to admit, that this test would be harder to write than it should, but you can rely on PCRE being available; it is a required extension.

@TimWolla
Copy link
Member Author

but you can rely on PCRE being available; it is a required extension.

Oh, indeed. Not sure where I looked before to get the false impression that it isn't. I'll have a look at adding the test then.

@cmb69
Copy link
Member

cmb69 commented Aug 12, 2022

BTW, the fact that PCRE is required is the reason for us to bundle libpcre.

Copy link
Member

@cmb69 cmb69 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you! Looks good to me (assuming AppVeyor CI passed).

@TimWolla TimWolla merged commit b825756 into php:master Aug 12, 2022
@TimWolla TimWolla deleted the session-cookie-expires branch August 12, 2022 17:52
nicolas-grekas added a commit to symfony/symfony that referenced this pull request Aug 17, 2022
This PR was merged into the 4.4 branch.

Discussion
----------

[HttpFoundation] Fix tests on PHP 8.2

| Q             | A
| ------------- | ---
| Branch?       | 4.4
| Bug fix?      | no
| New feature?  | no
| Deprecations? | no
| Tickets       | -
| License       | MIT
| Doc PR        | -

Follows php/php-src#9304

Commits
-------

1c574bb [HttpFoundation] Fix tests on PHP 8.2
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants