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

Update expire date on expireCookie function #405

Merged
merged 2 commits into from Nov 25, 2018

Conversation

@gabrielgisoldo
Copy link
Contributor

commented Nov 22, 2018

According to https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Date and https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Set-Cookie , the date used on expires should be in the same format as Date on HTTP Header.
As of Firefox 60.3.0esr, passing the date in any other format doesn't work and is ignored by the browser.

Update expire date on expireCookie function
According to https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Date and https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Set-Cookie , the date used on expires should be in the same format as Date on HTTP Header.
As of Firefox 60.3.0esr, passing the date in any other format doesn't work and is ignored by the browser.
@dataflake

This comment has been minimized.

Copy link
Member

commented Nov 22, 2018

Hi Gabriel, did you sign a committer agreement (see https://docs.zope.org/developer/becoming-a-committer.html)? Without it, we cannot accept any contribution.

Secondly, the checkin breaks all current tests for that date format. Fixing those is a required part of a contribution like this.

Update expire date format on tests
Correcting all tests involving cookies to use the correct date format
@gabrielgisoldo

This comment has been minimized.

Copy link
Contributor Author

commented Nov 23, 2018

Hi Jens, I just sent the committer agreement and corrected the tests to use the new date format.
Can you take a look, please?

@ale-rt

ale-rt approved these changes Nov 23, 2018

@dataflake

This comment has been minimized.

Copy link
Member

commented Nov 23, 2018

I'm trying to verify the contributor agreement with those who handle them. I am not involved in that so I have to wait to hear back from them.

@dataflake

This comment has been minimized.

Copy link
Member

commented Nov 23, 2018

@gabrielgisoldo I just heard from Jim Fulton who gets the agreements at the official address contributor-admin@zope.org, and he says he has not received anything yet.

@dataflake dataflake merged commit 96627fb into zopefoundation:master Nov 25, 2018

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage remained the same at 80.639%
Details
@dataflake

This comment has been minimized.

Copy link
Member

commented Nov 25, 2018

@tseaver confirmed that the contributor agreement has been approved.

I checked several documentation resources about that date format for the Expires header. While I couldn't find any information about when it changed or why, I did notice that old documentation (pre-2010 roughly) still shows the old format we use in Zope, but anything after that uses the space-separated format. So with the tests passing this seems safe to merge.

@dataflake

This comment has been minimized.

Copy link
Member

commented Nov 25, 2018

P.S.: I've done the merge

icemac added a commit that referenced this pull request Nov 30, 2018

@zopefoundation zopefoundation deleted a comment from gabrielgisoldo Nov 30, 2018

@icemac icemac added enhancement and removed do not merge labels Nov 30, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.