Log error when a TranslatableMarkup is created with a non-string input

Patch for 9.5

We had the same problem but the pmkanse fix works in latest version: 9.5.7

Facing this issue in Drupal 10.1.0-beta1

We are facing this issue on Drupal 9.2.20, I didn't saw the version support for it so created a new for it.

Came here from 🐛 WSOD on admin/modules if description is set but is NULL in module.info.yml Fixed , where this was raised as a possible fix, but this seems unrelated.

Here is a test for this.

This problem arises when a config item is marked as translatable or uses a translatable type such as 'label', but contains something other than a string. #28 is a good example of this where schema says its a sting and the config is an array.

1. +++ b/core/lib/Drupal/Core/StringTranslation/TranslatableMarkup.php
   @@ -127,6 +127,7 @@ class TranslatableMarkup extends FormattableMarkup {
      public function __construct($string, array $arguments = [], array $options = [], TranslationInterface $string_translation = NULL) {
   +    $string = !empty($string) ? $string : '';
   

   We should not be changing TranslatableMarkup

2. +++ b/core/modules/locale/src/LocaleConfigManager.php
   @@ -175,7 +175,7 @@ protected function getTranslatableData(TypedDataInterface $element) {
   -      if (!empty($definition['translatable']) && $value !== '' && $value !== NULL) {
   +      if (!empty($definition['translatable']) && (($value !== '' && $value !== NULL) && is_string($value))) {
   

   Do we want to be fault tolerant here and just skip this? Or should we throw something to indicate something it trying to translate something untranslatable? The currently suggested fix would leave users without any indication that anything is wrong. But it doesn't necessarily feel like locales responsibility to inform about faulty config

This implements the previous proposed fix of skipping the faulty config, but somewhat refactored.

The last submitted patch, 56: 2719553-56-TEST_ONLY.patch, failed testing. View results →

The last submitted patch, 57: 2719553-57.patch, failed testing. View results →

+++ b/core/modules/locale/src/LocaleConfigManager.php
@@ -175,7 +175,7 @@ protected function getTranslatableData(TypedDataInterface $element) {
+      if (!empty($definition['translatable']) && $value !== '' && is_string($value)) {

As there are some discussions ( #2275865: Per language settings (vs translated settings) are not directly supported → ) around how to localize configuration (even when they are not strings) and this is subject to change, I would use !is_array instead of asserting for is_string().

Even if we never promised to support that you can translate non-labels, I'm pretty sure people is doing that in custom or even contrib modules. This could break their assumptions, which might be ok-ish but we should avoid if we can.

Fixing the failing test.

@penyaskito thanks for linking that, hadn't seen that issue yet. In this case though, the current way getTranslatableData works will always send the value to TranslatableMarkup and since that does this:

    if (!is_string($string)) {
      $message = $string instanceof TranslatableMarkup ? '$string ("' . $string->getUntranslatedString() . '") must be a string.' : '$string ("' . (string) $string . '") must be a string.';
      throw new \InvalidArgumentException($message);
    }

anything that can't get passed is_string() will cause an exception. So I think keeping the check in getTranslatableData the same as in TranslatableMarkup would be the best way to prevent the exception. Should we change in the future what we allow to be translated, we probably need new logic here anyway.

But again, I'm not 100% convinced that preventing the exception is the right thing to do here. It's just that the current exception makes it very unclear what is going wrong. I'd be totally on board with catching the current exception and re-throwing it with a clearer message that a specified config setting can't be translated or something along those lines.

@penyaskito So yeah, maybe we shouldn't use is_string, but catch the exception? That way the logic of what can be translated stays within TranslatableMarkup and any changes would work in getTranslatableData too....

Sill leaves the question about what we want to do if we catch that exception, silently move along or log something or rethrow it?

@Lendude think something should be logged and move along.

Lot of my sites I routinely check the logs for issues and know that's when I would fix that.

Ok lets log something!

This requires the logger factory to be added, so added a deprecation and change record for that.
Added test coverage for the error.

Not sure about the wording of the error, best I could come up with.

The last submitted patch, 64: 2719553-64.patch, failed testing. View results →

Fix the last test

Logged addition looks good.

I think this is the best we can do for having _some_ error handling while still not breaking what people might be doing as in #60. Thanks!

Might be good having a summary update, or at least a title update. I tried to do that, but suggestions welcome.

The last submitted patch, 66: 2719553-66.patch, failed testing. View results →

Re-rolled for core 10.1.x

Patch #70 doesn't work on Drupal 10.3.2.

Patch #66 works for Drupal 10.3.2.

Issue summary appears incomplete

Fixes should be in MRs

Also the patches #73 ad #74 seem to go drastically down in size without explanation or interdiff.

Will need test coverage

Removing "Needs tests"
merge request is from @lendude patch 66 straight up, which just had a bit of fuzz against the HEAD of 11.x.

Tests
LocaleConfigSubscriberTest fail
and
FATAL Drupal\Tests\locale\Kernel\LocaleConfigSubscriberForeignTest: test runner returned a non-zero error code (2).
0 passes 1 fails

https://git.drupalcode.org/project/drupal/-/merge_requests/9808.patch

Maybe another set of eyes could have a look.