Ensure no empty strings are fed to TranslatableMarkup

+++ b/core/lib/Drupal/Core/StringTranslation/TranslatableString.php
@@ -79,6 +79,7 @@ class TranslatableString implements SafeStringInterface {
+    assert($string != '', '$string must not be empty.');
     $this->string = $string;

Ideally we would put it into an eval string, just curious ...

On the other hand its basically for free here.

I am looking at this at DC Barcelona Sprint.
EDIT: ...and will continue to do so for a while, as I didn't get very far at the Sprint but I did understand what I need to do.

Issue renamed in view of the TranslatableString class having been renamed.

Building on what mr.baileys started, here's a patch which I think fixes all the places where empty strings may get fed to a TranslatableMarkup constructor, at least as far as the simpletests in core are concerned. Does there need to be a more thorough change to all the code and a test added whenever TranslatableMarkup objects are created, or was the aim just to make all the tests pass?

By the way, I don't know if this is the best way to fix the getTranslatableData method, but it seems to be a sound way to do it. That method seems to be designed to look for the valid translatable things and only translate them, so this change is just an extension of that behaviour.

1. +++ b/core/lib/Drupal/Core/Plugin/Discovery/YamlDiscovery.php
   @@ -93,7 +93,9 @@ public function getDefinitions() {
   +            if (!empty($definition[$property])) {
   +              $definition[$property] = new TranslatableMarkup($definition[$property], [], $options);
   +            }
   

   Should we define the property as '' instead?

2. +++ b/core/lib/Drupal/Core/StringTranslation/TranslatableMarkup.php
   @@ -75,6 +75,7 @@ class TranslatableMarkup extends FormattableMarkup {
   +    assert($string != '', '$string must not be empty.');
   

   Let's use !== so we don't cause a cast of objects to an empty string. You never know.

Patch rerolled to fix the fatal error caused by the use of the now obsolete TranslationWrapper instead of TranslatableMarkup, as well as to include @dawehner's second suggestion.

@dawehner
1. Sorry, I don't understand your suggestion. Instead of what? Do you mean there should be an else statement?
2. Sure. Somewhere else I saw that we are requiring strings (not just "no empty strings"), but I guess checking for that that is outside the scope of this issue.

1. Sorry, I don't understand your suggestion. Instead of what? Do you mean there should be an else statement?

Yeah there could be an else to ensure we have the property set all the time. Not sure you how can actually end up in that situation but I think its not worth to cause the notice.

@dawehner Looking at the code again, there is already a check that isset($definition[$property]) so it would not make sense to have an else {$definition[$property] = '';} statement connected to the if in the patch.

Conceivably this else could be added to the if (isset($definition[$property])) statement, but IMHO that is beyond the scope of this issue.

The assertion makes me think this is for 8.1.x now, but we could back-port a version without it to 8.0.x since then it would be a straight bug fix.

Also, this does not look major to me, feel free to restore the priority if I'm missing something.

Hmm it might be easier to split the assertion into a new patch, then the bug fix can go into both versions at the same time without a backport.

Hmm it might be easier to split the assertion into a new patch, then the bug fix can go into both versions at the same time without a backport.

I'm a bit confused about this particular statement to be honest. The goal of that issue is to avoid putting empty strings into the TranslatableMarkup.
All the other fixes are just symptoms, the assertion is the actual bugfix.

+++ b/core/lib/Drupal/Core/StringTranslation/TranslatableMarkup.php
@@ -75,6 +75,7 @@ class TranslatableMarkup extends FormattableMarkup {
+    assert($string !== '', '$string must not be empty.');

If this condition is executed regardless of whether assertions are enabled, why don't we just throw an \InvalidArgumentException instead?

@dawehner

I'm a bit confused about this particular statement to be honest. The goal of that issue is to avoid putting empty strings into the TranslatableMarkup.
All the other fixes are just symptoms, the assertion is the actual bugfix.

The assertion if committed by itself would break core, and potentially contrib/custom tests, we don't want to break people's tests in patch releases (unless we absolutely have to for some reason).

The other fixes are a pre-requisite for adding the assertion, but there's nothing about them which could break any other code, so they can go in a patch release easily.

I'm not sure what's confusing about that?

@Xano - if we throw an exception, a module that's not upgraded yet will throw exceptions on production, which will mean fatal errors for end users. We could do that in 9.x, not in a minor release. I'll admit I still have trouble between assertions and trigger_error() though particulary in this case where we're adding it after the fact - should we be using E_USER_DEPRECATED instead here?

@catch: Thanks for the clarification. Assertions are still new to me, but you helped me understand their use case here.

I haven't been rational until jan 6, so yeah I agree, let's fix the issues in core.

Could the issue summary be updated for what's needed here? We looking to throw another exception if empty?

I'm getting a InvalidArgumentException: $string ("") must be a string. in Drupal\Core\StringTranslation\TranslatableMarkup->__construct() (line 132 of /opt/drupal/web/core/lib/Drupal/Core/StringTranslation/TranslatableMarkup.php).

And I was hoping to go around it with patch #13, but it doesn't work on Drupal 10.3.2.

@smustgrave: Well, Cannot access offset can be symptom-ish for empty string - however I wonder if this new issues on Drupal 11 are related:

Cannot access offset of type Drupal\Core\StringTranslation\TranslatableMarkup in isset or empty in Drupal\Core\Plugin\DefaultPluginManager->doGetDefinition() (line 45 of core/lib/Drupal/Component/Plugin/Discovery/DiscoveryTrait.php

When users try to enter paths like admin/structure/types/manage/article/fields/add-field

Raised this in slack with @catch, posting response below:

I still think #20 is what we should do tbh. Use the assertion to find the errors, fix the errors in their own issue, then add the assertion to core itself later.

Back to NW to get this into an MR.