Add support for static permission definitions with *.permissions.yml

Really like that.

There are modules, especially in contrib which provide permissions based upon config entities, like node types, so we have
to provide a way to register dynamic ones, similar to routing.

There will be issues with some permission descriptions in core which are dynamic, depending on the access of the user. Yes, I do think this is total BS and not doing that any longer would be a win, as it is fragil by nature.

Decide if that's still an allowable change for D8. If so, do it. If not, decide if system_permissions() is an acceptable place in which to define those permissions.

If we care about this API change so late we can always support both but given that the change itself will not be hard for module developers and people learn the D8 APIs will think that the yml file is way more consistent.

procrastination!

Thought that @timplunkett already opened an issue for this quite some time ago.

Some things to consider:

- A fair amount of permissions are dynamic... so we would also need object callbacks similar to local tasks and so on. Making that part more complex than it is now...
- It would also mean that yet another system needs custom localization and integration with potx to identify translatable strings.

Since this isn't a runtime hook, I wouldn't rule out adding YAML and keeping hook_permission() for bc. Although the dynamic permissions is tricky.

Changing this from 'beta deadline' to 'minor verson target'. If there's a good reason to not put it into 8.x when it comes down to it, we can bump it to 9.x at that point.

I am not sure at which level we do want to sort the permissions by module name.

There are a couple of places where the module handler is used to dermine the list of available permissions.

A bit more work.

1. +++ b/core/modules/simpletest/src/WebTestBase.php
   @@ -647,7 +647,9 @@ protected function drupalCreateRole(array $permissions, $rid = NULL, $name = NUL
   +    /** @var \Drupal\user\PermissionManagerInterface $permission_manager */
   +    $permission_manager =  \Drupal::service('user.permissions');
   
   +++ b/core/modules/system/src/Tests/Menu/BreadcrumbTest.php
   @@ -32,7 +32,7 @@ class BreadcrumbTest extends MenuTestBase {
   -    $perms = array_keys(\Drupal::moduleHandler()->invokeAll('permission'));
   +    $perms = array_keys(\Drupal::service('user.permissions')->getPermissions());
   

   Why two lines here and one elsewhere?

2. +++ b/core/modules/simpletest/src/WebTestBase.php
   @@ -647,7 +647,9 @@ protected function drupalCreateRole(array $permissions, $rid = NULL, $name = NUL
   +    $permission_manager =  \Drupal::service('user.permissions');
   

   Two spaces after the =

3. +++ b/core/modules/user/src/Form/UserPermissionsForm.php
   @@ -19,11 +20,11 @@
   -   * The module handler.
   +   * The permission manager.
       *
   -   * @var \Drupal\Core\Extension\ModuleHandlerInterface
   +   * @var \Drupal\user\PermissionManagerInterface
       */
   -  protected $moduleHandler;
   +  protected $permissionManager;
   

   Yay! Inject the classes with what we actually need.

   Off-topic thought: For non-alter hooks that we still invoke, why not wrap them in a service so we can change them out later?

4. +++ b/core/modules/views/views.permissions.yml
   @@ -0,0 +1,4 @@
   +'access all views':
   

   Keying by a string with quotes looks weird, but nothing wrong with it.

5. +++ b/core/modules/views/views.permissions.yml
   @@ -0,0 +1,4 @@
   +  'title': 'Bypass views access control'
   +  'description': 'Bypass access control when accessing views.'
   +  'restrict access': TRUE
   

   Do we want to keep the quotes around these keys? They're optional for the first two, and we could always change it to restrict_access if we wanted.

Re: your point about quoted YAML keys. I have almost never seen an example of YAML with spaces in the keys*, and I've also wondered why the Drupalism of spaces in permission machine names has persisted so long. I wholeheartedly vote for using underscores where possible (so restrict_access), and would also vote to move to better machine names (no spaces) for permissions, but that would probably need to be a separate discussion/issue.

*One exception: When a key would contain special YAML characters, like '>>':'value', but that, too, is rare and feels wrong as well.

Good idea!
Do you think that we could convert the permissions with a space to "_" and with "_" to "__" ? This could be even done in a temporary layer to support both.

Are there permissions with underscores? I don't ever remember seeing any, but I guess they exist. Could we just leave the ones that have underscores alone?

You can end up with permissions in node type specific permissions for example, which often include _ or another other machine-name based permissions.

Yeah it also happens really easy if you mention the module name in there, just as example.

Off-topic thought: For non-alter hooks that we still invoke, why not wrap them in a service so we can change them out later?

Sounds like a good idea, but are there actually a lot still?

Keying by a string with quotes looks weird, but nothing wrong with it.

Do we want to keep the quotes around these keys? They're optional for the first two, and we could always change it to restrict_access if we wanted.

It looks really wrong to have quotes around just some of them. I would love to change the permissions to machine only in a follow up.

I just added #2309869: Use proper machine names for permissions, and set it to postponed on this issue until the conversion has been completed.

Agreed the permission names should happen in a follow-up.

+++ b/core/modules/views/views.permissions.yml
@@ -0,0 +1,4 @@
+  'title': 'Bypass views access control'
+  'description': 'Bypass access control when accessing views.'
+  'restrict access': TRUE

+++ b/core/modules/views_ui/views_ui.permissions.yml
@@ -0,0 +1,4 @@
+  'title': 'Administer views'
+  'description': 'Access the views administration pages.'
+  'restrict access': TRUE

I still think we shouldn't quote these. Supporting both restrict access for old-style hooks and restrict_access for YAML seems fine

This just removes the quotes, any additional work should be discussed on top of this.

1. +++ b/core/modules/user/src/PermissionManager.php
   @@ -0,0 +1,118 @@
   +    $this->ymlDiscovery = new YamlDiscovery('permissions', $this->moduleHandler->getModuleDirectories());
   

   One of the reasons we wanted this is so that Drupal\Core can provide an entity type and the relevant permissions, without needing a module. Can we still accomplish this, maybe by using container.namespaces?

2. +++ b/core/modules/user/src/PermissionManager.php
   @@ -0,0 +1,118 @@
   +  protected function buildPermissions() {
   ...
   +    $module_info = system_rebuild_module_data();
   +    foreach (array_keys($this->moduleHandler->getModuleList()) as $module) {
   ...
   +    foreach ($this->ymlDiscovery->findAll() as $provider => $permissions) {
   ...
   +    foreach ($this->moduleHandler->getImplementations('permission') as $provider) {
   ...
   +    uasort($all_permissions, function (array $permission_a, array $permission_b) use ($modules) {
   

   It'd be really nice to split up this method, and provide a unit test for it!

Worked on some unit tests.

That is *exactly* what I envisioned, AND it uses vfsStream! Very cool.

I think the follow-up about quoted strings is fine.

Added a change record: https://www.drupal.org/node/2311427

Thanks for the change record!

Middle-of-night thought: Do we have a sense of the performance implications of this, were we to move most permission definitions to YAML? Presumably the discovery is cached somewhere? And caching nonwithstanding, will the admin permissions page crash and burn like admin/config/development/testing currently does on first visit if you don't have approximately one bajillion memory? (The page is already touch-and-go in D7 and D6 with a lot of contrib).

Also I don't see any mention of translations on the issue after @Berdir's comment, which seemed to suggest it'd be more complicated than simply StringTranslationTrait and a passed-in translation. Am I missing something?

Oh, when adding change records, remember to add a reference to the issue so it doesn't get lost forever and so that it appears in the sidebar. :) I also have no idea how you keep filing these with a blank project which is a required field... Anyway I've updated https://www.drupal.org/node/2311427 with the reference:
https://www.drupal.org/node/2311427/revisions/view/7479921/7480951

Performance: AFAIK, the hook is only used on the permission UI page and simpletest uses it to validate permissions when you create users (where we until recently had a cache and removed it, but I don't think that's a problem). At runtime, permissions are just strings, nobody cares if they are defined or not. So it's not much of a problem. For the UI, the only problem is count($roles) * count($permissions) => lots of checkboxes, it doesn't matter much if they're defined in files or not. We're talking abbout max 1 file per module, while we often have dozens of tests in modules.

Translations: The main problem is discovering translatable strings, which is something that potx/localize.drupal.org need to solve, it's not something that core can provide.

Middle-of-night thought: Do we have a sense of the performance implications of this, were we to move most permission definitions to YAML? Presumably the discovery is cached somewhere? And caching nonwithstanding, will the admin permissions page crash and burn like admin/config/development/testing currently does on first visit if you don't have approximately one bajillion memory? (The page is already touch-and-go in D7 and D6 with a lot of contrib).

There is one general advantage as described by catch in #2295469-5: Add support for static permission definitions with *.permissions.yml hook_permission()/yml parsing is just executed for the admin page
so there is a small, neglectable, boost for any other page as the module files will be smaller. HEAD just does a invokeAll('permission') so this patch also does not introduce caching of them.
I don't think though it is worth, at least for now.

Also I don't see any mention of translations on the issue after @Berdir's comment, which seemed to suggest it'd be more complicated than simply StringTranslationTrait and a passed-in translation. Am I missing something?

I cannot proove this but for core services we always inject the string translation as well, and just use the trait for t()/formatPlural(). Yes potx would need another patch, but they are really used to scanning
all YML files out there.

Oh, when adding change records, remember to add a reference to the issue so it doesn't get lost forever and so that it appears in the sidebar. :

Someone has to explain me that: All I do is to copy&paste the title of the issue, and do some dishwashing until the autocomplete is done and then hit save.

What's the use of PermissionsManager::$permissions if getPermissions() always rebuilds the full list ? As is, this property acts as a static cache we write to but never read from ?

Also, I tried (but failed) to have us standardize on FooPluginManager for plugin managers. I lost that battle, so we currently have a lot of FooManagers that are plugin managers. PermissionsManager, in turn, is *not* a plugin manager, and more like ModuleHandler, so why not PermissionHandler ?

What's the use of PermissionsManager::$permissions if getPermissions() always rebuilds the full list ? As is, this property acts as a static cache we write to but never read from ?

Yeah when I start writing it, I thought caching would be required.

Also, I tried (but failed) to have us standardize on FooPluginManager for plugin managers. I lost that battle, so we currently have a lot of FooManagers that are plugin managers. PermissionsManager, in turn, is *not* a plugin manager, and more like ModuleHandler, so why not PermissionHandler ?

I don't care. We have all kind of names all over the place. As a non-native speaker handle and manage is kind of similar anyway.

Why should it work that easy ...

+++ b/core/modules/user/src/Form/UserPermissionsForm.php
@@ -19,11 +20,11 @@
+   * The permission manager.
...
+   * @var \Drupal\user\PermissionManagerInterface
...
+  protected $permissionManager;

@@ -35,13 +36,13 @@ class UserPermissionsForm extends FormBase {
+   * @param \Drupal\user\PermissionManagerInterface $permission_manager
+   *   The permission manager.

+++ b/core/modules/user/src/PermissionHandler.php
@@ -0,0 +1,159 @@
+class PermissionHandler implements PermissionManagerInterface {

+++ b/core/modules/user/src/PermissionManagerInterface.php
@@ -0,0 +1,47 @@
+interface PermissionManagerInterface {

O_o?!

@tim.plunkett
Nice, three attemps for that trivial change

@xjm
#2311935: Add support for .permissions.yml files

I am bored so here is just another one.

Thanks, that last patch looks good, and thanks for the translation extractor issue!

+1, thanks @dawehner!

Reroll

Small nitpick:

+++ b/core/modules/user/src/PermissionHandler.php
@@ -0,0 +1,159 @@
+  protected function buildPermissionYaml() {

Should be plural: buildPermissionsYaml()

I was asked to chime in for the localize.drupal.org effects. The issue summary does not make it clear if the YML file is the new suggested way of defining permissions or should be the only one for static permissions or not. The code both supports the hook and the YML file. Is the hook the long term (AKA Drupal 8 release) plan for defining any dynamic permissions? As for parsing the permission YML file, it is one more alongside the links.ymls, routing.yml, etc. #2311935: Add support for .permissions.yml files is a good start. @herom has some good ideas in #2088371-36: YAML discovery incompatible with translations to make this work with contrib's new YML files as well. Either way, we can continue here, and figure out the localize.drupal.org stuff later, its not a big deal at this point :)

@Gábor Hojtsy
Well, we cannot get rid of dynamic permissions like permissions based upon node types, but I think the YML file should be the way to go.

Right, we do need dynamic permissions. The question was how. Would it be a plugin, referenced via a class file from the permissions YML (like tasks) or via a provider class like routes or via a hook like as is now. What's the eventual plan? :)

Reroll after tim's mega patch.

Ah yeah, one idea I do have in mind is to adapt the routes_callback pattern ([#2122201]) for permissions.

Rerolled, just a context change in core/modules/user/user.services.yml.

one idea I do have in mind is to adapt the routes_callback pattern

+1 for that in a follow-up

1. +++ b/core/modules/user/src/Form/UserPermissionsForm.php
   @@ -36,13 +37,13 @@ class UserPermissionsForm extends FormBase {
   -  public function __construct(ModuleHandlerInterface $module_handler, RoleStorageInterface $role_storage) {
   

   Need to also remove the use ModuleHandlerInterface from this file.

2. +++ b/core/modules/user/src/PermissionHandler.php
   @@ -0,0 +1,159 @@
   +    $this->ymlDiscovery = new YamlDiscovery('permissions', $this->moduleHandler->getModuleDirectories());
   

   Missing a class property for this and let's call it yamlDiscovery() to be consistent.

3. +++ b/core/modules/user/src/PermissionHandler.php
   @@ -0,0 +1,159 @@
   +    $all_permissions = $this->sortPermissionsByProviderName($all_permissions);
   +
   +    return $all_permissions;
   

   Could just return the result here.

4. +++ b/core/modules/user/src/PermissionHandler.php
   @@ -0,0 +1,159 @@
   +  /**
   +   * Returns all module names.
   +   *
   +   * @return array
   +   */
   +  protected function getModuleNames() {
   +    $modules = array();
   +    $module_info = $this->systemRebuildModuleData();
   +    foreach (array_keys($this->moduleHandler->getModuleList()) as $module) {
   +      $modules[$module] = $module_info[$module]->info['name'];
   +    }
   +    asort($modules);
   +    return $modules;
   +  }
   +
   

   Very minor nit but can this be moved to the bottom of the class - it has very little to do with the functionality. In some ways I don't get why we have this method at all - like why not just inline with the sortPermissions method?

5. +++ b/core/modules/user/tests/src/PermissionHandlerTest.php
   @@ -0,0 +1,225 @@
   +    if (!isset($this->moduleHandler)) {
   +      $this->moduleHandler = $this->getMock('Drupal\Core\Extension\ModuleHandlerInterface');
   +      $this->moduleHandler->expects($this->once())
   +        ->method('getModuleDirectories')
   +        ->willReturn(array());
   +    }
   

   How is it possible for $this->moduleHandler to be set at this point?

Nice use of file system mocking!

Thank you for the review.

Need to also remove the use ModuleHandlerInterface from this file.

It never had a property and the only usage was removed from the patch itself.

Missing a class property for this and let's call it yamlDiscovery() to be consistent.

Good point.

Very minor nit but can this be moved to the bottom of the class - it has very little to do with the functionality. In some ways I don't get why we have this method at all - like why not just inline with the sortPermissions method?

Moved the function. Well inlining a one-time method is a bad idea if you ask me. This makes it simply harder to understand, potentially harder to fix bugs
and especially there is no reason for optimizing the performance here.

How is it possible for $this->moduleHandler to be set at this point?

No idea what I was thinking.

re #53.1 I meant core/modules/user/src/Form/UserPermissionsForm.php now has an unused use for ModuleHandlerInterface... ie.

use Drupal\Core\Extension\ModuleHandlerInterface;

can be removed.

OH

+++ b/core/modules/user/tests/src/PermissionHandlerTest.php
@@ -48,12 +48,11 @@ class PermissionHandlerTest extends UnitTestCase {
+    $this->moduleHandler = $this->getMock(\Drupal\Core\Extension\ModuleHandlerInterface::class);

I would have loved to use php 5.5 here.

Sorry for all that comments.

Fixed the silly test failure.

meh.

--3way helped

+++ b/core/modules/user/src/Form/UserPermissionsForm.php
@@ -121,55 +113,59 @@ public function buildForm(array $form, FormStateInterface $form_state) {
-          if (!$hide_descriptions) {
-            $user_permission_description = $perm_item['description'];
-            // Append warning message.
-            if (!empty($perm_item['warning'])) {
-              $user_permission_description .= ' <em class="permission-warning">' . $perm_item['warning'] . '</em>';
-            }
-          }

Now we can just hide it https://www.drupal.org/node/2324025

1. +++ b/core/modules/user/src/PermissionHandler.php
   @@ -0,0 +1,176 @@
   +    // @todo Replace this with container.namespaces or something similar.
   

   We should add an explanation to the @todo that illustrates why this needs to be done.

2. +++ b/core/modules/user/src/PermissionHandler.php
   @@ -0,0 +1,176 @@
   +    return $this->sortPermissionsByProviderName($all_permissions);
   

   It's probably only a minor performance tweak, but shouldn't we sort permissions only when we need to?

3. +++ b/core/modules/user/src/PermissionHandler.php
   @@ -0,0 +1,176 @@
   +   * @return array
   

   @return array is never acceptable documentation. Instead, document the array contents. In this case @return array[] and the description should contain the actual structure or refer to documentation elsewhere that does.

4. +++ b/core/modules/user/src/PermissionHandler.php
   @@ -0,0 +1,176 @@
   +   * @return array
   

   Same here.

5. +++ b/core/modules/user/src/PermissionHandler.php
   @@ -0,0 +1,176 @@
   +   * @return array
   

   Same here.

6. +++ b/core/modules/user/src/PermissionHandler.php
   @@ -0,0 +1,176 @@
   +   * @return array
   

   @return string[], and document that keys are module machine names.

We also need an @covers ::__construct annotation in PermissionHandlerTest.

Now we can just hide it https://www.drupal.org/node/2324025

Isn't that out of scope of this issue?

It's probably only a minor performance tweak, but shouldn't we sort permissions only when we need to?

I would rather let the code be readable, instead of having better performance on a non existing edge case.

Fixed.

Two nitpicks:

1. +++ b/core/modules/user/src/PermissionHandlerInterface.php
   @@ -0,0 +1,47 @@
   +   *     permission on the permission administration page. This warning overrides
   

   80 chars

2. +++ b/core/modules/user/src/PermissionHandlerInterface.php
   @@ -0,0 +1,47 @@
   +   *     This should rarely be used, since it is important for all permissions to
   

   same here

- edit - note, looks good otherwise.

Cool, here is a nitpickfix.

I think all reviews and nitpicks have been addressed.

I actually think this is a good change to make, despite how late in the cycle. My rationale is from teaching module development for 5+ years at Lullabot. We would always use hook_perm (and later hook_permission) as the intro hook for new developers to get the lay of the land of how things are done in Drupal because it's very simple to wrap your head around. In Drupal 8 though, things these days are very rarely done with hooks, and when they are, they're generally done for event-style things, such as saving a node or running cron, not for "info-style" things which have almost exclusively moved elsewhere (and mostly to YAML). So by moving static permissions to YAML, it not only brings permissions more inline with what D8 does, but we also now regain a "simple" example to teach people before we thrust them into the thorn-covered land of routing.yml. ;)

dawehner walked me through the patch in IRC and I didn't see anything to really take issue with. The patch is very benign in that it leaves hook_permission() in place; I am concerned about what "phase 2" of switching hook_permission() to something else might entail and whether it'll be consistent with others, but in the worst case if we don't get to that we can always leave hook_permission() is as BC.

There were also concerns raised in IRC about whether or not moving to YAML, with its tempting human-editableness, could present a security risk but in the end these files are only loaded one place in core, which is on the permission form itself. Everywhere else is simple string matching, so we think we're covered there.

We also need the rest of the permissions converted. I talked to dawehner and he thinks he can script all of them in one go, which would be great. I'm about full-up on 5,000 sub-issue meta issues, esp. when I'm the lone committer this week. ;)

Therefore!

Committed and pushed to 8.x. Thanks!

Here is an issue to convert all static yml files: #2328411: Convert all permissions to yml files and permission callbacks

Really happy to see this in! I definitely shouldn't be first in the commit message though :( filed https://github.com/dreditor/dreditor/issues/226 after some testing.

Small point, Looks like some stray backticks crept in!

Oh

Committed c1dd108 and pushed to 8.0.x. Thanks!

This patch makes calling module_invoke($module, 'permissions') useless. So I created followup #2342229: Add permissionByModule to user.permissions service