Translation helpers should return the ID for untranslated messages as stated

[albe]

Currently, the EEL TranslationHelper states in its methods:

@return string Translated label or source label / ID key

However, it will instead return null for messages not translated, because the underlying Translator will return null for non existing messages translated by id:

@return string Translated message or NULL on failure

while it will indeed return the original label, if translated by label:

@return string Translated $originalLabel or $originalLabel itself on failure

This can be considered at least inconsistent and/or false code documentation.

Generally, it would be nice to get the translation ID as output for missing translations (at least in development context?) instead of an empty string (which null will eventually resolve to when rendered), because the ID is easier to see as a missing translation and immediately directs at the correct spot.

These are the options to go about this that I see:

• fix the docblock in TranslationHelper to state it will return null for missing translations, keeping things as they are (a lot of custom translation helpers have been built that work around this shortcoming)

• fix only the TranslationHelper by conditionally returning the ID if the result of the ->translate() call is null (See #1114) - this is breaking in a way that is probably hard to detect, because the output of this EEL helper will just change. OTOH a rendered translation key is probably as obvious as it gets.

• fix the underlying Translator::translateById to be more consistent generally in how the Translation Framework works from the bottom up, in that a missing translation will return the translation input. This is more breaking, because it actually affects an @api method. OTOH, it will make the return type of this method stricter. This would mean that any checks for null to detect missing translations programatically will need to be changed to check for output === id, which is not coverable by a code migration.

[albe]

In https://github.com/neos/neos-development-collection/pull/3102 translation for flashmessages was added with code like this:

            $this->addFlashMessage(
                $this->translator->translateById('users.userCreated.body', [htmlspecialchars($username)], null, null, 'Modules', 'Neos.Neos'),
                $this->translator->translateById('users.userCreated.title', [], null, null, 'Modules', 'Neos.Neos'),
                Message::SEVERITY_OK,
                [],
                1416225561
            );

If the translateById method returns null, the addFlashMessage() call will fail, because it expects a non-null string as first argument. This can happen if no translation for the current language exists and no fallback to the default english is specified (or the default language is changed). See https://neos-project.slack.com/archives/C050C8FEK/p1620908995184700 We should reconsider this.

[kdambekalns]

The fun fact here: The check for a string in addFlashMessage() is about 6 years old.

But yeah, this is inconsistent, and I

• would favour the last solution (fix the underlying Translator::translateById.)
• consider fixing only the TranslationHelper the second-best solution, but given it's "less breaking" it seems the way to go for anything "non-master-branchy"

[albe]

I agree, though "less breaking" is still breaking soooo... 😬 To fix #3345 we can change the caller to use null-coalescing in 7.1.x and then decide if we do want to have that breakiness in 7.2 or 8.0 only (in which case we can just fix the Translator instead).

[mhsdesign]

i think in general (even if this is handled the same in the fluid translation helper) it is odd to return the id back again.

This almost seems like the bad practice of returning false on error or an error object ;)

wouldnt be an exception be more justified, or a system log?

[albe]

wouldnt be an exception be more justified, or a system log?

I would agree if this weren't about the TranslationHelper. When you're building an application and translating it or merely adding new things, the last thing you want is your website no longer working because a translation is missing. It would lead to developers just putting in dummy translations that are maybe wrong, and even harder to detect as being missing. Having it return the original label resp. translate ID is the best DX IMO. We could argue if the inner-most translation service should throw an exception and the outer layers that integrate the translations into the application (TranslationHelper, Translator) catch that exception and return the label/id (while maybe logging the missing translation).