Trees: Prevent file system trees showing other file types that aren't supported for editing #20567
Conversation
|
|
||
| public override string[] GetFiles(string path) => FileSystem | ||
| .GetFiles(path) | ||
| .Where(file => file.EndsWith(".css")) |
There was a problem hiding this comment.
I wonder if people could add other types of stylesheets, such as .less and .scss, and expect those to show up. I know that requires an extra build step, but we are kind of disallowing that behavior now with this change.
The same could then be said for the Scripts tree: To be consistent, it, too, should only allow ".js" extensions. But people could have ".ts" extensions there as well.
This seems like a dangerous area to touch?
There was a problem hiding this comment.
We only support editing .css files in the backoffice, so we could allow other filetypes to be shown in the tree, but they'd be of no use other than showing that they are there. Clicking on any unsupported file type will just display the "stylesheet not found" screen.
So i think the question is, should we allow unsupported file types to be shown? If yes, then I'd argue the linked issue is working as intended.
We could do the same here for scripts, since we support editing both .js and .ts files in the backoffice.
There was a problem hiding this comment.
It may need to consider if developers have used StyleSheetTreeService directly in their own picker - or perhaps use data directly from management API?
Seems like a breaking change.
It can possible filter the tree items in the implemented stylesheet picker in Umbraco core?
or perhaps list files as it does at the moment, but a new overload method to filter files by extension?
There was a problem hiding this comment.
I think you might be right in saying this is a breaking change. This service has however was released very recently, so i highly doubt anyone actually used it yet - But anyways, it does seem like a breaking change and should be dealt with accordingly.
I would like some other eyes on this discussion, as i might be too "Junior" to take a decision here. @AndyButland
There was a problem hiding this comment.
Also, given the fact that it would only ever open .css files before, I don't see it as a breaking change:
There was a problem hiding this comment.
@nul800sebastiaan yes, it is correct it isn't handled in the workspace views, but I guess developers possible could have used data from management API in their own picker or something else (most likely not).
/umbraco/management/api/v1/tree/stylesheet/root?skip=0&take=1
So if it included .css.map files before, it may break if developers used this endpoint.
The risk is minimal and developers would most likely have created a custom endpoint anyway.
I don't have a concreate use-case, perhaps if someone made a implementation of the old Optimus package to pick less/scss files to bundle? 😅
I can't find the package anymore - I think it was @Jeavon who originally created that package back in days of v7 🙈
There was a problem hiding this comment.
Feels to me we should consider this a bug if we are currently showing files in the stylesheets tree that aren't stylesheets, and we can't edit them. I know you can argue it's behaviourally breaking- but in extremis any bug fix is - so there's always a balance. For me this feels currently wrong and we are OK to fix it by restricting such that only .css files are output. I'd expect anyone with a front-end build to not being using the backoffice for editing stylesheets.
For consistency I'd also suggest reviewing the "Scripts" endpoint and similarly make sure only .js files are available.
There was a problem hiding this comment.
Yes, I can certainly agree with that sentiment. As long as the behavior for the editors are the same, as in Stylesheets shows .css, Scripts show .js, and Partial Views show .cshtml (and possibly .razor?), we should be good. That is an opinionated approach, but so are the editors in the first place.
We could build a file editor in their place to edit any file, but it's all bound to change when runtime compilation disappears anyway.
There was a problem hiding this comment.
Just to be clear here @AndyButland @iOvergaard
Which filetypes would you like to be shown? I know you can edit .ts files as well as .js, so should i be allowing both to be shown or just .js?
Also for the partial views, are there others than .cshtml and .razor that needs to be shown? I'm unsure if there are other C# and HTML mixed filetypes
There was a problem hiding this comment.
I think scripts as .js, stylesheets as .css and partial views as .cshtml is enough. .razor is only for client-side I believe (you get some funny images if you Google "razor extension" by the way).
AndyButland
changed the title
.css
There was a problem hiding this comment.
Looks good @NillasKA so far. I answered a question and made a few suggestions having had a look over..
tests/Umbraco.Tests.Integration/ManagementApi/Services/Trees/FileSystemTreeServiceTestsBase.cs
Outdated
Show resolved
Hide resolved
src/Umbraco.Cms.Api.Management/Services/FileSystem/FileSystemTreeServiceBase.cs
Outdated
Show resolved
Hide resolved
tests/Umbraco.Tests.Integration/ManagementApi/Services/Trees/StyleSheetTreeServiceTests.cs
Outdated
Show resolved
Hide resolved
NillasKA
changed the title
.css
|
@AndyButland I've requested your review again I see that you have made another PR that might cause conflicts with this, i figured you can take point on how to handle this, unless you insist i do it |
There was a problem hiding this comment.
Works nicely @NillasKA and code all looks good. Thanks very much.
AndyButland
deleted the
v16/bugfix/show-only-css-files-in-stylesheet-tree
branch
AndyButland
changed the title
…nes (#20567) * Added check to only find .css files in FileSystemTreeServiceBase.cs * Marking GetFiles as virtual and overriding it in StyleSheetTreeService.cs to only find .css files * Redone tests to fit new format * Fix tests to use file extensions * Adding file extensions to all other relevant tests * Adding file filter to remaining trees * Adding tests to ensure invalid filetypes wont show * Encapulation and resolved minor warnings in tests. --------- Co-authored-by: Andy Butland <[email protected]>
Closes #20518
Description
Migrating from V13 to V17, created a .css.map file, that cannot be opened or edited from the backoffice.
The solution to this is to only show .css files in the backoffice tree, any other files will still exist on the filesystem. but wont be shown in the backoffice, since we only support editing of .css files.