Now that user roles are configurables (#1479454: Convert user roles to configurables) we should incorporate the permissions that have been assigned to a role into the UserRole entity.

For example the default anonymous role config entity

id: anonymous
label: Anonymous user
weight: 0
permissions:
  - 'access content'
langcode: en

Stale permission assignment on module uninstallation will temporarly be removed until #2003966: Helper issue for #1199946 : Disabled modules are broken beyond repair is resolved which will allow us to cleanup permissions in hook_module_preuninstall().

Follow-up issues:

#2025089: Remove redundant (role-/permission related) procedural wrappers from user.module
#2026983: Clean up module permissions on uninstall

Files: 
CommentFileSizeAuthor
#128 124-127-interdiff.txt1006 bytesalexpott
#128 1872876-127.patch73.71 KBalexpott
PASSED: [[SimpleTest]]: [MySQL] 58,090 pass(es).
[ View ]
#127 1872876_127.patch73.7 KBchx
PASSED: [[SimpleTest]]: [MySQL] 58,163 pass(es).
[ View ]
#127 interdiff.txt1006 byteschx
#124 1872876-124.patch73.71 KBfubhy
PASSED: [[SimpleTest]]: [MySQL] 58,196 pass(es).
[ View ]
1872876-124.patch73.71 KBfubhy
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]
#120 1872876-120.patch73.71 KBfubhy
PASSED: [[SimpleTest]]: [MySQL] 57,989 pass(es).
[ View ]
#118 1872876-118.patch75.88 KBfubhy
FAILED: [[SimpleTest]]: [MySQL] 58,025 pass(es), 22 fail(s), and 0 exception(s).
[ View ]
#116 1872876-116.patch69.86 KBfubhy
FAILED: [[SimpleTest]]: [MySQL] 58,059 pass(es), 34 fail(s), and 0 exception(s).
[ View ]
#112 1872876-111.patch75.89 KBfubhy
FAILED: [[SimpleTest]]: [MySQL] 57,994 pass(es), 23 fail(s), and 0 exception(s).
[ View ]
#112 interdiff.txt2.19 KBfubhy
#108 user_permissions_config-1872876-106.patch75.83 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] 57,426 pass(es), 91 fail(s), and 6,570 exception(s).
[ View ]
#108 interdiff.txt13.69 KBdawehner
#105 1872876-105.patch69.79 KBfubhy
FAILED: [[SimpleTest]]: [MySQL] 58,212 pass(es), 40 fail(s), and 1 exception(s).
[ View ]
#101 1872876-101.patch66.61 KBfubhy
FAILED: [[SimpleTest]]: [MySQL] 57,705 pass(es), 45 fail(s), and 202 exception(s).
[ View ]
#96 1872876-96.patch58.39 KBfubhy
FAILED: [[SimpleTest]]: [MySQL] 37,086 pass(es), 10,812 fail(s), and 2,956 exception(s).
[ View ]
#89 1872876-89.patch58.21 KBfubhy
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1872876-89.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#85 1872876_75.patch59.43 KBfubhy
FAILED: [[SimpleTest]]: [MySQL] 55,827 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#81 75-81-interdiff.txt13.67 KBalexpott
#81 1872876_81.patch62.81 KBalexpott
FAILED: [[SimpleTest]]: [MySQL] 56,278 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#75 74-75-interdiff.txt755 bytesalexpott
#75 1872876_75.patch59.43 KBalexpott
PASSED: [[SimpleTest]]: [MySQL] 56,097 pass(es).
[ View ]
#74 70-74-interdiff.txt1007 bytesalexpott
#74 1872876_74.patch59.55 KBalexpott
PASSED: [[SimpleTest]]: [MySQL] 55,780 pass(es).
[ View ]
#70 68-70-interdiff.txt1008 bytesalexpott
#70 1872876_70.patch59.55 KBalexpott
PASSED: [[SimpleTest]]: [MySQL] 55,624 pass(es).
[ View ]
#68 67-68-interdiff.txt919 bytesalexpott
#68 1872876_68.patch59.58 KBalexpott
FAILED: [[SimpleTest]]: [MySQL] 55,939 pass(es), 3 fail(s), and 0 exception(s).
[ View ]
#67 62-67-interdiff.txt15.11 KBalexpott
#67 1872876_67.patch59.71 KBalexpott
FAILED: [[SimpleTest]]: [MySQL] 56,539 pass(es), 3 fail(s), and 0 exception(s).
[ View ]
#62 1872876_62.patch56.48 KBchx
PASSED: [[SimpleTest]]: [MySQL] 55,680 pass(es).
[ View ]
#62 interdiff.txt724 byteschx
#58 1872876_57.patch56.79 KBchx
PASSED: [[SimpleTest]]: [MySQL] 55,407 pass(es).
[ View ]
#58 interdiff.txt630 byteschx
#55 1872876_55.patch56.75 KBchx
FAILED: [[SimpleTest]]: [MySQL] 56,303 pass(es), 0 fail(s), and 66 exception(s).
[ View ]
#55 interdiff.txt2.01 KBchx
#54 interdiff.txt1.04 KBchx
#54 1872876_54.patch56.06 KBchx
PASSED: [[SimpleTest]]: [MySQL] 55,565 pass(es).
[ View ]
#51 1872876_51.patch55.79 KBchx
PASSED: [[SimpleTest]]: [MySQL] 55,520 pass(es).
[ View ]
#51 interdiff.txt3.27 KBchx
#48 1872876_48.patch57.02 KBchx
PASSED: [[SimpleTest]]: [MySQL] 55,376 pass(es).
[ View ]
#48 interdiff_47_48.txt791 byteschx
#48 interdiff.txt12 KBchx
#47 1872876_47.patch57.01 KBchx
PASSED: [[SimpleTest]]: [MySQL] 55,491 pass(es).
[ View ]
#47 interdiff.txt11.99 KBchx
#47 interdiff_45_47.txt1.9 KBchx
#45 1872876_45.patch56.92 KBchx
FAILED: [[SimpleTest]]: [MySQL] 55,351 pass(es), 59 fail(s), and 31 exception(s).
[ View ]
#45 interdiff_43_45.txt1.93 KBchx
#45 interdiff.txt11.94 KBchx
#43 1872876_43.patch55.91 KBchx
FAILED: [[SimpleTest]]: [MySQL] 55,238 pass(es), 7 fail(s), and 82 exception(s).
[ View ]
#43 interdiff.txt10.88 KBchx
#43 interdiff_41_43.txt446 byteschx
#41 1872876_40.patch55.33 KBchx
FAILED: [[SimpleTest]]: [MySQL] 36,939 pass(es), 10,585 fail(s), and 39,971 exception(s).
[ View ]
#41 interdiff.txt6.22 KBchx
#39 1872876_38.patch55.53 KBchx
FAILED: [[SimpleTest]]: [MySQL] 36,824 pass(es), 10,485 fail(s), and 31,817 exception(s).
[ View ]
#39 interdiff.txt11.19 KBchx
#34 drupal-1872876-34.patch55.42 KBdawehner
PASSED: [[SimpleTest]]: [MySQL] 55,983 pass(es).
[ View ]
#34 interdiff.txt495 bytesdawehner
#31 drupal-1872876-31.patch55.44 KBdawehner
PASSED: [[SimpleTest]]: [MySQL] 55,432 pass(es).
[ View ]
#31 interdiff.txt10.44 KBdawehner
#30 drupal-1872876-30.patch55.21 KBdawehner
PASSED: [[SimpleTest]]: [MySQL] 55,839 pass(es).
[ View ]
#30 interdiff.txt1.34 KBdawehner
#28 role-perms-1872876-28.patch55.92 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] 55,583 pass(es), 3 fail(s), and 0 exception(s).
[ View ]
#26 interdiff.txt936 bytesdawehner
#26 drupal-1872876-26.patch55.96 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] 53,945 pass(es), 2 fail(s), and 0 exception(s).
[ View ]
#25 drupal-1872876-25.patch55.59 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] 50,930 pass(es), 60 fail(s), and 14 exception(s).
[ View ]
#22 1872876_22.patch56.24 KBchx
PASSED: [[SimpleTest]]: [MySQL] 53,285 pass(es).
[ View ]
#20 1872876_20.patch56.18 KBchx
FAILED: [[SimpleTest]]: [MySQL] 53,312 pass(es), 0 fail(s), and 12 exception(s).
[ View ]
#20 interdiff.txt388 byteschx
#17 1872876_17.patch55.64 KBchx
FAILED: [[SimpleTest]]: [MySQL] 53,319 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#17 interdiff.txt1.26 KBchx
#16 not_interdiff.txt1.3 KBchx
#16 1872876_16.patch55.64 KBchx
FAILED: [[SimpleTest]]: [MySQL] 53,212 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#14 1872876_14.patch55.39 KBchx
PASSED: [[SimpleTest]]: [MySQL] 53,134 pass(es).
[ View ]
#14 interdiff.txt3.29 KBchx
#12 1872876_12.patch55.39 KBchx
PASSED: [[SimpleTest]]: [MySQL] 53,146 pass(es).
[ View ]
#12 interdiff.txt3.93 KBchx
#11 1872876_11.patch55.29 KBchx
PASSED: [[SimpleTest]]: [MySQL] 53,107 pass(es).
[ View ]
#11 interdiff.txt17.82 KBchx
#8 drupal-1872876-8.patch54.68 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] 53,128 pass(es), 3 fail(s), and 31 exception(s).
[ View ]
#8 interdiff.txt18.6 KBdawehner
#4 1872876_4.patch35.65 KBchx
FAILED: [[SimpleTest]]: [MySQL] 53,097 pass(es), 0 fail(s), and 30 exception(s).
[ View ]
#4 interdiff.txt456 byteschx
#2 1872876_2.patch35.65 KBchx
FAILED: [[SimpleTest]]: [MySQL] 52,873 pass(es), 142 fail(s), and 30 exception(s).
[ View ]

Comments

fubhy’s picture

#1872874: Remove the module name property from the role permission table still blocks this issue from moving forward and didn't get any reviews yet. Anyone got a few minutes to take a look?

chx’s picture

Assigned:Unassigned» chx
Status:Active» Needs review
StatusFileSize
new35.65 KB
FAILED: [[SimpleTest]]: [MySQL] 52,873 pass(es), 142 fail(s), and 30 exception(s).
[ View ]

That doesn't block this. I've seen Drupal install and the Field UI upgrade test pass. If that test passes... note: I have made some cleanups that could've been done otherwise, mostly to use API functions where they weren't. Also, I brutally simplified a Views filter, left a message to Daniel to verify it is indeed unnecessary code.

Status:Needs review» Needs work

The last submitted patch, 1872876_2.patch, failed testing.

chx’s picture

StatusFileSize
new456 bytes
new35.65 KB
FAILED: [[SimpleTest]]: [MySQL] 53,097 pass(es), 0 fail(s), and 30 exception(s).
[ View ]

While it doesn't quite block, it is quite tedious :)

chx’s picture

Status:Needs work» Needs review

Status:Needs review» Needs work

The last submitted patch, 1872876_4.patch, failed testing.

chx’s picture

After a discussion with dawehner: a) we need a filter for enabled permissions cos only uninstalled permissions are removed -- but the list of permissions retrieved by module_invoke_all includes only the permissions by enabled modules so we don't need to consult the other config file which is slated for removal anyways b) we will want to move $rid to the config object name from the top key so that we have one object per rid. Next revision will include a) and once that's passed I will get to b).

dawehner’s picture

StatusFileSize
new18.6 KB
new54.68 KB
FAILED: [[SimpleTest]]: [MySQL] 53,128 pass(es), 3 fail(s), and 31 exception(s).
[ View ]

Started to work on tests for the filter and field, which clearly shows how many issues we have with the filter.

chx’s picture

Status:Needs work» Needs review

Status:Needs review» Needs work

The last submitted patch, drupal-1872876-8.patch, failed testing.

chx’s picture

Status:Needs work» Needs review
StatusFileSize
new17.82 KB
new55.29 KB
PASSED: [[SimpleTest]]: [MySQL] 53,107 pass(es).
[ View ]

This splits the config objects per role id because -- config('foo')->get() is always an array but config('foo')->get('something') can be NULL and PHP doesnt like a NULL when it expects an array. Huge props to dawehner for making the Views user test coverage more solid.

chx’s picture

StatusFileSize
new3.93 KB
new55.39 KB
PASSED: [[SimpleTest]]: [MySQL] 53,146 pass(es).
[ View ]

One more roll, the modules-permissions is moved out of the user.permission. namespace to avoid confusion. The commented out Views tests have a todo , dawehner will file a followup and get 'em fixed. This should be ready.

fubhy’s picture

Mostly nitpicky review. There are some ugly things which are caused by the module name that we currently store along with the permission assignments but that can be solved later as chx pointed out. So, thanks for solving this issue despite that uglyness.

+++ b/core/includes/update.incundefined
@@ -1533,3 +1533,35 @@ function update_add_cache_columns($table) {
+/**
+ * Rename permissions during update.
+ *
+ * This function can rename one permission to several or even delete an old
+ * one.
+ *
+ * @param array $rename
+ *   An associative array. The keys are the old permissions the values are a
+ *   list of new permissions. If the list is an empty array, the old permission
+ *   is removed.
+ */
+function update_rename_permissions($rename) {
+  $prefix = 'user.role.';
+  $cut = strlen($prefix);
+  $role_names = \Drupal::service('config.storage')->listAll($prefix);
+  foreach ($role_names as $role_name) {
+    $rid = substr($role_name, $cut);
+    $config = \Drupal::service('config.factory')->get("user.permission.$rid");
+    $permissions = $config->get();
+    foreach ($rename as $old_permission => $new_permissions) {
+      $key = array_search($old_permission, $permissions);
+      if ($key !== FALSE) {
+        unset($permissions[$key]);
+        $permissions = array_merge($permissions, $new_permissions);
+      }
+    }
+    $config
+      ->setData($permissions)
+      ->save();
+  }

Not sure about the name of that function given what it does. I had to read the function 3 times before I saw what was going on. The feature set (rename, replace with multiple, delete) is basically caused by the array_merge(). It's okay to have all that in this one function but it's rather a replace then. So can we name it 'update_replace_permissions'?

+++ b/core/modules/system/system.installundefined
@@ -1611,13 +1611,12 @@ function system_update_8020() {
+      ->set("ban.ban IP addresses", TRUE)

double quotes! (not that it matters)

+++ b/core/modules/user/lib/Drupal/user/Tests/Views/HandlerFilterPermissionTest.phpundefined
@@ -0,0 +1,123 @@
+   *
+   * @var array
+   */
+  public static $testViews = array('test_filter_permission');
+
+
+  protected $columnMap;
+
+  public static function getInfo() {
+    return array(

Two line breaks.

+++ b/core/modules/user/user.installundefined
@@ -1055,5 +999,41 @@ function user_update_8017() {
+ * @param int $rid
+ *   A D7 numeric role ID.
+ * @return string
+ *   A D8 string role ID.

Missing empty line between @param and @return.

+++ b/core/modules/user/user.installundefined
@@ -1055,5 +999,41 @@ function user_update_8017() {
+function _user_update_map_rid($rid) {

Do we really need a helper function for that? We are only using that in two places :). Well, I guess it doesn't hurt.

+++ b/core/modules/user/user.installundefined
@@ -1055,5 +999,41 @@ function user_update_8017() {
+}
+/**
+ * Migrate roles permissions into configuration.

Missing empty line after function.

----

Is there a follow-up for solving the views tests as mentioned in the @todos?

chx’s picture

StatusFileSize
new3.29 KB
new55.39 KB
PASSED: [[SimpleTest]]: [MySQL] 53,134 pass(es).
[ View ]

Two line breaks! Oh dear. Quick, let's fix it before the universe unravels in the face of such a horror.

I am grateful for the review but I couldn't resist.

fubhy’s picture

Status:Needs review» Reviewed & tested by the community

Okay. RTBC once this comes back green (which it will) :)

chx’s picture

StatusFileSize
new55.64 KB
FAILED: [[SimpleTest]]: [MySQL] 53,212 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
new1.3 KB

Keeping up with HEAD. As interdiff is not possible, I attached a direct diff of the two patches.

chx’s picture

StatusFileSize
new1.26 KB
new55.64 KB
FAILED: [[SimpleTest]]: [MySQL] 53,319 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

fubhy points out that while update_rename_permissions is now update_replace_permissions the variable and the doxygen didnt'f wolow. As you can see, replacing letters is easy and cheap :)

fubhy’s picture

Especially with PhpStorm, eh? +1 RTBC for the HEAD chasing.

Status:Reviewed & tested by the community» Needs work

The last submitted patch, 1872876_17.patch, failed testing.

chx’s picture

Status:Needs work» Reviewed & tested by the community
StatusFileSize
new388 bytes
new56.18 KB
FAILED: [[SimpleTest]]: [MySQL] 53,312 pass(es), 0 fail(s), and 12 exception(s).
[ View ]

Grr!

Status:Reviewed & tested by the community» Needs work

The last submitted patch, 1872876_20.patch, failed testing.

chx’s picture

Status:Needs work» Reviewed & tested by the community
StatusFileSize
new56.24 KB
PASSED: [[SimpleTest]]: [MySQL] 53,285 pass(es).
[ View ]

To clarify the last few rerolls: during the commitfest another installSchema call got added. I needed to remove that. But, that installSchema does not get a list of tables to install but the name of the module and then either a string or an array is a DrupalWTF. And, has absolutely nothing to do with the substrance of this patch and so the RTBC is correct.

xjm’s picture

#22: 1872876_22.patch queued for re-testing.

alexpott’s picture

Assigned:chx» Unassigned
Status:Reviewed & tested by the community» Needs work

It seems like the HandlerFilterPermissionTest is unfinished

+++ b/core/modules/user/lib/Drupal/user/Tests/Views/HandlerFilterPermissionTest.phpundefined
@@ -0,0 +1,122 @@
+    // Filter by a non existing permission with operator NOT.
+    // $view->initHandlers();
+    // $view->filter['permission']->value = array('Non existent permission');
+    // $view->filter['permission']->operator = 'not';
+    // $this->executeView($view);
+    // $this->assertEqual(count($view->result), count($this->users), 'A non existent permission with the NOt operator should result in every user');
+    // $expected = array();
+    // $expected[] = array('uid' => 1);
+    // $expected[] = array('uid' => 2);
+    // $expected[] = array('uid' => 3);
+    // $expected[] = array('uid' => 4);
+    // $this->assertIdenticalResultset($view, $expected, $column_map);
+    // $view->destroy();
+++ b/core/modules/user/lib/Drupal/user/Tests/Views/HandlerFilterPermissionTest.phpundefined
@@ -0,0 +1,122 @@
+    // Filter by a permission with operator NOT.
+    // $view->initHandlers();
+    // $view->filter['permission']->value = array('administer permissions');
+    // $view->filter['permission']->operator = 'not';
+    // $this->executeView($view);
+    // $this->assertEqual(count($view->result), 2);
+    // $expected = array();
+    // $expected[] = array('uid' => 1);
+    // $expected[] = array('uid' => 2);
+    // $this->assertIdenticalResultset($view, $expected, $column_map);
+    // $view->destroy();
+++ b/core/modules/user/lib/Drupal/user/Tests/Views/HandlerFilterPermissionTest.phpundefined
@@ -0,0 +1,122 @@
+    // Filter by another permission of a role with multiple permissions and
+    // operator NOT.
+    // $view->initHandlers();
+    // $view->filter['permission']->value = array('administer users');
+    // $view->filter['permission']->operator = 'not';
+    // $this->executeView($view);
+    // $this->assertEqual(count($view->result), 3);
+    // $expected = array();
+    // $expected[] = array('uid' => 1);
+    // $expected[] = array('uid' => 2);
+    // $expected[] = array('uid' => 3);
+    // $this->assertIdenticalResultset($view, $expected, $column_map);
+    // $view->destroy();
+++ b/core/modules/user/lib/Drupal/user/Tests/Views/HandlerFilterPermissionTest.phpundefined
@@ -0,0 +1,122 @@
+    // @todo Test the available UI options.
dawehner’s picture

Status:Needs work» Needs review
StatusFileSize
new55.59 KB
FAILED: [[SimpleTest]]: [MySQL] 50,930 pass(es), 60 fail(s), and 14 exception(s).
[ View ]

Opened another issue for the @todo in the test: #1959296: Fix the not operation in the many to one handling.

That's just a rerole.

dawehner’s picture

StatusFileSize
new55.96 KB
FAILED: [[SimpleTest]]: [MySQL] 53,945 pass(es), 2 fail(s), and 0 exception(s).
[ View ]
new936 bytes

Added tests for the UI part.

Status:Needs review» Needs work

The last submitted patch, drupal-1872876-26.patch, failed testing.

tim.plunkett’s picture

Status:Needs work» Needs review
StatusFileSize
new55.92 KB
FAILED: [[SimpleTest]]: [MySQL] 55,583 pass(es), 3 fail(s), and 0 exception(s).
[ View ]

Rerolled.

Status:Needs review» Needs work

The last submitted patch, role-perms-1872876-28.patch, failed testing.

dawehner’s picture

Status:Needs work» Needs review
StatusFileSize
new1.34 KB
new55.21 KB
PASSED: [[SimpleTest]]: [MySQL] 55,839 pass(es).
[ View ]

It's actually good to see that we got more and more tests over time.

dawehner’s picture

StatusFileSize
new10.44 KB
new55.44 KB
PASSED: [[SimpleTest]]: [MySQL] 55,432 pass(es).
[ View ]

I don't really understand the todo here:

+  // @TODO: remove.
+  $modules = user_permission_get_modules();
+  $config = Drupal::config('user.module.permission');
+  foreach ($modules as $permission => $module) {
+    $config->set("$module.$permission", TRUE);
   }
+  $config->save();

I hope that chx can enlighten us.

This is just a list of small cleanups/adaptions to new things in drupal.

chx’s picture

That whole permissions-by-module stuff is just for uninstall. Die, die, die! I hope we can remove it at one point. So that's what the todo is: remove if we can.

tim.plunkett’s picture

Status:Needs review» Reviewed & tested by the community

Another one bites the dust!
@dawehner++ @chx++

dawehner’s picture

StatusFileSize
new495 bytes
new55.42 KB
PASSED: [[SimpleTest]]: [MySQL] 55,983 pass(es).
[ View ]

We agree that this @todo is not helpful there, so let's drop it.

andypost’s picture

This requires follow-up to be filed.

+++ b/core/modules/user/lib/Drupal/user/Plugin/views/field/Permissions.phpundefined
@@ -40,32 +40,24 @@ function pre_render(&$values) {
+      $permission_names = \Drupal::moduleHandler()->invokeAll('permission');
+      $query = db_select('users_roles', 'u');
...
+      foreach ($result as $row) {
+        foreach (\Drupal::config("user.permission.$row->rid")->get() as $permission) {
+          $this->items[$row->uid][$permission]['permission'] = $permission_names[$permission]['title'];

Maybe better to implement helper in follow-up. Seems hook execution for for each field is too expensive without caching

+++ b/core/modules/user/lib/Drupal/user/Tests/Views/HandlerFilterPermissionTest.phpundefined
@@ -0,0 +1,131 @@
+   * @todo Fix the different commented out tests by fixing the many to one
+   *   handler handling with the NOT operator.
...
+    // Filter by a non existing permission with operator NOT.
+    // $view->initHandlers();
...
+    // Filter by a permission with operator NOT.
+    // $view->initHandlers();
...
+    // Filter by another permission of a role with multiple permissions and
+    // operator NOT.
+    // $view->initHandlers();

Is there a reason to not make a follow up and add link here for @todo

dawehner’s picture

Thanks for your comment.

Maybe better to implement helper in follow-up. Seems hook execution for for each field is too expensive without caching

This just happens once for the full result. I don't think that this is a performance issue we have to tackle.

Is there a reason to not make a follow up and add link here for @todo

There is already a follow up for that: #1959296: Fix the not operation in the many to one handling.

alexpott’s picture

Status:Reviewed & tested by the community» Needs review

From a conversation on IRC.

alexpott:
just wondering why the info contained in user.permission.anonymous.yml is not just an array on user.role.anonymous.yml. If I want to deploy a role - to me that's always going to include its permissions… and the relationship between these files will always be 1 to 1
dawehner:
that is a good point. as far as I know one goal to the current approach is to avoid using the user roles config entity
alexpott:
gotta ask… why?
dawehner:
i'm not 100% sure, but I think it's considered as a bad practice to load the config files directly for config entities... then loading the entity might be too much overhead
alexpott:
hmmm... it seems a shame

There doesn't seem to be be any explanation on the issue as to why adding a permission property to the role config entity was not considered.

Putting the issue at needs review cause I want to know why we're making it more difficult to deploy a role we have move three files instead of just two... (remember the manifest :) )

alexpott’s picture

Tagging

chx’s picture

StatusFileSize
new11.19 KB
new55.53 KB
FAILED: [[SimpleTest]]: [MySQL] 36,824 pass(es), 10,485 fail(s), and 31,817 exception(s).
[ View ]

Status:Needs review» Needs work

The last submitted patch, 1872876_38.patch, failed testing.

chx’s picture

Status:Needs work» Needs review
StatusFileSize
new6.22 KB
new55.33 KB
FAILED: [[SimpleTest]]: [MySQL] 36,939 pass(es), 10,585 fail(s), and 39,971 exception(s).
[ View ]

Status:Needs review» Needs work

The last submitted patch, 1872876_40.patch, failed testing.

chx’s picture

Status:Needs work» Needs review
StatusFileSize
new446 bytes
new10.88 KB
new55.91 KB
FAILED: [[SimpleTest]]: [MySQL] 55,238 pass(es), 7 fail(s), and 82 exception(s).
[ View ]

The interdiff is against #34 because that's the last reviewed patch. The fix wasn't too hard.

Status:Needs review» Needs work

The last submitted patch, 1872876_43.patch, failed testing.

chx’s picture

Status:Needs work» Needs review
StatusFileSize
new11.94 KB
new1.93 KB
new56.92 KB
FAILED: [[SimpleTest]]: [MySQL] 55,351 pass(es), 59 fail(s), and 31 exception(s).
[ View ]

Status:Needs review» Needs work

The last submitted patch, 1872876_45.patch, failed testing.

chx’s picture

Status:Needs work» Needs review
StatusFileSize
new1.9 KB
new11.99 KB
new57.01 KB
PASSED: [[SimpleTest]]: [MySQL] 55,491 pass(es).
[ View ]

Hopefully this is the last. Still interdiff against #34.

chx’s picture

StatusFileSize
new12 KB
new791 bytes
new57.02 KB
PASSED: [[SimpleTest]]: [MySQL] 55,376 pass(es).
[ View ]

Opsie. I suspect this doesn't have test coverage.

andypost’s picture

Looks awesome! +1 to RTBC

andypost’s picture

Status:Needs review» Needs work

NW for user.schema and let's not commit dead code

+++ b/core/modules/user/lib/Drupal/user/Plugin/Core/Entity/Role.phpundefined
@@ -65,6 +65,13 @@ class Role extends ConfigEntityBase implements RoleInterface {
+   * The permissions belonging to this role.
...
+  public $permissions = array();

/user/config/schema/user/schema.yml needs update too

+++ b/core/modules/user/lib/Drupal/user/Tests/Views/HandlerFilterPermissionTest.phpundefined
@@ -0,0 +1,131 @@
+    // Filter by a non existing permission with operator NOT.
+    // $view->initHandlers();
+    // $view->filter['permission']->value = array('Non existent permission');
+    // $view->filter['permission']->operator = 'not';
+    // $this->executeView($view);
+    // $this->assertEqual(count($view->result), count($this->users), 'A non existent permission with the NOt operator should result in every user');
+    // $expected = array();
+    // $expected[] = array('uid' => 1);
+    // $expected[] = array('uid' => 2);
+    // $expected[] = array('uid' => 3);
+    // $expected[] = array('uid' => 4);
+    // $this->assertIdenticalResultset($view, $expected, $column_map);
+    // $view->destroy();
...
+    // Filter by a permission with operator NOT.
+    // $view->initHandlers();
+    // $view->filter['permission']->value = array('administer permissions');
+    // $view->filter['permission']->operator = 'not';
+    // $this->executeView($view);
+    // $this->assertEqual(count($view->result), 2);
+    // $expected = array();
+    // $expected[] = array('uid' => 1);
+    // $expected[] = array('uid' => 2);
+    // $this->assertIdenticalResultset($view, $expected, $column_map);
+    // $view->destroy();
...
+    // Filter by another permission of a role with multiple permissions and
+    // operator NOT.
+    // $view->initHandlers();
+    // $view->filter['permission']->value = array('administer users');
+    // $view->filter['permission']->operator = 'not';
+    // $this->executeView($view);
+    // $this->assertEqual(count($view->result), 3);
+    // $expected = array();
+    // $expected[] = array('uid' => 1);
+    // $expected[] = array('uid' => 2);
+    // $expected[] = array('uid' => 3);
+    // $this->assertIdenticalResultset($view, $expected, $column_map);
+    // $view->destroy();

do we really need this code without @todo and link?

chx’s picture

Status:Needs work» Needs review
StatusFileSize
new3.27 KB
new55.79 KB
PASSED: [[SimpleTest]]: [MySQL] 55,520 pass(es).
[ View ]

Views tests are by dawehner and I have nothing to do with them so I just removed them so this can get in. Added schema.

andypost’s picture

Status:Needs review» Reviewed & tested by the community

should be green!

beejeebus’s picture

Status:Reviewed & tested by the community» Needs work
23:08       beejeebus   chx: i'm a bit scared by this comment:
23:08       beejeebus   Can not use the entity system as this function is used during update.
23:08       beejeebus   in user_role_permissions()
23:09             chx   and why is that scary
23:09       beejeebus   chx: i guess we just have to document that you can't use entity hooks to restrict access?
23:09       beejeebus   or events or WhateverTF they are now
23:11             chx   nice c_atch
23:11             chx   i will reroll with a function_exists('entity_load_multiple') and fork the function
23:11             chx   please set to nw
23:11             chx   i need to run

so, marking as NW as chx suggests.

chx’s picture

Status:Needs work» Needs review
StatusFileSize
new56.06 KB
PASSED: [[SimpleTest]]: [MySQL] 55,565 pass(es).
[ View ]
new1.04 KB

Thanks.

chx’s picture

StatusFileSize
new2.01 KB
new56.75 KB
FAILED: [[SimpleTest]]: [MySQL] 56,303 pass(es), 0 fail(s), and 66 exception(s).
[ View ]

Maybe easier to understand this way.

beejeebus’s picture

nice work chx, i think that is RTBC if green.

Status:Needs review» Needs work

The last submitted patch, 1872876_55.patch, failed testing.

chx’s picture

Status:Needs work» Needs review
StatusFileSize
new630 bytes
new56.79 KB
PASSED: [[SimpleTest]]: [MySQL] 55,407 pass(es).
[ View ]

Sigh.

andypost’s picture

Status:Needs review» Reviewed & tested by the community

Back to RTBC

alexpott’s picture

Status:Reviewed & tested by the community» Needs work

We're nearly there...

+++ b/core/modules/system/system.installundefined
@@ -1521,13 +1521,12 @@ function system_update_8020() {
+    config('user.module.permission')
+      ->set('ban.ban IP addresses', TRUE)
+      ->save();

+++ b/core/modules/user/user.moduleundefined
@@ -1982,23 +1989,20 @@ function user_role_change_permissions($rid, array $permissions = array()) {
+  $modules = user_permission_get_modules();
+  $config = Drupal::config('user.module.permission');
+  foreach ($modules as $permission => $module) {
+    $config->set("$module.$permission", TRUE);
   }
+  $config->save();

@@ -2635,11 +2632,25 @@ function user_modules_installed($modules) {
function user_modules_uninstalled($modules) {
-   db_delete('role_permission')
-     ->condition('module', $modules, 'IN')
-     ->execute();
+  $config = Drupal::config('user.module.permission');
+  $permissions_to_remove = array();
+  foreach ($modules as $module) {
+    $permissions_to_remove += (array) $config->get($module);
+    $config->clear($module);
+  }
+  $config->save();
+  if ($permissions_to_remove) {
+    $permissions_to_remove = array_keys($permissions_to_remove);
+    foreach (user_roles() as $rid => $role_entity) {
+      $role_entity->permissions = array_diff($role_entity->permissions, $permissions_to_remove);
+      $role_entity->save();
+    }
+  }

I think we can do something different in user_modules_uninstalled(). We should either move the info in user.module.permission to state OR (and I like preferably)... just get all the permissions by invoking hook_perm and then removing all the ones that don't exist from each role. What's really nice is that the snippet from system_update_8020() becomes completely unnecessary... as this is the only place where we've actually taken care of this information during upgrade.

+++ b/core/modules/user/lib/Drupal/user/RoleStorageController.phpundefined
@@ -34,25 +34,30 @@ public function preSave(EntityInterface $entity) {
+    $prefix = 'user.permission.';
+    if ($ids) {
+      $names = array();
+      foreach ($ids as $id) {
+        $names[] = $prefix . $id;
+      }
+    }
+    else {
+      $names = \Drupal::service('config.storage')->listAll($prefix);
+    }
+    foreach ($names as $name) {
+      config($name)->init();
+    }

This code is unnecessary now that permissions are stored in the role configuration entity.

fubhy’s picture

I think we can do something different in user_modules_uninstalled(). We should either move the info in user.module.permission to state OR (and I like preferably)... just get all the permissions by invoking hook_perm and then removing all the ones that don't exist from each role.

There is a issue with that due to hook_modules_uninstalled() being fired when the affected modules are already disabled which obviously causes their hook_permission() implementation (and therefore also the information about provided permissions) to disappear.

OR (and I like preferably)... just get all the permissions by invoking hook_perm and then removing all the ones that don't exist from each role.

That doesn't work with the current situation with disabled/enabled modules either as that would cause permissions for (potentially only temporarly) disabled modules to be removed as well which is definitely unwanted.

The only viable solution currently (while we still have the stupid 'disabled' state for modules) would be to write them to state() which is what I tried in the patch over at #1872874: Remove the module name property from the role permission table. I think we spoke about that on IRC before. However, I also remember speaking to @chx about this on IRC one or two months ago and we agreed to move forward with this patch with the solution he implemented here as seen above. It's a workaround, yes, but it should not hinder this from getting committed. We can solve it afterwards imho.

chx’s picture

Status:Needs work» Needs review
StatusFileSize
new724 bytes
new56.48 KB
PASSED: [[SimpleTest]]: [MySQL] 55,680 pass(es).
[ View ]

Agreed, right now we do not have a better choice so this revision only changes RoleStorageController

Status:Needs review» Needs work
Issue tags:-Configuration system, -Configurables

The last submitted patch, 1872876_62.patch, failed testing.

chx’s picture

Status:Needs work» Needs review

#62: 1872876_62.patch queued for re-testing.

Status:Needs review» Needs work
Issue tags:+Configuration system, +Configurables

The last submitted patch, 1872876_62.patch, failed testing.

chx’s picture

Status:Needs work» Needs review

Sigh. bot flukes.

alexpott’s picture

StatusFileSize
new59.71 KB
FAILED: [[SimpleTest]]: [MySQL] 56,539 pass(es), 3 fail(s), and 0 exception(s).
[ View ]
new15.11 KB

Came up with a solution that does not use an additionally config file (which is not config) and does not use state :)...

Basically in the role entity permissions are stored as an array keyed by the permission name and with a value of the implementing module...

This patch also fixes a bug on admin/people where in #62 and before you'll get an sql error is you select a permission no one has...

alexpott’s picture

StatusFileSize
new59.58 KB
FAILED: [[SimpleTest]]: [MySQL] 55,939 pass(es), 3 fail(s), and 0 exception(s).
[ View ]
new919 bytes

Removed @todo I introduced... and removed some british optimisation :)

Also some performance testing...

Without patch

Overall Summary
Total Incl. Wall Time (microsec): 333,264 microsecs
Total Incl. CPU (microsecs): 294,523 microsecs
Total Incl. MemUse (bytes): 21,281,176 bytes
Total Incl. PeakMemUse (bytes): 21,507,312 bytes
Number of Function Calls: 43,095

With patch

Overall Summary
Total Incl. Wall Time (microsec): 314,027 microsecs
Total Incl. CPU (microsecs): 277,472 microsecs
Total Incl. MemUse (bytes): 21,354,136 bytes
Total Incl. PeakMemUse (bytes): 21,580,048 bytes
Number of Function Calls: 43,320

Status:Needs review» Needs work

The last submitted patch, 1872876_68.patch, failed testing.

alexpott’s picture

StatusFileSize
new59.55 KB
PASSED: [[SimpleTest]]: [MySQL] 55,624 pass(es).
[ View ]
new1008 bytes

Fixed comments indexing perms...

fubhy’s picture

Hmm... I am not really sure about the benefit of that change. I don't think that it is any better than what we had in patch #62. You are basically just moving the information about the providing module from A to B but it stays in config. And we don't want it there. I think what we really need is a custom, temporary keyvalue store as tried in #1872874: Remove the module name property from the role permission table. But that's really a follow-up. Let's get #62 in and then fix this properly in #1872874: Remove the module name property from the role permission table.

alexpott’s picture

Status:Needs work» Needs review

Okay so here is what's better about #70...

  1. The additional config file user.module.permission should never be deployed
  2. user.module.permission's upgrade path was not complete... afaics the only module's perms that would have been in here after a 7.x to 8.x upgrade would have been ban's if you were using the blocked ip feature otherwise this file would not even exist although you would have roles with permissions....
  3. Leading on from (2) user.module.permission's maintenance is completely untested
  4. In #70 keying by permissions means that we can use isset() rather than in_array() for all of our lookups on the permissions array http://stackoverflow.com/questions/13483219/what-is-faster-in-array-or-i... (apart from when uninstalling modules but who cares about the performance there)...
  5. Since http://buytaert.net/reducing-risk-in-the-drupal-8-release-schedule we really should not be committing patches with an any critical followups.

So in summary: #70 is complete, has upgrade path (because there is no additional storage of info), better tested (less new stuff), and more performant.

fubhy’s picture

Okay, so we are adding lot's of array juggling code now (and invest time in writing that code) just to remove it again once that other key.value store issue is done. Yes it has to be done before release but it's far from critical. It doesn't break anything right now. It's just upgrade-path stuff. And the patch is already there, probably just needs a re-roll and some discussion/fixes. Don't get me wrong, I am not against #70 in any way. I just don't see the point of refactoring this patch with temporary solutions as those benefits you described in #72 are just temporary either until we fix this properly. Let's just move on quickly.

alexpott’s picture

StatusFileSize
new59.55 KB
PASSED: [[SimpleTest]]: [MySQL] 55,780 pass(es).
[ View ]
new1007 bytes

Noticed a bug... obviously we're missing some views test coverage :)

alexpott’s picture

StatusFileSize
new59.43 KB
PASSED: [[SimpleTest]]: [MySQL] 56,097 pass(es).
[ View ]
new755 bytes

More british optimisation replaced with true Drupal optimization!

fubhy’s picture

Status:Needs review» Reviewed & tested by the community

Okay then, let's get this in!

alexpott’s picture

Assigned:Unassigned» catch

Assigning to catch for commit/review

catch’s picture

Status:Reviewed & tested by the community» Needs work
+++ b/core/includes/update.incundefined
@@ -1597,3 +1597,35 @@ function update_add_cache_columns($table) {
+ * @param array $replace
+ *   An associative array. The keys are the old permissions the values are
+ *   an array keyed by new permission with a value of the module that implements
+ *   that permission. If the list is an empty array, the old permission is

Could we maybe add a @code example here?

+++ b/core/modules/comment/comment.moduleundefined
@@ -1211,18 +1211,21 @@ function comment_node_update_index(EntityInterface $node, $langcode) {
+    // comments is not granted by the 'authenticated user' role. In this case
+    // all users might have both permissions from various roles but it is also
+    // possible to set up a user to have only search content and so a user
+    // edit could change the security situation so it is not safe to index the

The last sentence is very long, "and so.." onwards could be a new sentence maybe.

+++ b/core/modules/node/node.installundefined
@@ -479,6 +479,15 @@ function _update_7000_node_get_types() {
/**
+ * Implements hook_update_dependency().
+ */
+function node_update_dependency() {
+  $dependencies['node'][8013] = array(
+    'user' => 8002,
+  );
+}
+
+/**
  * Rename node type language variable names.
  *
  * @see http://drupal.org/node/540294
@@ -704,15 +713,11 @@ function node_update_8012() {

@@ -704,15 +713,11 @@ function node_update_8012() {
  * Renames global revision permissions to use the word 'all'.
  */
function node_update_8013() {
-  $actions = array('view', 'delete', 'revert');
-
-  foreach ($actions as $action) {
-    db_update('role_permission')
-      ->fields(array('permission' => $action . ' all revisions'))
-      ->condition('permission', $action . ' revisions')
-      ->condition('module', 'node')
-      ->execute();
-  }
+  update_replace_permissions(array(
+    'view revisions' => array('view all revisions' => 'node'),
+    'revert revisions' => array('revert all revisions' => 'node'),
+    'delete revisions' => array('delete all revisions' => 'node'),
+  ));
}

I'm not sure about these retrospective updates, mainly due to the horrors of the Drupal 7 upgrade path, but in this case it's a tiny amount of data, we're still pretty far off supporting head to head updates, and saves doing the work twice, so it's probably fine.

+++ b/core/modules/node/node.views_execution.incundefined
@@ -30,17 +30,9 @@ function node_views_analyze(ViewExecutable $view) {
+          $anonymous_has_access = isset(entity_load('user_role', DRUPAL_ANONYMOUS_RID)->permissions['access content']);
+          $authenticated_has_access = isset(entity_load('user_role', DRUPAL_AUTHENTICATED_RID)->permissions['access content']);

Both the existing code and the new could use user_role_permissions() (or whatever that changes to if this patch removes it).

+++ b/core/modules/user/config/user.role.anonymous.ymlundefined
@@ -1,4 +1,6 @@
+permissions:
+  'access content': node

The module key shouldn't be necessary in the yml. This originally got added because of disabled modules which there's a critical issue to remove. module/permission relationships if they have to be kept for now could go in state or somewhere but there's no way this is config much less for individual assignments.

+++ b/core/modules/user/user.moduleundefined
@@ -400,44 +399,53 @@ function user_password($length = 10) {
+/**
+ * Determine the permissions for one or more roles during update.
+ *
+ * A separate version is needed because during update the entity system can't
+ * be used and in non-update situations the entity system is preferred because
+ * of the hook system.
+ *
+ * @param array $roles
+ *   An array whose keys are the role IDs of interest, such as $user->roles.
+ *
+ * @return array
+ *   An array indexed by role ID. Each value is an array whose keys are the
+ *   permission strings for the given role ID.
+ */
+function _user_role_permissions_update($roles) {
+  $role_permissions = array();
+  foreach ($roles as $rid => $name) {
+    $role_permissions[$rid] = array();
+    $permissions = Drupal::config("user.role.$rid")->get('permissions') ?: array();
+    foreach ($permissions as $permission => $module) {
+      $role_permissions[$rid][$permission] = TRUE;
     }

If this is for updates only shouldn't it be in user.install? We shouldn't be using the config API in update situations either but there's already an issue for that and it's not introduced here.

fubhy’s picture

The module key shouldn't be necessary in the yml. This originally got added because of disabled modules which there's a critical issue to remove. module/permission relationships if they have to be kept for now could go in state or somewhere but there's no way this is config much less for individual assignments.

Yes, it's not config. But it was not introduced here either and we still have the disabled module state. There is a follow-up issue to use a key-value store to temporarily store the permissions of disabled modules until they are either re-enabled or uninstalled. #1872874: Remove the module name property from the role permission table

catch’s picture

user_role_grant_permissions() doesn't require a module name so it's a regression to have to specify that.

alexpott’s picture

Status:Needs work» Needs review
StatusFileSize
new62.81 KB
FAILED: [[SimpleTest]]: [MySQL] 56,278 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
new13.67 KB

I've moved the module - permissions info into state :)

Also implemented a has() method on the role to make the in_array nicer.

Status:Needs review» Needs work

The last submitted patch, 1872876_81.patch, failed testing.

fubhy’s picture

I've moved the module - permissions info into state :)

Okay, if I read the interdiff correctly it only writes permissions to state upon installation of new modules. What about dynamically added permissions based on things like node types, etc.? If those get added later they never make it into state().

Also, has() is maybe a bit too unspecific. What about hasPermission()?

fubhy’s picture

I am sorry, but there are even more problems with this... Imho, #62 or #75 are still the way to go until #1199946: Disabled modules are broken beyond repair so the "disable" functionality needs to be removed is fixed. As I realize now, not even the patch from #1872874: Remove the module name property from the role permission table is going to help us as we can't couple key/value and config like this... It's simply going to leave us in an awkward and dirty state upon deployment of the permission config too. Sorry, but we have to keep the module name in the configuration. We really don't want to have config depending on key/value. In any case, as mentioned previously, the situation we have now in D7 is very very dirty as well anyways. Also, I am not aware of any module that properly cleans up it's permission unless uninstalled. E.g. currently, if you create a node type, then assign node-type specific permissions, then remove that node type again, the permission assignments are going to stay in config forever unless the node module gets uninstalled. This whole thing is really sub-optimal atm. and I can't think of a proper solution to the problem atm.

Hence, the best thing we can do atm. is to retain the same functionality we had in D7 so we at least don't introduce any regressions. That means: Keep the module name in the config for now.

fubhy’s picture

Status:Needs work» Needs review
StatusFileSize
new59.43 KB
FAILED: [[SimpleTest]]: [MySQL] 55,827 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

Re-uploading patch from #75 by @alexpott

Status:Needs review» Needs work

The last submitted patch, 1872876_75.patch, failed testing.

heyrocker’s picture

Note that as of #1199946-148: Disabled modules are broken beyond repair so the "disable" functionality needs to be removed Dries has stated he is OK with removing the disabled module state. This work is progressing at #2003966: Helper issue for #1199946 : Disabled modules are broken beyond repair. So it appears we can remove all the disable-specific code from this patch and move on.

fubhy’s picture

Assigned:catch» fubhy

Will do.

fubhy’s picture

Status:Needs work» Needs review
StatusFileSize
new58.21 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1872876-89.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Probably missed a few parts... Anyways, this basically blindly removes any of the module keys from the config and anywhere else. Hence, cleaning up permissions is _NOT_ supported atm as we do not have a pre-uninstalled hook in which we could fire hook_permissions().

fubhy’s picture

Assigned:fubhy» Unassigned

Status:Needs review» Needs work
Issue tags:-Configuration system, -Configurables

The last submitted patch, 1872876-89.patch, failed testing.

fubhy’s picture

Status:Needs work» Needs review

#89: 1872876-89.patch queued for re-testing.

Status:Needs review» Needs work
Issue tags:+Configuration system, +Configurables

The last submitted patch, 1872876-89.patch, failed testing.

fubhy’s picture

@catch / @alexpott Can we get this committed even with the "regression" of not cleaning up stale permissions on uninstall for now? This issue blocks #1966334: Convert user_access to User::hasPermission() (more or less)

fubhy’s picture

Status:Needs work» Needs review
StatusFileSize
new58.39 KB
FAILED: [[SimpleTest]]: [MySQL] 37,086 pass(es), 10,812 fail(s), and 2,956 exception(s).
[ View ]

re-roll

Status:Needs review» Needs work

The last submitted patch, 1872876-96.patch, failed testing.

fubhy’s picture

I'd like to get this in the shape of #96. No cleanup code. We won't need that once the disabled module patch is in. And I am close to finishing it. I can get back to this today and upload a green patch that does not incorporate any cleanup for now. I'd hardly consider that a regression. And it's really something we absolutely need for D8. Pretty please?

chx’s picture

Assigned:Unassigned» catch
Status:Needs work» Needs review

Moving to catch so we can get an opinion on how to continue with this.

fubhy’s picture

StatusFileSize
new66.61 KB
FAILED: [[SimpleTest]]: [MySQL] 57,705 pass(es), 45 fail(s), and 202 exception(s).
[ View ]

Let's see if I missed any permissions.

fubhy’s picture

I opened a follow-up issue to clean up user.module and remove the redundant procedural wrappers.

#2025089: Remove redundant (role-/permission related) procedural wrappers from user.module

fubhy’s picture

Issue summary:View changes

Updated issue summary.

Status:Needs review» Needs work

The last submitted patch, 1872876-101.patch, failed testing.

chx’s picture

Assigned:catch» Unassigned

we discussed this on irc and we will go ahead w/o cleanup for now

fubhy’s picture

Status:Needs work» Needs review
StatusFileSize
new69.79 KB
FAILED: [[SimpleTest]]: [MySQL] 58,212 pass(es), 40 fail(s), and 1 exception(s).
[ View ]

Status:Needs review» Needs work
Issue tags:-Configuration system, -Configurables

The last submitted patch, 1872876-105.patch, failed testing.

fubhy’s picture

Status:Needs work» Needs review
Issue tags:+Configuration system, +Configurables

#105: 1872876-105.patch queued for re-testing.

dawehner’s picture

Issue tags:+VDC
StatusFileSize
new13.69 KB
new75.83 KB
FAILED: [[SimpleTest]]: [MySQL] 57,426 pass(es), 91 fail(s), and 6,570 exception(s).
[ View ]

Fixed the code/tests

Status:Needs review» Needs work
Issue tags:-Configuration system, -VDC, -Configurables

The last submitted patch, user_permissions_config-1872876-106.patch, failed testing.

dawehner’s picture

Status:Needs work» Needs review

Status:Needs review» Needs work
Issue tags:+Configuration system, +Configurables

The last submitted patch, user_permissions_config-1872876-106.patch, failed testing.

fubhy’s picture

Status:Needs work» Needs review
StatusFileSize
new2.19 KB
new75.89 KB
FAILED: [[SimpleTest]]: [MySQL] 57,994 pass(es), 23 fail(s), and 0 exception(s).
[ View ]

Fixing views filter plugin static factory, upgrade_replace_permissions() and ViewsExecutableTest.

Status:Needs review» Needs work
Issue tags:-Configuration system, -Configurables

The last submitted patch, 1872876-111.patch, failed testing.

fubhy’s picture

Status:Needs work» Needs review

#112: 1872876-111.patch queued for re-testing.

Status:Needs review» Needs work
Issue tags:+Configuration system, +Configurables

The last submitted patch, 1872876-111.patch, failed testing.

fubhy’s picture

Status:Needs work» Needs review
StatusFileSize
new69.86 KB
FAILED: [[SimpleTest]]: [MySQL] 58,059 pass(es), 34 fail(s), and 0 exception(s).
[ View ]

Status:Needs review» Needs work

The last submitted patch, 1872876-116.patch, failed testing.

fubhy’s picture

Status:Needs work» Needs review
StatusFileSize
new75.88 KB
FAILED: [[SimpleTest]]: [MySQL] 58,025 pass(es), 22 fail(s), and 0 exception(s).
[ View ]

Status:Needs review» Needs work

The last submitted patch, 1872876-118.patch, failed testing.

fubhy’s picture

Status:Needs work» Needs review
StatusFileSize
new73.71 KB
PASSED: [[SimpleTest]]: [MySQL] 57,989 pass(es).
[ View ]
andypost’s picture

Walked through patch and found no problems. RTBC?

+++ b/core/modules/user/lib/Drupal/user/RoleInterface.php
@@ -14,4 +14,45 @@
+  public function getPermissions();
...
+  public function hasPermission($permission);
...
+  public function grantPermission($permission);
...
+  public function revokePermission($permission);

Great!

alexpott’s picture

+++ b/core/modules/comment/comment.moduleundefined
@@ -1223,20 +1223,24 @@ function comment_node_update_index(EntityInterface $node, $langcode) {
+    $roles = Drupal::entityManager()->getStorageController('user_role')->load();
+    $authenticated_can_access = $roles[DRUPAL_AUTHENTICATED_RID]->hasPermission('access comments');
+    foreach ($roles as $rid => $role) {
+      if ($role->hasPermission('search content')) {
+        if (!$role->hasPermission('access comments') && ($rid == DRUPAL_AUTHENTICATED_RID || $rid == DRUPAL_ANONYMOUS_RID || !$authenticated_can_access)) {
+          $index_comments = FALSE;
+          break;
+        }
       }
     }

This would be even sweeter if it looked like this...

    $roles = Drupal::entityManager()->getStorageController('user_role')->load();
    $authenticated_cannot_access = $roles[DRUPAL_AUTHENTICATED_RID]->cannot('access comments');
    foreach ($roles as $rid => $role) {
      if ($role->can('search content') && $role->cannot('access comments')) {
        if ($rid == DRUPAL_AUTHENTICATED_RID || $rid == DRUPAL_ANONYMOUS_RID || $authenticated_cannot_access) {
          $index_comments = FALSE;
          break;
        }
      }
    }

Even if you don't change hasPermission() to can() and implement cannot() I think we separate the if's as above as it makes it easier to correlate with the big comment above!

fubhy’s picture

StatusFileSize
new73.71 KB
PASSED: [[SimpleTest]]: [MySQL] 58,196 pass(es).
[ View ]

Agreed about the if() statement. I disagree with ->can() and ->cannot() though as I simply feel that hasPermission() is more verbose and also in-line with ->grantPermission() and ->revokePermission().

Also, ->can() could be mistaken has "is this user a container?" :D /jk

EDIT: Sorry, doublepost.

andypost’s picture

Status:Needs review» Reviewed & tested by the community

Now it much more readable.

catch’s picture

+++ b/core/modules/node/node.views_execution.incundefined
@@ -30,17 +30,11 @@ function node_views_analyze(ViewExecutable $view) {
+          $anonmyous_role = entity_load('user_role', DRUPAL_ANONYMOUS_RID);

anonmyous

Can we open a critical, postponed, follow-up for the uninstall cleanup? Otherwise this looks ready.

catch’s picture

Issue summary:View changes

Updated issue summary.

chx’s picture

StatusFileSize
new1006 bytes
new73.7 KB
PASSED: [[SimpleTest]]: [MySQL] 58,163 pass(es).
[ View ]
alexpott’s picture

StatusFileSize
new73.71 KB
PASSED: [[SimpleTest]]: [MySQL] 58,090 pass(es).
[ View ]
new1006 bytes

Added followup to issue summary add rerolled for

+++ b/core/modules/node/node.views_execution.incundefined
@@ -30,17 +30,11 @@ function node_views_analyze(ViewExecutable $view) {
+          $anonmyous_role = entity_load('user_role', DRUPAL_ANONYMOUS_RID);
+          $anonymous_has_access = $anonymous_role && $anonmyous_role->hasPermission('access content');

Actually a bug...

catch’s picture

Status:Reviewed & tested by the community» Fixed

Committed/pushed to 8.x, thanks!

larowlan’s picture

Title:Turn role permission assignments into configuration.» Change Notice: Turn role permission assignments into configuration.
Priority:Normal» Critical
Status:Fixed» Active

Can we get a change notice here.

fubhy’s picture

Status:Active» Needs review

Very simple change record: https://drupal.org/node/2027241

Not sure if that's enough?

scor’s picture

Issue tags:+Needs change record

update tags (normalize to "Needs change notification")

chx’s picture

Status:Needs review» Fixed

Status:Fixed» Closed (fixed)

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

xjm’s picture

Title:Change Notice: Turn role permission assignments into configuration.» Turn role permission assignments into configuration.
Priority:Critical» Normal
Issue tags:-Needs change record

Looks like the change notice was posted.

xjm’s picture

Issue summary:View changes

Update