PluralTranslatableMarkup serialization error

Thanks!

Should also document why translatedString does not also need to be serialized.

I'm tempted to say this needs test coverage, but it seems excessive for something as trivial as this.

Yeah this looks like a total oversight. translatedString is computed I believe at all times, so it should not be / don't need to be serialized.

Re tests, I agree "seems excessive for something as trivial as this.". So let's get this in then :)

Yay :)

Hmmm... I would have thought that we occasionally serialize these in form state and the like. Also I really do think this should be tested. It'd be easy to remove this code without it. Consider the following code:

>>> $a = new \Drupal\Core\StringTranslation\PluralTranslatableMarkup(2, 'singular @count', 'plural @count');
=> Drupal\Core\StringTranslation\PluralTranslatableMarkup {#10570}
>>> (string) $a;
=> "plural 2"
>>> (string) unserialize(serialize($a)));
PHP Parse error: Syntax error, unexpected ')' on line 1
>>> (string) unserialize(serialize($a));
=> "plural "

@Alex Bukach: I think that alexpott's code sample can be used to test this. I'm not sure were we should put this test though.

+++ b/core/modules/locale/src/Tests/LocalePluralFormatTest.php
@@ -37,6 +37,18 @@ protected function setUp() {
   /**
+   * Tests serialization of PluralTranslatableMarkup().
+   */
+  public function testPlurialTranslatableMarkupSerialization() {
+    // Tests with singular and plural values.
+    foreach ([1, 2] as $value) {
+      $markup = new PluralTranslatableMarkup($value, 'singular @count', 'plural @count');
+      $serialized_markup = unserialize(serialize($markup));
+      $this->assertIdentical($markup->render(), $serialized_markup->render());
+    }
+  }

There's no need for this to be a WebTestBase test... also this is a core object not a Locale module object. Should be a new unit test in core/tests/Drupal/Tests/Core/StringTranslation. Also should use a data provider and expected text.

Here's what I mean in patch form.

The test only patch is the interdiff to #2.

Looks great :) Woot :)

Committed and pushed 87c1e2e to 8.4.x and 1fbf3db to 8.3.x and 15d68a7 to 8.2.x. Thanks!

I only added the unit test and not the fix so I think it is okay for me to commit this fix.

Woot!