- Issue created by @berdir
- Merge request !8262Issue #3451741: use static cache on ContentLanguageSettings β (Open) created by berdir
- Status changed to Needs review
8 months ago 11:18am 1 June 2024 - Status changed to Needs work
8 months ago 1:31pm 3 June 2024 - Status changed to Needs review
about 1 month ago 8:10pm 5 January 2025 - π¨πSwitzerland berdir Switzerland
This causes a nasty test fail.
Drupal\Tests\node\Functional\NodeTranslationUITest::testTranslationUI RuntimeException: Adding non-existent permissions to a role is not allowed. The incorrect permissions are "translate article node".
What happens is this:
1. We uninstall content_translation.
2. that collects all configs that depend on content_translation, including thee user role with the translate permission and the language content settings.
3. This then calls into \Drupal\Core\Config\Entity\ConfigEntityBase::onDependencyRemoval() for the language content settings config entity, it removes the content translation third party setting.
4. Then it goes into Role::onDependencyRemoval()
5. That collects all permissions to figure if they depend on the thing that is being removed.
6. That calls \Drupal\content_translation\ContentTranslationPermissions::contentPermissions
7. That calls \Drupal\content_translation\ContentTranslationManagerInterface::isEnabled() to check if the article bundle is enabled for translation
8. NEW: Because this config entity is now statically cached, it returns the same object as the one that got its third party settings removed in step 3. Which means at this point, article is already not translatable anymore
9. It doesn't return the translate article node permission.
10. \Drupal\user\Entity\Role::onDependencyRemoval() ignores undefined permissions.
11. But then it explodes when trying to save in \Drupal\user\Entity\Role::calculateDependencies() because the permission is undefined.The whole thing is very fragile.
I see two options:
a) It seems weird to me that Role explodes in case of an undefined permission in calculateDependencies() but ignores it in onDependencyRemoval(). Maybe we should remove them?
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. - π¬π§United Kingdom catch
This looks very similar to the steps to reproduce in the issue summary in π¬ RuntimeException: Adding non-existent permissions to a role is not allowed Active .
- π¬π§United Kingdom alexpott πͺπΊπ
Created π Translation permissions do not add correct dependencies to user roles and are not cleaned Active for the permissions part of this.
- π¨πSwitzerland berdir Switzerland
@alexpott: Note that this is an edge case that is not covered by your issue. If we enable static caching, then the PermissionsProviders sees the changed entity that already has the third party setting removed and won't define the permission for it then. But it hasn't (yet) gone through save/update hook, so ContentTranslationHooks can't remove the permission before this happens.
I'm also unsure what will happen exactly in the scenario where both roles and language content entities are updated and saved in parallel and then ContentTranslationHooks will *also* load and save the role. Because role config entities already are statically cached, the result might be correct. Still, very complex and scary stuff.
I think to avoid side effects due to changing config entities in memory, we should look into option b that I mentioned:
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.
We'll likely run into more issues as we enable static caching on more config entities.
- π¨πSwitzerland berdir Switzerland
This doesn't require any test changes anymore thanks to #3040878: Reset static entity cache on POST requests in tests β and the permission related problem now just results in a logged warning.
How much this really helps is hard to say, this method clearly shows up in blackfire profiles on pages that load and display a lot of translated entities (this includes menu links of which there can be dozens to hundreds), how much that is blown up to the overhead of blackfire is the tricky questions, but I'm confident that it does help.