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

Fix for issue Newly created sub-folder not shown if the parent folder… #3976

Merged
merged 1 commit into from
Sep 27, 2020

Conversation

kmuralidaran
Copy link
Contributor

… name starts with "0"

Fixes #3975

Summary

Trimming the prefix "0" from folder name creates a folder with different name compared to the user provided name and the subfolder is also not created in the user provided folder name.

Removing the code which trims the prefix "0" from folder name addresses this issue

@dnfadmin
Copy link

dnfadmin commented Aug 12, 2020

CLA assistant check
All CLA requirements met.

Copy link
Contributor

@valadas valadas left a comment

Choose a reason for hiding this comment

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

I am worried there was a reason for this replacement. \0 means null in a string ( see https://docs.microsoft.com/en-us/dotnet/csharp/programming-guide/strings/ ) and I believe this is there to prevent the first 0 being next to a path separator \ and thus being interpreted as a null. For instance tryting to create a folder named 0_MyFile.txt in the c:\websites\dnn\portals\1\textFiles\ folder, it would become c:\websites\dnn\portals\1\textFilesNULL_MyFile.txt.

Now stripping the 0 altogether is probably not the right solution, I wonder if we replace it with "\0" if that would work to escape the escape sequence and that would be interpreted properly on what consumes it or if maybe this hits a bump elsewhere in the same case as what we have on the old line 233...

At any rate, I think this change needs a good set of testing.

@mitchelsellers
Copy link
Contributor

I second this concern

@mitchelsellers
Copy link
Contributor

This item needs more review before merging, to validate usage. It appears to be only called by the file manager, but possibly could have a check done at a later time to validate that it isn't an invalid path.

BUT, the true history of WHY this was done predates GitHub so we need to further determine why.

Also - Is this used in the new file manager? if not, lets NOT change this.

@donker
Copy link
Contributor

donker commented Aug 20, 2020

The code checks if the path contains a backslash. If it does then it checks whether it starts with "0\" (note TWO backslahses). In which case it gets cut off. Finally it checks if it starts with 0 in which case it gets cut off. It is puzzling why this is here (maybe @cnurse knows). Unfortunately the naming of the mehod or the documentation provide no clues. Very frustrating. The issue is valid though, so my vote goes to doing one round of research and then if it isn't found, we mark it as deprecated and clean up this code.

Going through various installations I cannot find a single record that has this pattern. Maybe others can do the same. SELECT * FROM dbo.Folders WHERE FolderPath LIKE '0%'. Then check those records. It must give us a hint. Maybe a specific folder provider uses this for something.

@sleupold
Copy link
Contributor

I can't remember ever having a DNN website (myself or any client), where a folder started with "0".
The only idea would be the default folder for the first website, which always starts with "0" inside the website directory, but this should not be included in folder path.

@mitchelsellers
Copy link
Contributor

Funny enough...DNN will create folders that start with a Zero.

Look under the /Users/ folder for an example. So it seems that code bypasses this

@sleupold
Copy link
Contributor

@mitchelsellers yes, but not directly in the portal folder, i.e. the path starts with "users/0%"

@daguiler
Copy link
Contributor

daguiler commented Sep 4, 2020

Hi guys.
I wonder what's so special about \0.
Yes, it's the null char, but there are dozens of other special characters that might produce similar issues, like \r, \n, \a, \b, \f, \t, etc., etc. Yet, we can create folders starting with these letters normally.

@bdukes
Copy link
Contributor

bdukes commented Sep 4, 2020

Has anyone looked into whether the new asset/resource manager uses this code?

@valadas
Copy link
Contributor

valadas commented Sep 5, 2020

Hi guys.
I wonder what's so special about \0.
Yes, it's the null char, but there are dozens of other special characters that might produce similar issues, like \r, \n, \a, \b, \f, \t, etc., etc. Yet, we can create folders starting with these letters normally.

True, true... Yeah, I must admit I am a bit clueless about the purpose here...

@valadas
Copy link
Contributor

valadas commented Sep 5, 2020

Has anyone looked into whether the new asset/resource manager uses this code?

It does not directly but it calls AssetManager.Instance.CreateFolder which in turn calls this method. So I would say yeah.

@kmuralidaran
Copy link
Contributor Author

@valadas Do we have any use case to validate the side effects of this fix?

@valadas
Copy link
Contributor

valadas commented Sep 15, 2020

@kmuralidaran Well, I am really not sure, my guess was that it was to prevent \0 which is a null escape character, but maybe it's not since other replacement characters would also be a problem here, so really not sure what is special with 0 and why this was put in place...

@bdukes bdukes added this to the Future: Patch milestone Sep 24, 2020
Copy link
Contributor

@bdukes bdukes left a comment

Choose a reason for hiding this comment

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

I support this change. We allow folders that start with zero via other methods, and we have no specific scenario that is adversely affected by this change.

Copy link
Contributor

@mitchelsellers mitchelsellers left a comment

Choose a reason for hiding this comment

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

I'm with Brian on this, lets approve!

@mitchelsellers
Copy link
Contributor

@valadas You good with this?

@valadas
Copy link
Contributor

valadas commented Sep 27, 2020

@valadas You good with this?

Yeah, I mean it looks it this must be a leftover from god knows what... I am approving.

Copy link
Contributor

@valadas valadas left a comment

Choose a reason for hiding this comment

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

Looks good to me

@valadas valadas merged commit 9ebc627 into dnnsoftware:develop Sep 27, 2020
@kmuralidaran kmuralidaran deleted the bugfix/DNN-40416 branch October 1, 2020 12:30
@valadas valadas modified the milestones: 9.7.3, 9.8.0 Oct 7, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Newly created sub-folder not shown if the parent folder name starts with "0"
8 participants