Skip to content

Prefer IHttpContextAccessor over HttpContextAccessor #471

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 5 commits into from
Jan 18, 2024

Conversation

twsouthwick
Copy link
Member

This change will check if we're currently in a runtime environment that contains a IHttpContextAccessor; if we are, then we'll prefer that over HttpContextAccessor. This allows people to potentially customize what it means to get and set a HttpContext.

This change will check if we're currently in a runtime environment that contains a IHttpContextAccessor; if we are, then we'll prefer that over HttpContextAccessor. This allows people to potentially customize what it means to get and set a HttpContext.
@twsouthwick
Copy link
Member Author

@joperezr can you approve this? sharepoint needs to be able to use a custom IHttpContextAccessor

using Microsoft.Extensions.Options;

namespace System.Web.Hosting;

internal sealed class HostingEnvironmentAccessor
{
private static readonly HttpContextAccessor _defaultHttpContextAccessor = new();
Copy link
Member

Choose a reason for hiding this comment

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

I'm not very familiar with this code, but how often is _accessor or _current null? If not too often, should we make this be lazily evaluated instead so as to only initialize it if needed? (might also not matter if instantiating an HttpContextAccessor is cheap)

Copy link
Member Author

Choose a reason for hiding this comment

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

how often is _accessor or _current null

good point - most of the time they are not null. It will only be if someone is running outside of a asp.net core host. I've changed it to just be a back up.

Copy link
Member

@joperezr joperezr left a comment

Choose a reason for hiding this comment

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

Added a tiny question, but looks good otherwise.

@twsouthwick twsouthwick enabled auto-merge (squash) January 17, 2024 23:54
@twsouthwick twsouthwick merged commit 21b925f into main Jan 18, 2024
@twsouthwick twsouthwick deleted the use_ihttpcontextaccessor_if_available branch January 18, 2024 00:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants