Skip to content
This repository was archived by the owner on Dec 13, 2018. It is now read-only.

Revert obsoleting CookieAuthenticationOptions.ExpireTimeSpan #1296

Merged
merged 2 commits into from
Jul 5, 2017

Conversation

natemcmaster
Copy link
Contributor

CookieAuthenticationOptions.Cookie.Expiration is conceptually different from CookieAuthenticationOptions.ExpireTimeSpan.

Un-oboleting the property and updating xmldocs.

@natemcmaster natemcmaster requested review from Tratcher and HaoK July 5, 2017 22:22
@dnfclas
Copy link

dnfclas commented Jul 5, 2017

@natemcmaster,
Thanks for having already signed the Contribution License Agreement. Your agreement was validated by .NET Foundation. We will now review your pull request.
Thanks,
.NET Foundation Pull Request Bot

@HaoK
Copy link
Member

HaoK commented Jul 5, 2017

Changes look fine, but are we missing some test coverage since shouldn't a test have to be updated? :) They probably have always been missing, but maybe we should take this opportunity to add 1-2 basic tests that actually enforce the behavior we expect?

@natemcmaster
Copy link
Contributor Author

A bunch tests in CookieTests.cs were updated. Which behavior should we test for that isn't already covered?

@HaoK
Copy link
Member

HaoK commented Jul 5, 2017

I mean the Cookie.Expires property I guess, or is that the one we basically ignore?

@natemcmaster
Copy link
Contributor Author

@HaoK
Copy link
Member

HaoK commented Jul 5, 2017

Okay, worth adding a single test to capture that we ignore CookieBuilder.Expires so we don't regress this accidentally in the future?

@natemcmaster
Copy link
Contributor Author

Added a test.

@natemcmaster
Copy link
Contributor Author

Btw, let's use https://github.com/aspnet/Security/issues/1293 to clarify what CookieBuilder.Expiration should mean. We can address this after 2.0.0. For now, we'll just ignore its value.

@natemcmaster natemcmaster merged commit bd19ba9 into dev Jul 5, 2017
@natemcmaster natemcmaster deleted the namc/un-obsolete branch July 5, 2017 22:43
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants