Enforce order of permissions in config export

Test added.

fix

We should all try and use the same sprint tag. According to https://groups.drupal.org/node/447258 it should be SprintWeekend2015.

1. +++ b/core/modules/user/src/Plugin/views/field/Permissions.php
   @@ -80,7 +80,6 @@ public function query() {
   -    $uids = array();
   
   @@ -105,11 +104,10 @@ public function preRender(&$values) {
   -      foreach ($uids as $uid) {
   -        if (isset($this->items[$uid])) {
   -          ksort($this->items[$uid]);
   -        }
   +      foreach ($this->items as &$permission) {
   +        ksort($permission);
          }
   +      $debug = 1;
   
   +++ b/core/modules/user/src/Tests/Views/HandlerFieldPermissionTest.php
   @@ -42,9 +42,9 @@ public function testFieldPermission() {
   +    $expected_permissions[$this->users[3]->id()][] = t('View user information');
        $expected_permissions[$this->users[3]->id()][] = t('Administer permissions');
        $expected_permissions[$this->users[3]->id()][] = t('Administer users');
   -    $expected_permissions[$this->users[3]->id()][] = t('View user information');
   

   Discussed this with @webflo in person:
   The previous code did not work because $uids was never populated. This now makes it work the way it seemingly was supposed to work all along. The question is whether that actually makes sense because the titles of the permissions are displayed but they are now sorted by machine name. But I guess having some sort of deterministic sorting - even if it is not exactly intuitive - is better than complete randomness. So I would say let's leave that in for now. Especially because of the test coverage, this will actually reduce the fragility of the testing system.

   Let's remove the $debug, though ;-)

2. +++ b/core/modules/user/src/Tests/RoleEntityTest.php
   @@ -0,0 +1,48 @@
   +use Drupal\simpletest\KernelTestBase;
   ...
   +class RoleEntityTest extends KernelTestBase {
   

   Since we have to re-roll anyway, could be using \Drupal\KernelTests\KernelTestBase (TNG)

   For extra credit, then replace assertIdentical() -> assertEquals().

Awesome, thanks a lot!

KTBTNG++

1. +++ b/core/modules/user/src/Entity/Role.php
   @@ -131,6 +130,7 @@ public function grantPermission($permission) {
   +      $this->normalize();
   
   @@ -143,6 +143,7 @@ public function revokePermission($permission) {
   +    $this->normalize();
   
   @@ -186,4 +187,14 @@ public function preSave(EntityStorageInterface $storage) {
   +  /**
   +   * Normalizes the stored permissions.
   +   *
   +   * Permissions are always ordered alphabetically to avoid conflicts in the
   +   * exported configuration.
   +   */
   +  protected function normalize() {
   +    sort($this->permissions);
   +  }
   +
    }
   

   I don't understand why we need the ::normalize() wrapper to wrap a single PHP function. And the name is about normalizing while we are doing a sorting. Drop the method and use sort() directly in grantPermission() and revokePermission(). EDIT: revokePermission() doesn't need any sorting. Normally it should keep the same order as before revoking.

2. +++ b/core/modules/user/src/Plugin/views/field/Permissions.php
   @@ -80,7 +80,6 @@ public function query() {
   -    $uids = array();
   
   @@ -105,10 +104,8 @@ public function preRender(&$values) {
   -      foreach ($uids as $uid) {
   -        if (isset($this->items[$uid])) {
   -          ksort($this->items[$uid]);
   -        }
   +      foreach ($this->items as &$permission) {
   +        ksort($permission);
   

   Why not adding the same sort() in Role::getPermission() and this change we can avoid.

3. +++ b/core/modules/user/src/Tests/RoleEntityTest.php
   @@ -0,0 +1,48 @@
   +  /**
   +   * Modules to enable.
   +   *
   +   * @var array
   +   */
   

   {@inheritdoc}

4. +++ b/core/modules/user/src/Tests/RoleEntityTest.php
   @@ -0,0 +1,48 @@
   +  public static $modules = [
   +    'system',
   +    'user'
   +  ];
   

   Inline?

5. +++ b/core/modules/user/src/Tests/RoleEntityTest.php
   @@ -0,0 +1,48 @@
   +    $role = Role::create(array('id' => 'test_role'));
   +    $role->grantPermission('b');
   +    $role->grantPermission('a');
   +    $role->grantPermission('c');
   

   Let's use modern array representation with square brackets. Also this can be written in more compact way, by chaining the method calls:

   $role = Role::create(...)
     ->grantPermission(...)
     ->grantPermission(...)
     ->grantPermission(...);
   

Extra.

+++ b/core/modules/user/src/Plugin/views/field/Permissions.php
index 0000000..39c6b16
--- /dev/null

--- /dev/null
+++ b/core/modules/user/src/Tests/RoleEntityTest.php

+++ b/core/modules/user/src/Tests/RoleEntityTest.php
@@ -0,0 +1,48 @@
+ * Contains \Drupal\user\Tests\RoleEntityTest.
...
+class RoleEntityTest extends KernelTestBase {

All roles tests are named following the pattern: UserRole*Test. Let's use the same convention.

I am working on it.

I don't understand why we need the ::normalize() wrapper to wrap a single PHP function. And the name is about normalizing while we are doing a sorting. Drop the method and use sort() directly in grantPermission() and revokePermission(). EDIT: revokePermission() doesn't need any sorting. Normally it should keep the same order as before revoking.

revokePermission() does need sorting because we would need to sort users in alphabetical order after the user is deleted and also sort removes array key.

Why not adding the same sort() in Role::getPermission() and this change we can avoid.

Since grantPermission() and revokePermission() are already sorted so you can't change the permissions

#20 is not addressed. Why keeping the useless ::normalize()?

Since grantPermission() and revokePermission() are already sorted so you can't change the permissions

Not enough. A piece of code can bypass the grantPermission() sort in this way:

$role->set('permissions') = $permissions;

We have to sort on revokePermission because we want to normalize the array keys after we removed a permissions with array_diff

array_diff() is not touching the order of first array in the arguments list.

1. +++ b/core/modules/user/src/Tests/UserRoleEntityTest.php
   @@ -0,0 +1,44 @@
   +  /**
   +   * Modules to enable.
   +   * @var array
   +   */
   

   See #20.

2. +++ b/core/modules/user/src/Tests/UserRoleEntityTest.php
   @@ -0,0 +1,44 @@
   +    $role = Role::create(['id' => 'test_role']);
   +    $role->grantPermission('b')
   

   See #20.

Not enough. A piece of code can bypass the grantPermission() sort in this way:

IMHO this is a weak point ... we have an API and module authors don't use it. This is a clear problem. When you use an API, you get benefits, when you don't, your life will be bad.

array_diff() is not touching the order of first array in the arguments list.

Well, this is not really the point here. This is about array keys, let me show an example: https://3v4l.org/Yf6Zq
As you see this is not longer a simple list with increasing array index, which results in a completely different YAML output.

@webflo and myself talked about that issue and we came to the conclusion on presave would be ideal, but we should skip the resorting, in case we actually sync configuration.

I love it. I've had merge conflicts because of this issue on every single issue.

Great! RTBC if green.

As far as I can see we should have an upgrade path here to fix existing roles. Nice test coverage btw.

+++ b/core/modules/user/tests/src/Kernel/Views/HandlerFieldPermissionTest.php
@@ -37,9 +37,9 @@ public function testFieldPermission() {
     // View user profiles comes first, because we sort by the permission
     // machine name.
+    $expected_permissions[$this->users[3]->id()][] = t('View user information');

Amusing that comment is now correct :)

+++ b/core/modules/user/user.post_update.php
@@ -0,0 +1,21 @@
+ */
+use Drupal\user\Entity\Role;

Empty line before use

But still needs test for the upgrade path.

Yes :)

1. +++ b/core/modules/user/src/Tests/Update/UserUpdateOrderPermissions.php
   @@ -0,0 +1,41 @@
   +/**
   + * @file
   + * Contains \Drupal\user\Tests\Update\UserUpdateSortPermissions.
   + */
   

   Should be removed.

2. +++ b/core/modules/user/src/Tests/Update/UserUpdateOrderPermissions.php
   @@ -0,0 +1,41 @@
   +      __DIR__ . '/../../../../system/tests/fixtures/update/drupal-8.bare.standard.php.gz',
   

   Use the latest: drupal-8-rc1.bare.standard.php.gz

3. +++ b/core/modules/user/src/Tests/Update/UserUpdateOrderPermissions.php
   @@ -0,0 +1,41 @@
   +  public function testPermissionsOrder() {
   +    $this->runUpdates();
   +    array_map(function (Role $role) {
   +      $permissions = $role->getPermissions();
   +      sort($permissions);
   +      $this->assertIdentical($permissions, $role->getPermissions());
   +    }, Role::loadMultiple());
   +  }
   

   How do we know that permission were unsorted initially? I think we don't need to iterate on all roles. What we need is a single role with unsorted order. Then we assert, before running updates, that the list is not sorted and again after. If the core doesn't have such a case we need to add one via a fixture.

+++ b/core/modules/user/src/Plugin/views/field/Permissions.php
--- /dev/null
+++ b/core/modules/user/src/Tests/Update/UserUpdateOrderPermissions.php

+++ b/core/modules/user/src/Tests/Update/UserUpdateOrderPermissions.php
@@ -0,0 +1,41 @@
+class UserUpdateSortPermissions extends UpdatePathTestBase {

The file and the class names don't match. Also the class name should end with *Test.

Hope that bot agrees. Thank you!

Committed and pushed f5be97c to 8.4.x and cf6312f to 8.3.x. Thanks!

Thanks for adding the update path with tests.

diff --git a/core/modules/user/user.post_update.php b/core/modules/user/user.post_update.php
index 3f19b0c..cdfa758 100644
--- a/core/modules/user/user.post_update.php
+++ b/core/modules/user/user.post_update.php
@@ -8,6 +8,11 @@
 use Drupal\user\Entity\Role;
 
 /**
+ * @addtogroup updates-8.3.x
+ * @{
+ */
+
+/**
  * Enforce order of role permissions.
  */
 function user_post_update_enforce_order_of_permissions() {
@@ -20,3 +25,7 @@ function user_post_update_enforce_order_of_permissions() {
   };
   array_map($entity_save, Role::loadMultiple());
 }
+
+/**
+ * @} End of "addtogroup updates-8.3.x".
+ */

Added some missing comments.