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

Command icon 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:

Comments

nicxvan created an issue. See original summary.

nicxvan’s picture

Issue summary: View changes
nicxvan’s picture

Status: Active » Needs review
berdir’s picture

Component: configuration system » configuration entity system
Status: Needs review » Needs work
Issue tags: +Needs change record
Parent issue: » #2910353: Prevent saving config entities when configuration overrides are applied

This 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.

nicxvan’s picture

Issue summary: View changes
nicxvan’s picture

Status: Needs work » Needs review
Issue tags: -Needs change record

CR updates look great!

I also addressed all of your feedback in the MR.

berdir’s picture

Status: Needs review » Reviewed & tested by the community

I 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.

amateescu’s picture

Posted a few minor comments on the MR. I think this looks good so leaving at RTBC :)

nicxvan’s picture

I applied two of your changes, and replied to the third. I'm not sure if we can safely restrict to just string.

  • amateescu committed e1f2b85d on 11.x
    task: #3620216 Add loadOverrideFree and loadMultipleOverrideFree to...

  • amateescu committed 1b496c98 on main
    task: #3620216 Add loadOverrideFree and loadMultipleOverrideFree to...
amateescu’s picture

Version: main » 11.x-dev
Status: Reviewed & tested by the community » Fixed

Committed 1b496c9 and pushed to main, and e1f2b85 to 11.x. Thanks!

Now that this issue is closed, review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, credit people who helped resolve this issue.

Status: Fixed » Closed (fixed)

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