Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.Problem/Motivation
Just as 2 of possibly many examples, hook_image_effect_info_alter() and hook_editor_info_alter() use t() in their example implementations. Additionally, hook_block_alter() isn't documented, but a test module implementation, block_test_block_alter(), calls t().
There are two problems with this:
- These plugin definitions are not cached per language. So what is cached will be the text translated into the interface language of the request that triggered the first discovery after a cleared cache. And then not translated into the interface language of the requests that use those definitions.
- t() marks the output as SafeMarkup safe. But if this only happens at discovery time, then for all requests that retrieve the definitions from cache, those values will not be registered in SafeMarkup. So if one of these alter() implementation includes HTML in the value, it will get escaped when printed, which is not desired.
Proposed resolution
TranslationWrapper was designed to solve this very problem: deferring the call to t() until run-time, not discovery time. So, the resolution is to change all of these examples to use new TranslationWrapper(...) instead of t(). However, there might be contrib implementations out there already calling t() (since our docs were giving that as an example, and nothing noticeably breaks when you do that until you enable multiple languages and populate caches with requests in a different language than what you later view in), so what, if anything, do we want to do about that?
Remaining tasks
- Look for every plugin definition alter hook (e.g., by searching HEAD for
$this->alterInfo(). - Make sure that each alter hook is documented.
- Make sure that the documentation doesn't say to use t(), and that the example code doesn't use t().
- Make sure that core implementations of these hooks use TransaltionWrapper instead of t().
- Figure out how to help contrib avoid the mistake of using t() instead of TranslationWrapper in these places.
Comments
Comment #1
effulgentsia CreditAttribution: effulgentsia at Acquia commentedComment #2
effulgentsia CreditAttribution: effulgentsia at Acquia commentedAs background, #2244447: Translation of low-level info/annotations leads to circular dependencies was the issue that added TranslationWrapper, and #2281905: Stop caching plugin discovery/info hooks by language was the issue that took advantage of that and stopped caching plugin definitions per language. I left comments on both issues, linking to here.
Comment #3
pwolanin CreditAttribution: pwolanin at Acquia commentedShould we make this a "plan" or meta to group those sub-tasks?
Comment #4
effulgentsia CreditAttribution: effulgentsia at Acquia commentedSure. Why not.
Comment #5
Gábor HojtsyA change notice is pretty much what we can do about that IMHO.
Comment #9
andypostit makes sense to have separate api group for plugin definitions alter
Right now they are scattered all over files a-la
core/lib/Drupal/Core/Menu/menu.api.php:115