* When a helper module of mine does a $role->revokePermission('foo'),
* and i do a config export,
* i get a diff like this (note the missing '3' index)
diff --git a/config-sync/user.role.toggle_all.yml b/config-sync/user.role.toggle_all.yml
index 0ce46a0..35ac270 100644
--- a/config-sync/user.role.toggle_all.yml
+++ b/config-sync/user.role.toggle_all.yml
@@ -7,11 +7,10 @@ label: 'Toggle all'
weight: -2
is_admin: null
permissions:
- - 'abstractpermissions:role_toggle:toggle_all'
- - 'access administration pages'
- - 'access toolbar'
- - 'role_toggle:administrator'
- - 'role_toggle:debugger'
- - 'role_toggle:design_manager'
- - 'role_toggle:site_manager'
- - 'role_toggle:translation_manager'
+ 0: 'abstractpermissions:role_toggle:toggle_all'
+ 1: 'access administration pages'
+ 2: 'access toolbar'
+ 4: 'role_toggle:debugger'
+ 5: 'role_toggle:design_manager'
+ 6: 'role_toggle:site_manager'
+ 7: 'role_toggle:translation_manager'
When i then re-add it, i get:
diff --git a/config-sync/user.role.toggle_all.yml b/config-sync/user.role.toggle_all.yml
index 0ce46a0..8cb0074 100644
--- a/config-sync/user.role.toggle_all.yml
+++ b/config-sync/user.role.toggle_all.yml
@@ -10,8 +10,8 @@ permissions:
- 'abstractpermissions:role_toggle:toggle_all'
- 'access administration pages'
- 'access toolbar'
- - 'role_toggle:administrator'
- 'role_toggle:debugger'
- 'role_toggle:design_manager'
- 'role_toggle:site_manager'
- 'role_toggle:translation_manager'
+ - 'role_toggle:administrator'
In \Drupal\user\Entity\Role::preSave, add the missing array_values to the normilization.
Ex. 2 above shows that neither index normalization nor sorting work reliably.
\Drupal\user\Entity\Role::preSave:
if (!$this->isSyncing()) {
// Permissions are always ordered alphabetically to avoid conflicts in the
// exported configuration.
sort($this->permissions);
}
Doit
None
None
None
| Comment | File | Size | Author |
|---|---|---|---|
| #20 | 3039499-20.patch | 2.72 KB | alexpott |
| #20 | 3039499-9.5.x-20.patch | 3.79 KB | alexpott |
| #20 | 3039499-20.test-only.patch | 845 bytes | alexpott |
| #19 | 3039499-19.patch | 1.9 KB | alexpott |
| #19 | 3039499-9.5.x-19.patch | 2.96 KB | alexpott |
Comments
Comment #2
geek-merlinComment #4
geek-merlinComment #10
quietone commentedThis would have been fixed in Drupal 9.3.0, #2852557: Config export key order is not predictable, use config schema to order keys for maps
Therefore, closing as duplicate. If this is incorrect reopen the issue, by setting the status to 'Active', and add a comment explaining what still needs to be done.
Thanks!
Comment #11
alexpottWe need to add the sort to the schema - there's also another issue with how permissions are revoked which can cause numeric keys to be added unexpectedly.
Comment #12
alexpottHere's a fix using schema to sort the permissions key. This actually will also fix the problems caused by revoking a role and storing numeric keys because the sort will remove the keys.
Comment #13
alexpottHere's a post update function to ensure existing roles are fixed. We also did this in Drupal 8 - see #2409129: Enforce order of permissions in config export - but we have new abilities with schema to ensure this is the case.
Comment #14
alexpottComment #15
alexpottWe should add a test around the revoke permission case that's detailed in the issue summary.
Comment #18
alexpottA duplicate was created of this - #3341431: user_update_10000 causes exported permissions to be numerically keyed - and this is now critical because it is causing unnecessary config change during an update.
Comment #19
alexpottFixing the update function and providing 9.5.x and 10.x patches...
Comment #20
alexpottHere's a test that proves we've fixed what the issue summary outlines. I'm not sure that an explicit update test is necessary because the logic if only triggering a re-save of the role and we have implicit testing in all the existing update tests (as you can see from the fails in #13).
The test-only patch is the interdiff.
Comment #21
alexpottComment #23
catchAdding credit from the duplicate. Looks RTBC assuming the test only patch fails.
Agreed the update function itself doesn't need explicit test coverage given it's not doing much.
Comment #24
alexpottAdding a note to explain why this fixes #3341431: user_update_10000 causes exported permissions to be numerically keyed. Currently in HEAD sorting occurs on the entity level - in
Role::preSave(). This patch moves it to the config schema level. Therefore permissions are sorted regardless of whether you interact with the Role configuration entity or with the underlying raw configuration.user_update_10000is changing the raw configuration so in HEAD was bypassing the sorting but once this is moved to schema it no longer can.Comment #27
catchTest failure looks good, moving to RTBC.
Comment #31
larowlanCommitted to 10.1.x and backported to 10.0.x
Committed the 9.5.x patch to 9.5.x
Was going to ask about update path tests, but saw catch was ok with it as is, so didn't flag it.