Add HttpContext to IBuildShapeContext #18117
Conversation
|
I'm a bit concerned about the impact of this change. It introduces overhead to all requests, even though only a small subset of drivers actually require access to the HTTP context. For example, public content which typically doesn't need user-specific data will now always access the |
|
I remember this had been suggested long time ago, I need to find the related issue |
|
After reconsidering, I think adding the HTTP context to |
|
Do you mean this #1058? |
|
Exactly |
| { | ||
| _handlers = handlers; | ||
| _contentDefinitionManager = contentDefinitionManager; | ||
| _shapeFactory = shapeFactory; | ||
| _layoutAccessor = layoutAccessor; | ||
| _logger = logger; | ||
| _httpContext = httpContextAccessor.HttpContext; |
There was a problem hiding this comment.
Are we sure this can be done in the constructor, once? Is DefaultContentDefinitionDisplayManager really scoped?
There was a problem hiding this comment.
Yes it's
|
I think this is worth being taken. But I would suggest to also include some usage examples, even if it makes the PR bigger. That can show the benefit of the change. |
|
This will be HUGE :) I never mind to do it if there's no objection |
|
It seems that this pull request didn't really move for quite a while. Is this something you'd like to revisit any time soon or should we close? Please comment if you'd like to pick it up. |
|
This should need some actual usages of the new property. |
Sure |
This will allow us to access the
ClaimsPrincipalin the drivers, which will simplify the user access from there.Fixes #1058