Skip to content

check for PhysicalFileProvider - #47934

Closed
stefannikolei wants to merge 1 commit into
dotnet:mainfrom
stefannikolei:checkPhysicalFileProvider
Closed

check for PhysicalFileProvider#47934
stefannikolei wants to merge 1 commit into
dotnet:mainfrom
stefannikolei:checkPhysicalFileProvider

Conversation

@stefannikolei

Copy link
Copy Markdown
Contributor

* check that it is not a subclass of PhysicalFileProvider
@ghost

ghost commented Feb 5, 2021

Copy link
Copy Markdown

Tagging subscribers to this area: @maryamariyan
See info in area-owners.md if you want to be subscribed.

Issue Details
Author: stefannikolei
Assignees: -
Labels:

area-Extensions-Configuration

Milestone: -

@stefannikolei
stefannikolei marked this pull request as ready for review February 8, 2021 20:05
@maryamariyan

maryamariyan commented Feb 18, 2021

Copy link
Copy Markdown
Contributor

@stefannikolei thanks for the PR, could you also please add a test for this?

Base automatically changed from master to main March 1, 2021 09:07

@maryamariyan maryamariyan left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The PR change seems fine, @stefannikolei could you please also add a tests for this fix?

@maryamariyan

maryamariyan commented Mar 18, 2021

Copy link
Copy Markdown
Contributor

@mcm-ham (who reported the issue) if you have a small repro code that would help with preparing a test case here.

{
if (fileInfo.PhysicalPath != null)
if (fileInfo.PhysicalPath != null
&& provider is PhysicalFileProvider && !provider.GetType().IsSubclassOf(typeof(PhysicalFileProvider)))

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Couldn't this just be

Suggested change
&& provider is PhysicalFileProvider && !provider.GetType().IsSubclassOf(typeof(PhysicalFileProvider)))
&& provider?.GetType() == typeof(PhysicalFileProvider))

Also - would be great to add a comment here why we are doing this.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Can we do this check once in the constructor

@safern

safern commented Apr 22, 2021

Copy link
Copy Markdown
Member

@stefannikolei thanks for your PR, do you plan on addressing the feedback and adding a test case for this?

@stefannikolei

stefannikolei commented Apr 23, 2021

Copy link
Copy Markdown
Contributor Author

thanks for your PR, do you plan on addressing the feedback and adding a test case for this?
@safern

Sorry missed the feedback. Gonna look at it.
Was waiting for @mcm-ham to hopefully deliver some repro code to make the test easier.

@maryamariyan

Copy link
Copy Markdown
Contributor

Closing this PR. @stefannikolei this is a good change, you could create a new PR if you plan on addressing the feedback

@karelz karelz added this to the 6.0.0 milestone May 20, 2021
@ghost ghost locked as resolved and limited conversation to collaborators Jun 19, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Json Configuration custom IFileProvider stopped working for .NET 5.0

6 participants