Created option for RequireHttpsAttribute.Permanent #5026
Conversation
|
Hi @iscifoni, I'm your friendly neighborhood .NET Foundation Pull Request Bot (You can call me DNFBOT). Thanks for your contribution! The agreement was validated by .NET Foundation and real humans are currently evaluating your PR. TTYL, DNFBOT; |
| /// </summary> | ||
| public RequireHttpsAttribute() | ||
| { | ||
| this.Permanent = null; |
There was a problem hiding this comment.
This assignment is redundant. Why even have these constructors (including the one with the parameter)? When I added the Permanent property, I left them out intentionally.
There was a problem hiding this comment.
Effectively the assignment is redundant, and also the contructor, I have use it to do debug.
I remove it.
- RequireHttpsAttribute modify permanent
| public bool Permanent { | ||
| get | ||
| { | ||
| return _permanent ?? false; |
There was a problem hiding this comment.
when i use it to mark a controller or action an error occurs when specify a value , is not a valid attribute parameter type because is nullable value.
There was a problem hiding this comment.
I meant that you could collapse the getter and setter:
public bool Permanent
{
get { return _permanent ?? false; }
set { _permanent = value; }
}😄
| /// Gets or sets the default value for Permanent property of <see cref="RequireHttpsAttribute"/>. | ||
| /// </summary> | ||
| public bool RequireHttpsPermanent { get; set; } | ||
|
|
|
Left some comments on minor style nits, not sure how strongly the team feels about those 😝 |
|
Ok thanks for your observations 😄 |
|
Awesome ❤️ |
| @@ -140,5 +140,10 @@ public int MaxModelValidationErrors | |||
| /// is used. If not set the port won't be specified in the secured URL e.g. https://localhost/path. | |||
| /// </summary> | |||
| public int? SslPort { get; set; } | |||
|
|
|||
| /// <summary> | |||
| /// Gets or sets the default value for Permanent property of <see cref="RequireHttpsAttribute"/>. | |||
There was a problem hiding this comment.
Change to: ... for *the* Permanent...
|
@iscifoni thanks for sending the PR! And @khellang thanks for doing the code review! I've added a few other small comments, and I assigned to @sebastienros for final review and to merge. |
| @@ -82,6 +88,9 @@ protected virtual void HandleNonHttpsRequest(AuthorizationFilterContext filterCo | |||
| host = new HostString(host.Host); | |||
| } | |||
|
|
|||
| //i use MvcOption.requireHttpsPermanent value if Permanent parameter is null | |||
| _permanent = _permanent ?? optionsAccessor.Value.RequireHttpsPermanent; | |||
There was a problem hiding this comment.
Just thought about one thing... Should this be assigned back to the field, or should we keep falling back to optionsAccessor.Value.RequireHttpsPermanent for each call? Can the option change?
There was a problem hiding this comment.
i think the options don't change, i can use a new field but it will have the value from optionsAccessor.Value.RequireHttpsPermanent for each call...
There was a problem hiding this comment.
I think there's functionality for reloading configuration (i.e. when a config file changes). Whether that also propagates into options (bound to configuration), I'm not sure about.
Also, changing this exact value at runtime makes little sense to me. I just thought it was worth asking 😄
Anyway, I don't think a new field is necessary. A variable would probably do it.
There was a problem hiding this comment.
I would indeed just use a variable to store the result of the computation. I didn't notice that it sets the property - that's not good.
There was a problem hiding this comment.
(And yes, there is such a thing as reloadable configuration and options, but MVC doesn't use it, so that isn't an actual concern here.)
There was a problem hiding this comment.
Ok i will use a variabile instead of property
|
|
|
@iscifoni thanks for the PR! |
Created an option for RequireHttpsAttribute.Permanent
#4650