Problem/Motivation
Drupal\Core\Action\ActionManager extends DefaultPluginManager with the static cache key action_info not keyed by current language. Action plugin derivers commonly compose labels from entity data, e.g.
Drupal\user\Plugin\Action\Derivative\UserRoleActionDeriverusest('Add the @label role to the selected users', ['@label' => $role->label()])- Contrib
Drupal\ai_automators\Plugin\Action\RunAutomatorActionDeriverin drupal/ai usest('AI: @label', ['@label' => $automator->label()])
TranslatableMarkup translates the template lazily but evaluates arguments eagerly. Once the manager caches the derivative definitions on the first request, the @label argument is frozen as the resolved label in that request's language. Editors in other languages see e.g. "KI: AI Tag Assignment" instead of "KI: KI Tag-Zuweisung".
The same anti-pattern affects BlockManager ? see d.o 3038717 (closed-outdated) and d.o 2597608 (user role label staleness).
Steps to reproduce
- Install Drupal core with
languageandcontent_translation; add a non-default interface language with a UI translation of the action label prefix. - Create an action plugin derivative whose label depends on a translatable entity label. For verification, use
drupal/ai1.4.x-dev: anai_automatorwithworker_type: action, English label "AI Tag Assignment", and a German config language override "KI Tag-Zuweisung". - Place a Views Bulk Operations bulk-form field referencing the derived action.
- Visit
/admin/contentin English to populate the action manager cache, then switch to German. - Observed: "KI: AI Tag Assignment". Expected: "KI: KI Tag-Zuweisung".
The bug applies to every ActionManager derivative whose label includes entity-derived translatable text.
Proposed resolution
Apply the canonical pattern already used by LocalActionManager, LocalTaskManager, and ContextualLinkManager: inject LanguageManagerInterface into ActionManager and append the current language ID to the discovery cache key. Each interface language then gets its own cache entry, populated when a request in that language first asks for action definitions. Because the deriver runs in the requesting language, the resolved TranslatableMarkup arguments are correct for that language and stay correct on every subsequent request that hits the same cache entry.
Cache invalidation has to walk every installed language so all per-language entries are dropped together. LocalActionManager::clearCachedDefinitions() is the model: iterate $languageManager->getLanguages(), build the per-language cache IDs, delete them in one batch, and reset the in-memory definitions array.
No deriver code changes are needed ? derivers in core and contrib already build labels in the current language; the bug is purely that the manager only stored one snapshot. Backwards compatibility is preserved with the standard core deprecation pattern: the new LanguageManagerInterface constructor parameter is optional and falls back to \Drupal::languageManager() with a @trigger_error for one minor, so the rare direct instantiations in tests and contrib continue to work.
References for the existing pattern: lib/Drupal/Core/Menu/LocalActionManager.php, LocalTaskManager.php, ContextualLinkManager.php.
Remaining tasks
User interface changes
Introduced terminology
API changes
Data model changes
Release notes snippet
| Comment | File | Size | Author |
|---|---|---|---|
| #3 | 3589007-actionmanager-cache-language-aware-11.x.patch | 3.37 KB | petar_basic |
Issue fork drupal-3589007
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
Comment #3
petar_basic commentedWhat the MR delivers:
@language_manageras an optional 4th constructor argument toActionManager(promoted, nullable, NULL default for BC + deprecation).action_info:<langcode>) when provided, so derived action plugin labels (e.g.UserRoleActionDeriver) resolve per language instead of sticking to whichever language warmed the cache first.clearCachedDefinitions()walks all installed languages (mirrorsLocalActionManager).Follow-up: d.o 3589765 applies the same pattern to drupal/views_bulk_operations, which has its own service and cache bin.
Comment #4
smustgrave commentedWill need test coverage
Comment #5
petar_basic commentedAdded tests.
Comment #6
fagolanguage manager service is always available, so there is no need to make it conditional?
- Without language module: you get Drupal\Core\Language\LanguageManager (single default language).
- With language module: the service is swapped to ConfigurableLanguageManager.
So I don't see why it would be a conditional service. The service is always there. So let's simply use it?
Also, other core classes ContextualLinkManager do it the same way, unconditionally.
Comment #7
petar_basic commentedThanks @fago. Agreed the language_manager service is always available, so functionally it could be injected unconditionally like ContextualLinkManager.
We're keeping it nullable purely for backwards compatibility: ActionManager isn't @internal and is subclassed in contrib — e.g. Views Bulk Operations' ViewsBulkOperationsActionManager calls parent::__construct($namespaces, $cache_backend, $module_handler) with three positional args. Promoting the new argument to required would be a hard BC break (TypeError) for those subclasses until they're updated. The optional argument + @trigger_error follows core's standard pattern for adding a constructor dependency;
The service definition still wires @language_manager unconditionally, so on a real site the language-aware cache key is always used — the NULL path only applies to legacy direct instantiation. Back to Needs review.
Comment #8
fagoGood point, it's protected - so it' would be an API change. The deprecation is the right way. Tests are there as well, so it's ready to go.
Does this need a change record for the new deprecation?
Comment #9
catchConstructor deprecations (unless it's a value object where it's important) are best effort so we don't add change records for them - the deprecation test could also be removed here for the same reason.
Comment #10
godotislateSome comments on the MR, including @catch's note from #9.