Add CookieBuilder property to SessionOptions and obsolete duplicated properties #173
Conversation
| public override string Name | ||
| { | ||
| get => base.Name; | ||
| set => base.Name = !string.IsNullOrEmpty(value) |
There was a problem hiding this comment.
Consider moving this check to the base class, there's no reason for the cookie name to ever be null/empty.
There was a problem hiding this comment.
| @@ -105,8 +105,11 @@ public class SessionTests | |||
| { | |||
| app.UseSession(new SessionOptions | |||
There was a problem hiding this comment.
Unrelated to this PR, but this is pretty gross, given that Session is using IOptions<SessionOptions> we should really kill this pattern.
If we want to avoid making them go Configure<SessionOptions>(o => ), we can change UseSession(o => o.Cookie.Name = "TestCookie"), this new SessionOptions/Wrapper pattern stuff needs to burn....
There was a problem hiding this comment.
@Tratcher @davidfowl @ajcvickers @divega thoughts? Can we get rid of all instances where we UseXyz(new XyzOptions()) and consistently always use UseXyz(o => )
There was a problem hiding this comment.
Assuming UseXyz is using IOptions...
There was a problem hiding this comment.
Worth considering, but not as part of this PR.
There was a problem hiding this comment.
Yeah, I just meant a cleanup pass for all middleware doing this, not something for this PR, I'll file an issue
|
|
||
| public override TimeSpan? Expiration | ||
| { | ||
| get => default(TimeSpan); |
natemcmaster
force-pushed
the
namc/cookie-builder
branch
from
931aee7 to
dece939
Compare
Addresses aspnet/HttpAbstractions#853.
Uses the new CookieBuilder API added here: aspnet/HttpAbstractions#882.
cc @HaoK @blowdart