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\UserRoleActionDeriver uses t('Add the @label role to the selected users', ['@label' => $role->label()])
  • Contrib Drupal\ai_automators\Plugin\Action\RunAutomatorActionDeriver in drupal/ai uses t('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

  1. Install Drupal core with language and content_translation; add a non-default interface language with a UI translation of the action label prefix.
  2. Create an action plugin derivative whose label depends on a translatable entity label. For verification, use drupal/ai 1.4.x-dev: an ai_automator with worker_type: action, English label "AI Tag Assignment", and a German config language override "KI Tag-Zuweisung".
  3. Place a Views Bulk Operations bulk-form field referencing the derived action.
  4. Visit /admin/content in English to populate the action manager cache, then switch to German.
  5. 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

Issue fork drupal-3589007

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

petar_basic created an issue. See original summary.

petar_basic’s picture

Assigned: petar_basic » Unassigned
Status: Active » Needs review
StatusFileSize
new3.37 KB

What the MR delivers:

  • Adds @language_manager as an optional 4th constructor argument to ActionManager (promoted, nullable, NULL default for BC + deprecation).
  • Discovery cache keyed by current interface language (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 (mirrors LocalActionManager).

Follow-up: d.o 3589765 applies the same pattern to drupal/views_bulk_operations, which has its own service and cache bin.

smustgrave’s picture

Status: Needs review » Needs work
Issue tags: +Needs test

Will need test coverage

petar_basic’s picture

Status: Needs work » Needs review

Added tests.

fago’s picture

Status: Needs review » Needs work

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

petar_basic’s picture

Status: Needs work » Needs review

Thanks @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.

fago’s picture

Status: Needs review » Reviewed & tested by the community

Good 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?

catch’s picture

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

godotislate’s picture

Status: Reviewed & tested by the community » Needs work

Some comments on the MR, including @catch's note from #9.