Add permission support for menu items #17263
Conversation
…CMS/OrchardCore into ma/admin-menu-permission
| { | ||
| ArgumentException.ThrowIfNullOrEmpty(name); | ||
|
|
||
| if (_permissions is null) |
There was a problem hiding this comment.
What about using the dictionary if it's initialized, and just iterating the providers if not? This way the first match will return the permission.
There was a problem hiding this comment.
What if we don't load the list first, that will cause enumeration for each name lookup which will defeat the purpose.
| return _permissions.Values; | ||
| } | ||
|
|
||
| private async Task LoadPermissionsAsync() |
There was a problem hiding this comment.
Not sure there is a reason to create this. GetPermissionsAsync could do the same directly. Even if you don't need the result.
There was a problem hiding this comment.
I actually did you how you described it. But I find it more readable this way.
...OrchardCore.Infrastructure.Abstractions/Security/Permissions/PermissionProviderExtensions.cs
Show resolved
Hide resolved
src/OrchardCore.Themes/TheTheme/Views/MenuItemLink-ContentMenuItem.cshtml
Outdated
Show resolved
Hide resolved
| return null; | ||
| } | ||
|
|
||
| public async ValueTask<IEnumerable<Permission>> GetPermissionsAsync() |
There was a problem hiding this comment.
If you return IReadOnlyList<Permission> all the ToArray() can be removed. And the result can be arrays or lists. Same for ByNames.
There was a problem hiding this comment.
The .ToArray() are needed convert IEnumerable to IList I don't think there is a benefit on using IReadOnlyList unless I am missed something. I did some cleanup now to use the FindByNamesAsync extension in more places.
Fix #12748