Added usage of TimeProvider instead of DateTimeOffset - #63042
Conversation
|
@dotnet-policy-service agree |
| var lockoutTime = await store.GetLockoutEndDateAsync(user, CancellationToken).ConfigureAwait(false); | ||
| return lockoutTime >= DateTimeOffset.UtcNow; | ||
| #if NET8_0_OR_GREATER | ||
| var utcNow = UtcNow(); |
There was a problem hiding this comment.
If you made the #if just change the behaviour of UtcNow() you could do it in just one place and then use the method everywhere.
There was a problem hiding this comment.
Using #if inside UtcNow() causes CA1822 error and results in build failure. This happens because the ServiceProvider field is used only in .NET 8 and higher versions. For older versions, this method should be marked static.
private DateTimeOffset UtcNow()
{
#if NET8_0_OR_GREATER
var timeProvider = ServiceProvider.GetService<TimeProvider>();
return timeProvider?.GetUtcNow() ?? DateTimeOffset.UtcNow;
#else
return DateTimeOffset.UtcNow;
#endif
}
Another solution I see is to create two separate methods for UtcNow(). One of them should be static for older versions and the other should not.
#if NET8_0_OR_GREATER
private DateTimeOffset UtcNow()
{
var timeProvider = ServiceProvider.GetService<TimeProvider>();
return timeProvider?.GetUtcNow() ?? DateTimeOffset.UtcNow;
}
#else
private static DateTimeOffset UtcNow()
{
return DateTimeOffset.UtcNow;
}
#endif
If you have any other ideas or approaches for this, please let me know.
There was a problem hiding this comment.
If it were me I would do the second one.
There was a problem hiding this comment.
Changed UtcNow method implementation
martincostello
left a comment
There was a problem hiding this comment.
Maybe add a test that demonstrates it's used for .NET 8.0+, but otherwise looks ok to me.
Added unit test that verifies using TimeProvider |
|
@martincostello, do I need to rerun CI/CD myself and ping reviewer for last approve? |
|
If you close and reopen the PR it will run it again. |
|
@martincostello, is there anything else I should do to merge this PR? |
|
Assuming the test failures aren't due to your changes, you'll just have to wait for someone from the aspnetcore team to look at it and merge it. |
|
/azp run |
|
Azure Pipelines successfully started running 2 pipeline(s). |
|
@StickFun Sorry for the delay in reviewing this, and thanks for the contribution! |
|
/azp run |
|
Azure Pipelines successfully started running 2 pipeline(s). |
|
/ba-g The build analysis check seems to be remembering an old failure that is long lost and not occurring on current runs. |
|
Thanks again @StickFun! |
Added usage of TimeProvider instead of DateTimeOffset
Description
Added usage of TimeProvider for NET 8 and higher versions.
Other versions using DateTimeOffset
Fixes #62796