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

Add IClientIpAddressAccessor interface for accessing client IP Address #13894

Merged
merged 2 commits into from
Jul 12, 2023

Conversation

MikeAlhayek
Copy link
Member

@MikeAlhayek MikeAlhayek requested a review from jtkech June 21, 2023 20:07
@jtkech
Copy link
Member

jtkech commented Jun 21, 2023

Looks good but IOrchardHelper is most commonly used to provide Razor extensions, particularly for decoupled cms, never seen injected elsewhere, so maybe better to provide an HttpContext extension.

@MikeAlhayek
Copy link
Member Author

@jtkech I understand. IOrchardHelper is injected when AddOrchardCore() extension is built. So it is a service that would always be available by default. I chose IOrchardHelper because one time I have a convo with @sebastienros and he suggested against adding extension for HttpContext directly. Since this is an extension of OC it maybe best if extend OC classes instead of .NET.

@jtkech
Copy link
Member

jtkech commented Jun 21, 2023

Okay but not sure when more related to a given class and to not have too many undifferentiated IOrchardHelper extensions, personally I prefer httpContext.GetClientIpAddress() than orchardHelper.GetClientIpAddress(), but no problem if @sebastienros prefers.

@MikeAlhayek
Copy link
Member Author

MikeAlhayek commented Jun 21, 2023

I personally also find httpContext.GetClientIpAddress() more appealing than orchardHelper.GetClientIpAddress(). @sebastienros can you chime in here to know which route is best for OC?

@sebastienros
Copy link
Member

My suggestion would be to have a custom service abstraction as an interface in Infrastructure.Abstractions. This would also allow for mocking in tests. Mike suggests IClientIpAddressAccessor.GetIpAddressAsync()

@MikeAlhayek
Copy link
Member Author

I created an interface IClientIpAddressAccessor as per @sebastienros recommendation.

@MikeAlhayek MikeAlhayek changed the title Add GetClientIpAddress() helper Add IClientIpAddressAccessor interface for accessing client IP Address Jul 12, 2023
@MikeAlhayek MikeAlhayek changed the title Add IClientIpAddressAccessor interface for accessing client IP Address Add IClientIpAddressAccessor interface for accessing client IP Address Jul 12, 2023
@MikeAlhayek MikeAlhayek merged commit 424a9ae into OrchardCMS:main Jul 12, 2023
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.

4 participants