ContentTranslationManager::isEnabled() can load/create the same config entity many times

Created on 1 June 2024, 8 months ago

Problem/Motivation

Identified this in profiling a multilingual site. isEnabled() loads the config entity (or even creates one on-demand if it doesn't explicitly exist). This has a non-trivial overhead because config entities by default aren't statically cached.

isEnabled() is called a lot, content_translation_language_fallback_candidates_entity_view_alter() calls it, and that can be called dozens to hundreds of times per page, because

Steps to reproduce

Proposed resolution

There are multiple options.

It might already help a lot to just enable static caching on that config entity type, might require some test fixes, we'll see.

We could also use a full cache collector approach that persistently caches this information.

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

πŸ› Bug report
Status

Active

Version

11.0 πŸ”₯

Component
Content translationΒ  β†’

Last updated 6 days ago

No maintainer
Created by

πŸ‡¨πŸ‡­Switzerland berdir Switzerland

Live updates comments and jobs are added and updated live.
Sign in to follow issues

Merge Requests

Comments & Activities

  • Issue created by @berdir
  • πŸ‡¨πŸ‡­Switzerland berdir Switzerland
  • Status changed to Needs review 8 months ago
  • πŸ‡¨πŸ‡­Switzerland berdir Switzerland
  • Pipeline finished with Failed
    8 months ago
    Total: 612s
    #188089
  • Status changed to Needs work 8 months ago
  • πŸ‡ΊπŸ‡ΈUnited States smustgrave

    Seems to have some test failures.

  • Status changed to Needs review about 1 month ago
  • πŸ‡¨πŸ‡­Switzerland berdir Switzerland
  • Pipeline finished with Failed
    about 1 month ago
    Total: 99s
    #386543
  • Pipeline finished with Failed
    about 1 month ago
    Total: 598s
    #386553
  • Pipeline finished with Failed
    about 1 month ago
    #386561
  • Pipeline finished with Failed
    29 days ago
    Total: 659s
    #387854
  • πŸ‡¨πŸ‡­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 πŸ‡ͺπŸ‡ΊπŸŒ
  • πŸ‡¨πŸ‡­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.

  • πŸ‡΅πŸ‡°Pakistan hmdnawaz

    Patch for 10.4

  • πŸ‡΅πŸ‡°Pakistan hmdnawaz
  • Pipeline finished with Failed
    27 days ago
    Total: 3010s
    #391240
  • Pipeline finished with Failed
    25 days ago
    Total: 537s
    #392852
  • πŸ‡¨πŸ‡­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.

Production build 0.71.5 2024