Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Problem/Motivation
- The
FilterFormat
config entity currently does not provide config dependencies to its configured roles. - Related to this, when exporting the config (
admin/config/development/configuration/single/export
) the dumped YAML contains no roles property as the default config provides.
Proposed resolution
The fix of the main issue is coupled with the fix of the 2nd because when calculating dependencies we want to rely on the $format->roles array.
Add them, like many other places in core has them already.
Remaining tasks
Fix the reverse flow: When deleting a format the roles granted with the permission to use that format should drop the stale permissions. This is fixed in #2571235: [regression] Roles should depend on objects that are building the granted permissions.
User interface changes
None.
API changes
None.
Data model changes
The FilterFormat
object will store the roles granted with "use text format {format}" permission.
Comment | File | Size | Author |
---|---|---|---|
#38 | 2569741-38.patch | 20.2 KB | jhedstrom |
Comments
Comment #2
claudiu.cristeaLet's see...
Comment #3
Wim LeersComment #4
claudiu.cristeaPatch.
Comment #5
catchShould it also do onDependencyRemoval()? I can see a site having 5 roles, three having access to the format, then a role gets deleted, the format shouldn't.
Comment #6
catchAlso missing config dependencies are at least major.
Comment #7
dawehnerThat seems to be the wrong way round. Permissions are configured by those roles, not the other way round, see Filterformat::postSave(), so you should not rely on the configuration of the permissions IMHO
Comment #9
claudiu.cristea@dawehner,
Not that simple. In fact what you see in
::postSave()
is a hack to import permissions when the config object is first time loaded from the default config. After that permissions are not stored in the config anymore, but they are stored as any other permission, in the role. Looking more to this I see that this is buggy while when exporting the config (admin/config/development/configuration/single/export
) the dumped YAML contains noroles
property as the default config provides (seecore/modules/filter/config/install/filter.format.plain_text.yml
).I propose to fix this hack (and the bug) in this issue:
roles
property from the default config YAML with normal dependencies definition, including dependencies to 'anonymous' and 'authenticated'.::postSave()
, for$update == FALSE
, check if roles specified in dependencies have proper permissions for this text format. If they miss, add the permissions. In fact this happens only the first time when a text format config default is imported. This is replacing what now does::postSave()
.Implement(see EDIT).onDependencyRemoval()
and return$changed = TRUE
::calculateDependencies()
should add also dependencies to 'anonymous' and 'authenticated'.@catch:
No, we don't need to update anything
onDependencyRemoval()
because the permissions are stored in roles. When a role gets deleted, its permissions to all text formats are deleted too.But, it's true, we need to implement(see EDIT).onDependencyRemoval()
and return$changed = TRUE
in such cases to signal that the format needs to save and recalculate the dependenciesEDIT: @catch, in fact we don't need
onDependencyRemoval()
because, if a role is deleted, on removal, the dependencies are recalculated and there will be no unresolved dependency that triggers the entire text format deletion.Comment #10
claudiu.cristeaThinking more on #9, I like the way now the .yml is written by explicitly defining the
roles
. This is more meaningful for module developers. But what about export? I think that at the time when this piece of code was written the config export was treated as "export the whole site config". And that assured the consistency because on an export also the roles & permissions would have been exported. But this, I think, is no more true. We need to re-add theroles
array on export in::toArray()
.Comment #11
claudiu.cristeaThe export of "roles" property is not handled here. I will open a separate issue for that, if case. In #9 I responded to problems raised in #5 and #7.
We have to clarify if we store the "roles" value inside the text format and if we export that value. If yes, I will open a followup for that.
Comment #12
alexpottNice find.
I agree with previous comments that we need to implement
onDependencyRemoval
At this point you need to reload the $format - because I think it would be deleted in the background.
Comment #13
claudiu.cristea@alexpott,
No, its not I explained in #9 (see EDIT at the bottom) why. Also I'm checking this in test. But anyway, after discussing with @dawehner on IRC, we'll fix the "roles" property here too by storing the roles along with the text format and keep it in sync with role permissions. This would add more consistency to text format and would solve the export bug that I mentioned in #9.
Comment #14
alexpottSee the changes to the test. This will fail.
Comment #17
claudiu.cristeaThis is solving also the bug explained in #9 by storing roles also inside the text format config entity.
But now I found that the reverse is also true: when a text format is deleted, the roles granted with permissions to use that format must update and remove the stale permissions.
EDIT/ADD: In HEAD, the sync of role permissions is done in the UI. I cleaned that, now everything is handled in the API.
Comment #19
claudiu.cristeaFixed the failing test.
Comment #23
claudiu.cristeaComment #24
jibranDo we have to update any filter format config file ?
Comment #25
claudiu.cristeaAs you can see, yes.
Comment #26
jibranOh I meant static yml files in core.
Comment #27
claudiu.cristea#2571235: [regression] Roles should depend on objects that are building the granted permissions must solve the reverse situation: when a text format is deleted, user_role entities involved must update.
Comment #28
claudiu.cristeaAdded support for the reverse flow. The role is directly granted with the permission to use a text format. The text format 'roles' property and the dependency to that role should be updated accordingly.
Updated the issue summary.
Comment #31
claudiu.cristeaFix the kernel test.
Comment #32
catchRight I think missing configuration dependencies is something we should aim to fix during rc. This was previously release blocking since it leads to data integrity issues but it's clear there are some we didn't catch in the previous sweeps.
Comment #33
tim.plunkettI need to dig up the original issues, but this was done very purposefully by @sun.
Comment #34
claudiu.cristea@tim.plunkett, can you review also the reverse issue? When a FilterFormat (but can be any config providing dynamic permissions -- e.g. a node_type) is removed update the roles granted with permissions to interact with that config to clear those permissions: #2571235: [regression] Roles should depend on objects that are building the granted permissions.
Comment #35
catchJust a note I tagged this rc target due to the missing config dependencies.
afaik this leads to config keys getting stale but no actual data loss or runtime errors at all.
Comment #36
jhedstromPatch from above no longer applied, this is just a reroll--I'm unclear on what remains to be done here, since the remaining task is in another issue.
Comment #38
jhedstromThis should fix the fails.
Comment #39
alexpottHmmm... if we do this and #2571235: [regression] Roles should depend on objects that are building the granted permissions won't we end up with circular dependencies. Thinking about this some more... the filter format should not be dependent on a role. A role does not need to exist for the filter format to exist. If the role has a permission "use text format X" that that role is indeed dependent on the filter format - BUT the issue here is that all such dependencies are soft - deleting the filter format should not delete the role.
The export bug mentioned in the issue summary is not real - if you are exporting the filter format and you what the roles to have the permission you need to export the roles too.
Comment #40
xjmComment #47
ducktape CreditAttribution: ducktape at District09 commented@alexpott, we should close this issue:
- there hasn't been an update in 3 years;
- https://www.drupal.org/project/drupal/issues/2571235 is moving towards completion
The roles should be completely removed from the text format edit form in the UI. This is not something that should be configured there, but solely in the Roles & Permissions UI.
Comment #50
tstoecklerBased on the thoughts in #39, I propose a different solution to this problem: removing the roles property altogether, see #3167198: Deprecate the 'roles' property of text formats and remove it from the UI
Comment #53
Wim LeersMarking this as a postponed on #3167198: Deprecate the 'roles' property of text formats and remove it from the UI.
If #3167198: Deprecate the 'roles' property of text formats and remove it from the UI happens as intended, then we'll be able to close this.
Comment #56
larowlanComment #57
Wim LeersActually, based on what @alexpott wrote in #39 and @tstoeckler wrote in #50, we can just close this issue because it's obsolete as soon as #3167198: Deprecate the 'roles' property of text formats and remove it from the UI lands! 😊