Convert all permissions to yml files and permission callbacks

Awesome script, but it would ne nicer if the permission names (i.e. the keys in the YAML) would not be quoted. Maybe some sed-magic on top? :-)

@tstoeckler / #5: Keys with spaces need to be quoted to be valid YAML. Hopefully we can change that after this conversion is complete, over in #2309869: Use proper machine names for permissions.

Re #7: That's not correct. I just checked and the Symfony parser parser YAML with spaces in the key without quotes just fine. Now that you brought up, though, I do agree removing the qutotes would be fairly confusing due to the spaces. So let's just leave it as is.

http://yaml.org/spec/1.2/spec.html#id2788859, specifically the plain implicit keys. Huh, learn something new every day! I still think snake case would be most appropriate (and consistent with other parts of our APIs), but at least it's not explicitly required...

(Edit: though, to avoid incomplete parser implementations, it might still be best to quote the keys; does our YAML parser work with spaces in keys?).

Missed one more little quote :)

+view own unpublished content':

to:

+view own unpublished content:

Someone added a link to any page permission in system_permission(), it seems. This patch should hopefully work.

Trying that again... don't know how it missed the permission in system.permissions.yml.

What still needs work, besides the issue summary; we don't need a change notice for this issue, correct?

Although it wasn't mentioned in the change record, there is a shorthand way to do this (for permissions that only have a title):

-administer actions:
-  title: 'Administer actions'
+administer actions: 'Administer actions'

Could we get that added to the change record? If this is a supported/recommended method of defining permissions, it seems it should be mentioned. It feels too informal for my liking, but I see how it's a nice way to do it.

+++ b/core/modules/node/node.module
@@ -676,44 +676,7 @@ function template_preprocess_node(&$variables) {
-    'access content overview' => array(
-      'title' => t('Access the Content overview page'),
-      'description' => t('Get an overview of <a href="!url">all content</a>.', array('!url' => \Drupal::url('system.admin_content'))),

+++ b/core/modules/node/node.permissions.yml
@@ -0,0 +1,23 @@
+access content overview:
+  title: 'Access the Content overview page'
+  description: 'Get an overview of all content.'

This seems like a bit of a cut corner here. We are now losing the link. This should stay in hook_permission if that's the case. #2329485: Allow permissions.yml files to declare 'permission_callbacks' for dynamic permissions can then implement that or something.

Also, RE #28: This was the case anyway, before this patch?

Okay, back to needs review—though we need to decide if we'll use the shorthand title syntax, or the regular title/description structured key syntax (and whether or not that should be added to the original change record)...

One callback needed to be fixed (see interdiff). Failing tests passed locally on current HEAD.

Just a minor nitpick:

+++ b/core/modules/views/views.permissions.yml
@@ -1,4 +1,4 @@
+  restrict access: TRUE

+++ b/core/modules/views_ui/views_ui.permissions.yml
@@ -1,4 +1,4 @@
+  restrict access: TRUE

s/TRUE/true

Still needs review—I picked up another s/TRUE/true in views_ui permissions too, thanks!

Let's remove their mentions in the docs too (grepped for '_permission(').

modules/filter/src/Entity/FilterFormat.php:213:      // filter_permission() and lastly filter_formats(), so its cache must be
modules/filter/src/Tests/FilterDefaultConfigTest.php:24:    // filter_permission() calls into url() to output a link in the description.
modules/node/tests/modules/node_access_test/node_access_test.module:47: * @see node_access_test_permission()
modules/node/tests/modules/node_access_test/node_access_test.module:75: * @see node_access_test_permission()

Also fixed a couple other docs instances, and added mention of $module.permissions.yml to core API docs.

But doesn't hook_permission() still work fine? The change records indicate that neither is really preferred, and make no mention of permissions_callbacks—is this just a matter of the change record being out of date?

hook_permission() is not deprecated as it is needed to support dynamic permissions.

From the 'Dynamic Permissions' section.

I see. In that case, shouldn't we mark the use of hook_permission as deprecated? Would that be done in a separate/follow-up issue?

Patch looks good. Only thing is whether we keep the hook_permission for now, deprecate. Then remove in a follow up? I think the maintainers like that pattern?

I would rtbc but I have too much skin in the game. Shall I open a follow-up to mark the old hook deprecated?

Since my changes didn't make it into the patch, I guess I can RTBC.

The patch looks great.

FYI #babywatch14 has come to an end today, so I will not be able to open that follow up (or do much else) for a few days... Glad to see this issue RTBC, and if someone else doesn't get the CR updated and follow-ups created by then, I'll jump back in and do it once I get some rest!

filed the followup issues:
#2338475: Remove hook_permission() and #2338487: Document and use the shorthand syntax in .permission.yml files..

oops! bringing back.

1. +++ b/core/modules/user/src/PermissionHandler.php
   @@ -25,7 +25,7 @@
   - * @see hook_permission()
   + * @see permissions.yml
   

   @see to a non existing file

2. +++ b/core/modules/simpletest/src/Tests/KernelTestBaseTest.php
   @@ -69,8 +70,8 @@ function testEnableModulesLoad() {
   -    $list = \Drupal::moduleHandler()->getImplementations('permission');
   -    $this->assertTrue(in_array($module, $list), "{$module}_permission() in \Drupal::moduleHandler()->getImplementations() found.");
   +    $list = \Drupal::moduleHandler()->getImplementations('query_efq_table_prefixing_test_alter');
   +    $this->assertTrue(in_array($module, $list), "{$module}_query_efq_table_prefixing_test_alter() in \Drupal::moduleHandler()->getImplementations() found.");
   

   Above here there is a corresponding assertion that is still checking hook_permission

3. +++ b/core/modules/system/system.module
   @@ -1118,7 +1077,13 @@ function system_get_module_admin_tasks($module, array $info) {
   +  $permissions = \Drupal::service('user.permissions')->getPermissions();
   +  $permission_modules = [];
   +  foreach ($permissions as $permission) {
   +    $permission_modules[$permission['provider']] = $permission['provider'];
   +  }
   +
   +  if (isset($permission_modules[$module])) {
   

   How about something like:

     $permissions = \Drupal::service('user.permissions')->getPermissions();
     $has_permissions = FALSE;
     foreach ($permissions as $permission) {
       if ($permission['provider'] == $module) {
         $has_permissions = TRUE;
         break;
       }
     }
   
     if ($has_permissions) {
   

   Or maybe something on the PermissionHandler to do this.

4. +++ b/core/modules/user/src/Plugin/views/access/Permission.php
   @@ -31,6 +34,41 @@ class Permission extends AccessPluginBase {
   +  public function __construct(array $configuration, $plugin_id, $plugin_definition, PermissionHandlerInterface $permission_handler) {
   +    $this->permissionHandler = $permission_handler;
   +  }
   

   Now you are injecting you can replace the \Drupal::service in this class.

5. user_permission_get_modules() has no usages and therefore should be removed. It usage we as removed by #1872876: Turn role permission assignments into configuration. so where are changing code with 0 test coverage and zero usages - better to remove it.

+++ b/core/modules/user/src/PermissionHandler.php
@@ -53,6 +53,13 @@ class PermissionHandler implements PermissionHandlerInterface {
+   * Stores the available permissions.
+   *
+   * @var array
+   */
+  protected $permissions;
+
+  /**

I guess since that last interdiff, this is useless? But are we sure we want to call build and sort on every call to getPermissions? That seems like a mistake.

Wow, I have no idea how you generated that interdiff :)
But the actual changes in the patch are correct. Thanks @dawehner!

Can we update https://www.drupal.org/node/2311427

Committed 52a101b and pushed to 8.0.x. Thanks!

In the meantime... the entity module was removed :)

Committed e206190 and pushed to 8.0.x. Thanks!

The link to permissions from modules page has disappeared since this patch.

Regression reported at #2513626: [Regression] Module permission links missing from module list page