Translation permissions do not add correct dependencies to user roles and are not cleaned

alexpott → credited poker10 → .

Creditting @poker10 for the steps to reproduce from #3358586-69: RuntimeException: Adding non-existent permissions to a role is not allowed →

I also commented in 🐛 ContentTranslationManager::isEnabled() can load/create the same config entity many times Active about this.

The dependency stuff is interesting. Do we have any other use case where the dependency is based on a certain *value* of another config entity as opposed to its existence.

As mentioned over there, combined with the hook, it results in two parallel cleanup processes of role permissions.

A couple suggestions on the MR for missing NULL/empty checks, one of which is causing the majority of test failures.

The update hook is also loading role entities without checking that the role entity type exists, but it's only an issue if the user module isn't installed, which seems like a very unusual edge case.

Rebased and updated the MR:

• Applied my suggestions for the null checks
• Removed bundle dependencies when entity permission granularity is at the entity type level
• The line $this->assertEquals(['entity_test.entity_test_mul_bundle.test'], $permissions['translate test entity_test_mul_with_bundle']['dependencies']['config']); when testing bundle granularity with bundle that is not config seems to be testing the wrong thing, so that has been replaced with $this->assertEquals(['language.content_settings.entity_test_mul.entity_test_mul'], $permissions['translate entity_test_mul entity_test_mul']['dependencies']['config']);
• Added test to confirm that when bundle translation is disabled, the permission is removed from the role

2 small comments on the MR. Nothing major

Updated MR to address review.

Thanks that was all the feedback I had. Rest LGTM

I'm not sure if #7 has been addressed. Still feels very weird and problematic that we have two parallel ongoing process that save the role entity and both make their own changes to it, as I wrote there, this only works because they rely on the static cache, their changes are combined and saved twice.

@berdir can you elaborate on what you mean in 🐛 ContentTranslationManager::isEnabled() can load/create the same config entity many times Active comment 7/10?

b) Clone entities that we're about to change in \Drupal\Core\Config\ConfigManager::getConfigEntitiesToChangeOnDependencyRemoval so that we alter things that others might depend on in their logic.

Which entities should be cloned?

The Needs Review Queue Bot → tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.

Fixed the coding standard sadness.

Re #14:

In short, the whole config dependency recalculation stuff goes through things, it loads all config entities, including roles and language content settings (for example when you delete a translatable node type). Then it makes changes to those config entities in memory, for example removing the permissions to create/edit/delete nodes of that node type. Then it goes through them and starts saving them. While saving the language_content_settings config entity ( think that happens before the roles, so they are still in memory), content_translation module now again loads the roles, makes another change to them, saves it. And then we save the role config entity again later on.

This whole thing only works because roles are statically cached, so Role::loadMultiple() already gets the altered versions.

I also think this really should be using override free load, see 🐛 Prevent saving config entities when configuration overrides are applied Needs work and 🐛 UserPermissionsForm should not use overridden permissions Needs work but there's a good chance that the whole thing will then break because they will then no longer share the static cache. At least not until we do the same in the config dependency management (which we should).