Problem/Motivation
We need to be able to load roles override free for deprecating user.module functions.
#2025089: Deprecate user_role_grant_permissions(), user_role_revoke_permissions() and user_role_change_permissions()
When loading roles for permissions changes you need the override free role otherwise the translations or other values can bleed when making changes.
Steps to reproduce
N/A
Tests and the fix for this already exist: #3497325: Config entity static cache is not cleared correctly when multiple language overrides are used
This is a helper to make the correct pathway simpler
The issue for deprecating the incorrect pathway is here: #2910353: Prevent saving config entities when configuration overrides are applied
Proposed resolution
Add loadOverrideFree and loadMultipleOverrideFree to ConfigEntityBase.
Remaining tasks
Review.
User interface changes
Introduced terminology
API changes
Data model changes
Release notes snippet
Issue fork drupal-3620216
Show commands
Start within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
- 3620216-add-loadoverridefree-and
changes, plain diff MR !16935
Comments
Comment #3
nicxvan commentedComment #4
nicxvan commentedComment #5
berdirThis issue summary is quite misleading. This bug doesn't exist anymore, it has been fixed with a test. There are no steps to reproduce. This was only a regression in the conversion issue that the test caught, that's what tests are for. This was done as part of #2910353: Prevent saving config entities when configuration overrides are applied, specifically in #3497325: Config entity static cache is not cleared correctly when multiple language overrides are used
The API already exists, this doesn't add anything that is not possible now. This is a pure convenience method. But that convenience is I think quite important, as the user role function issue alone introduces 70 or so ::loadOverrideFree() calls, mostly in tests) and to fix the issue fully, there are hundreds more to update. I count around 1800 ::load() calls in core, many of which config entities that then change and save them. And then contrib and custom code too.
These static methods are a bit controversial but I think having these versions along with the others will make it easier to apply these changes in bulk.
I could rewrite the issue summary myself, but I feel like I've already provided very specific instructions on what this should do that it's easier for me to RTBC if I haven't done as well.
And yes, lets do a CR as this is an API addition.
Comment #6
nicxvan commentedComment #7
nicxvan commentedCR updates look great!
I also addressed all of your feedback in the MR.
Comment #8
berdirI think this is ready.
Disclaimer: I did contribute quite a lot here here except actually write the code. Suggesting to add these methods, input on how to implement and test them. But given that the decision was already made to use Role::load() for #2025089: Deprecate user_role_grant_permissions(), user_role_revoke_permissions() and user_role_change_permissions() and the test we added for this prevented the regression, then the only alternative to the much more verbose version through getStorage() + an assert is this. And practically all other active entity/configuration/configuration entity subsystem maintainers are also core committers, so it doesn't make much of a difference between setting to RTBC and needs subsystem maintainer. The code is trivial, the only decision is whether or not we want to expand on the somewhat controversial static-load-method pattern.
Comment #9
amateescu commentedPosted a few minor comments on the MR. I think this looks good so leaving at RTBC :)
Comment #10
nicxvan commentedI applied two of your changes, and replied to the third. I'm not sure if we can safely restrict to just string.
Comment #13
amateescu commentedCommitted 1b496c9 and pushed to main, and e1f2b85 to 11.x. Thanks!