Problem/Motivation

#2295469: Add support for static permission definitions with *.permissions.yml added support for permissions in YML files.

Proposed resolution

Let's add them everywhere we can.

Remaining tasks

Review the latest patch. RTBC and commit!

Decide whether to use structure in #24 (where 'title' key is defined separately) or structure in #28 (where shorthand permission syntax for permissions with titles only is used), and possibly update the change record to mention the shorthand syntax. (If this is a concern, let's open a follow-up issue. It is not yet documented and breaks the D7->D8 mental transition of using a separate key for title.

User interface changes

None.

API changes

None.

Files: 
CommentFileSizeAuthor
#74 2328411-74.patch311 bytesherom
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 76,272 pass(es).
[ View ]
#70 permissions-2328411-70.patch88.58 KBdawehner
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 76,262 pass(es).
[ View ]
#70 interdiff.txt962 bytesdawehner
#66 interdiff.txt792 bytesdawehner
#66 permissions-2328411-66.patch88.59 KBdawehner
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 76,321 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#64 interdiff.txt2.95 KBdawehner
#64 permissions-2328411-64.patch89.24 KBdawehner
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 76,335 pass(es).
[ View ]
#62 permissions-2328411-62.patch90.03 KBdawehner
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,586 pass(es), 22 fail(s), and 16 exception(s).
[ View ]
#62 interdiff.txt7.35 KBdawehner
#52 interdiff.txt2.09 KBdawehner
#52 permissions-2328411-52.patch85.77 KBdawehner
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 76,193 pass(es).
[ View ]
#49 interdiff.txt9.62 KBdawehner
#49 permissions-2328411-49.patch91.92 KBdawehner
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 76,133 pass(es).
[ View ]
#45 interdiff.txt3.69 KBgeerlingguy
#45 permissions-2328411-45.patch83.68 KBgeerlingguy
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 76,086 pass(es).
[ View ]
#43 interdiff.txt824 bytesgeerlingguy
#43 permissions-2328411-43.patch80.33 KBgeerlingguy
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 76,088 pass(es).
[ View ]
#40 interdiff.txt599 bytesgeerlingguy
#40 permissions-2328411-40.patch80.33 KBgeerlingguy
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 76,113 pass(es).
[ View ]
#38 permissions-2328411-38.patch80.33 KBdawehner
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 73,336 pass(es), 16 fail(s), and 19 exception(s).
[ View ]
#36 permissions-2328411-36.patch61.07 KBdawehner
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 76,078 pass(es), 2 fail(s), and 0 exception(s).
[ View ]
#28 2328411-28.patch62.84 KBherom
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,620 pass(es).
[ View ]
#28 interdiff-2328411-24-28.txt14.75 KBherom
#24 2328411-24.patch63.38 KBgeerlingguy
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,576 pass(es).
[ View ]
#22 2328411-22.patch63.22 KBgeerlingguy
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 74,877 pass(es), 6 fail(s), and 3 exception(s).
[ View ]
#20 2328411-20.patch63.02 KBgeerlingguy
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch 2328411-20.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#18 interdiff.txt393 bytesdawehner
#18 2328411-18.patch63.02 KBdawehner
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,114 pass(es), 8 fail(s), and 0 exception(s).
[ View ]
#15 interdiff.txt22.39 KBdawehner
#15 permission-2328411-15.patch63.02 KBdawehner
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 70,154 pass(es), 60 fail(s), and 23 exception(s).
[ View ]
#11 interdiff.txt4.98 KBdawehner
#11 permission-2328411-11.patch62.39 KBdawehner
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Drupal installation failed.
[ View ]
#9 interdiff.txt8.55 KBdawehner
#9 permission-2328411-9.patch57.58 KBdawehner
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,249 pass(es), 2 fail(s), and 1 exception(s).
[ View ]
#6 interdiff.txt2.06 KBdawehner
#6 permissions-2328411-6.patch49.39 KBdawehner
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,231 pass(es), 9 fail(s), and 2 exception(s).
[ View ]
#2 permissions-2328411-2.patch49.38 KBdawehner
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 44,812 pass(es), 326 fail(s), and 626 exception(s).
[ View ]

Comments

dawehner’s picture

Assigned:Unassigned» dawehner

First part.

<?php
$module_handler
= \Drupal::moduleHandler();
$permission_by_module = [];
foreach (
$modules as $module) {
 
$permission_by_module[$module] = $module_handler->invoke($module, 'permission');
}

foreach (
$permission_by_module as $module => $permissions) {
 
$permission_yaml = \Drupal\Component\Serialization\Yaml::encode($permissions);
 
$module_extension = \Drupal::moduleHandler()->getModule($module);
 
$yaml_filename = $module_extension->getPath() . "/$module.permissions.yml";
 
file_put_contents($yaml_filename, $permission_yaml);
}
?>
dawehner’s picture

Status:Active» Needs review
StatusFileSize
new49.38 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 44,812 pass(es), 326 fail(s), and 626 exception(s).
[ View ]

Here is a patch.

dawehner’s picture

The dynamic permissions are handled by damiankloip himself.

Status:Needs review» Needs work

The last submitted patch, 2: permissions-2328411-2.patch, failed testing.

tstoeckler’s picture

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? :-)

dawehner’s picture

Status:Needs work» Needs review
StatusFileSize
new49.39 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,231 pass(es), 9 fail(s), and 2 exception(s).
[ View ]
new2.06 KB

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? :-)

OH I wasn't aware that you don't need the quotes at all.

Note: This patch for now just fixes the failures but I do agree, we could drop the quotes.

geerlingguy’s picture

@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.

Status:Needs review» Needs work

The last submitted patch, 6: permissions-2328411-6.patch, failed testing.

dawehner’s picture

Status:Needs work» Needs review
StatusFileSize
new57.58 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,249 pass(es), 2 fail(s), and 1 exception(s).
[ View ]
new8.55 KB

Meh, some code relies on hook_permission() being there.

Status:Needs review» Needs work

The last submitted patch, 9: permission-2328411-9.patch, failed testing.

dawehner’s picture

Status:Needs work» Needs review
StatusFileSize
new62.39 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Drupal installation failed.
[ View ]
new4.98 KB

More work. Simple patches are never simple.

Status:Needs review» Needs work

The last submitted patch, 11: permission-2328411-11.patch, failed testing.

tstoeckler’s picture

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.

dawehner’s picture

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.

This is indeed part of the spec: http://yaml.org/spec/1.2/spec.html, see "Flow scalar Styles".

dawehner’s picture

Assigned:dawehner» Unassigned
Status:Needs work» Needs review
Issue tags:+DX (Developer Experience)
StatusFileSize
new63.02 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 70,154 pass(es), 60 fail(s), and 23 exception(s).
[ View ]
new22.39 KB

Used no quotes now. This is much better to read.

PS: I am stupid.

Status:Needs review» Needs work

The last submitted patch, 15: permission-2328411-15.patch, failed testing.

geerlingguy’s picture

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?).

dawehner’s picture

Status:Needs work» Needs review
StatusFileSize
new63.02 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,114 pass(es), 8 fail(s), and 0 exception(s).
[ View ]
new393 bytes

@geerlingguy
See patch

It is a small quote for me, but it is a big quote for drupalkind.

Status:Needs review» Needs work

The last submitted patch, 18: 2328411-18.patch, failed testing.

geerlingguy’s picture

Status:Needs work» Needs review
StatusFileSize
new63.02 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch 2328411-20.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Missed one more little quote :)

+view own unpublished content':

to:

+view own unpublished content:

Status:Needs review» Needs work

The last submitted patch, 20: 2328411-20.patch, failed testing.

geerlingguy’s picture

Status:Needs work» Needs review
StatusFileSize
new63.22 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 74,877 pass(es), 6 fail(s), and 3 exception(s).
[ View ]

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

Status:Needs review» Needs work

The last submitted patch, 22: 2328411-22.patch, failed testing.

geerlingguy’s picture

Status:Needs work» Needs review
StatusFileSize
new63.38 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,576 pass(es).
[ View ]

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

dawehner’s picture

@geerlingguy
Thank you for the work in this issue. Feel free to bring home this issue, if you want.

geerlingguy’s picture

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

dawehner’s picture

Issue summary:View changes

I don't see why there should be another one. Updated the IS a bit.

herom’s picture

StatusFileSize
new14.75 KB
new62.84 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,620 pass(es).
[ View ]

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'
geerlingguy’s picture

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.

geerlingguy’s picture

Issue summary:View changes
damiankloip’s picture

+++ 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?

dawehner’s picture

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

Yes it was. I mirrored the way how the hook was handled. I actually though dislike the change a lot as it makes it a bit inconsistent. Note: All core permissions
are actually using title only. Classical out of scope change, if you ask me.

dawehner’s picture

Title:Convert all static permissions to yml files» Convert all permissions to yml files and permission callbacks.
Priority:Normal» Major
Status:Needs review» Postponed
geerlingguy’s picture

Issue summary:View changes
geerlingguy’s picture

Status:Postponed» Needs review

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)...

dawehner’s picture

StatusFileSize
new61.07 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 76,078 pass(es), 2 fail(s), and 0 exception(s).
[ View ]

Reroll of #24 for now. So yeah we should update the patch to use permission_callbacks now.

Status:Needs review» Needs work

The last submitted patch, 36: permissions-2328411-36.patch, failed testing.

dawehner’s picture

Status:Needs work» Needs review
StatusFileSize
new80.33 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 73,336 pass(es), 16 fail(s), and 19 exception(s).
[ View ]

Let's convert all other instances.

Status:Needs review» Needs work

The last submitted patch, 38: permissions-2328411-38.patch, failed testing.

geerlingguy’s picture

Status:Needs work» Needs review
StatusFileSize
new80.33 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 76,113 pass(es).
[ View ]
new599 bytes

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

dawehner’s picture

Ah cool, thank you for the fix. Anyone wants to RTBC it?

herom’s picture

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

geerlingguy’s picture

StatusFileSize
new80.33 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 76,088 pass(es).
[ View ]
new824 bytes

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

herom’s picture

Status:Needs review» Needs work

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()
geerlingguy’s picture

Status:Needs work» Needs review
StatusFileSize
new83.68 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 76,086 pass(es).
[ View ]
new3.69 KB

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

geerlingguy’s picture

Issue summary:View changes
dawehner’s picture

+++ b/core/modules/system/core.api.php
@@ -551,10 +551,11 @@
+ * Modules define permissions via a $module.permissions.yml file or by
+ * implementing hook_permission(). The return value defines machine names,

This still refers to hook_permission ... but it should actually talk about the permissions_callback ...

geerlingguy’s picture

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.

dawehner’s picture

StatusFileSize
new91.92 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 76,133 pass(es).
[ View ]
new9.62 KB

@geerlingguy
Well, the idea of this issue was actually to get rid of the module based support.

geerlingguy’s picture

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?

damiankloip’s picture

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?

dawehner’s picture

StatusFileSize
new85.77 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 76,193 pass(es).
[ View ]
new2.09 KB

Okay, let's adapt just the comment and mark it as deprecated for now.

geerlingguy’s picture

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

dawehner’s picture

Shall I open a follow-up to mark the old hook deprecated?

This would be great!

herom’s picture

Status:Needs review» Reviewed & tested by the community

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

The patch looks great.

dawehner’s picture

@herom
It would be also great if you could open up a follow up for your changes, so we can discuss things there.

geerlingguy’s picture

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!

dawehner’s picture

@geerlingguy
So have fun with whatever new episode you start now.

Adding a possible follow up for more consistency.

herom’s picture

alexpott’s picture

Status:Reviewed & tested by the community» Needs work
  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.
dawehner’s picture

Status:Needs work» Needs review
StatusFileSize
new7.35 KB
new90.03 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,586 pass(es), 22 fail(s), and 16 exception(s).
[ View ]

Thank you alex, for your great review!

Status:Needs review» Needs work

The last submitted patch, 62: permissions-2328411-62.patch, failed testing.

dawehner’s picture

Status:Needs work» Needs review
StatusFileSize
new89.24 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 76,335 pass(es).
[ View ]
new2.95 KB

Meh.

tim.plunkett’s picture

+++ 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.

dawehner’s picture

Related issues:+#2339487: Static cache permissions
StatusFileSize
new88.59 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 76,321 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
new792 bytes

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.

Well, those permissions never cached its result previously, so we did not added it in the original permissions handler issue, so do I want in that,
because this causes test failures, see #62

Added a follow up.

dawehner’s picture

Oh mh, the interdiff is totally screwed up.

tim.plunkett’s picture

Status:Needs review» Reviewed & tested by the community

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

Status:Reviewed & tested by the community» Needs work

The last submitted patch, 66: permissions-2328411-66.patch, failed testing.

dawehner’s picture

Status:Needs work» Reviewed & tested by the community
StatusFileSize
new962 bytes
new88.58 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 76,262 pass(es).
[ View ]

Meh!

alexpott’s picture

Status:Reviewed & tested by the community» Fixed
Issue tags:+Needs change record updates

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

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

  • alexpott committed 52a101b on 8.0.x
    Issue #2328411 by dawehner, geerlingguy, herom: Convert all permissions...
dawehner’s picture

Updated.

herom’s picture

Status:Fixed» Needs review
Issue tags:-Needs change record updates
StatusFileSize
new311 bytes
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 76,272 pass(es).
[ View ]

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

alexpott’s picture

Status:Needs review» Fixed

Committed e206190 and pushed to 8.0.x. Thanks!

  • alexpott committed e206190 on 8.0.x
    Issue #2328411 followup by herom: Convert all permissions to yml files...

Status:Fixed» Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

deguif’s picture

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

akalata’s picture