Comments

Alex Bukach created an issue. See original summary.

Alex Bukach’s picture

Assigned: Alex Bukach » Unassigned
Issue summary: View changes
Status: Needs work » Needs review
FileSize
546 bytes

Here's the patch that overrides the parent __sleep method.

Wim Leers’s picture

Version: 8.3.x-dev » 8.2.x-dev
Component: base system » language system

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.

Alex Bukach’s picture

Wim, thanks for the quick review.

Regarding translatedString, it's reset when the object is rendered. Any argument why we should preserve the original translatedString?

Gábor Hojtsy’s picture

Priority: Minor » Normal
Issue tags: +D8MI, +sprint

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.

Gábor Hojtsy’s picture

Status: Needs review » Reviewed & tested by the community

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

Wim Leers’s picture

Yay :)

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs tests

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’s picture

Assigned: Unassigned » Alex Bukach

I agree, we need also tests.

Alex Bukach’s picture

Actually I failed to figure out how to write the test for this case and whether it's possible. If it is, is IRC an appropriate place to ask for a hint?

maxocub’s picture

Status: Needs work » Needs review
Issue tags: -Needs tests
FileSize
914 bytes
1.43 KB

@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.

alexpott’s picture

Status: Needs review » Needs work
+++ 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.

alexpott’s picture

Status: Needs work » Needs review
FileSize
1.7 KB
2.24 KB

Here's what I mean in patch form.

The test only patch is the interdiff to #2.

The last submitted patch, 13: 2844181-13.test-only.patch, failed testing.

The last submitted patch, 11: 2844181-10-test-only.patch, failed testing.

Gábor Hojtsy’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: +language-ui, +SprintWeekend2017

Looks great :) Woot :)

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

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.

  • alexpott committed 87c1e2e on 8.4.x
    Issue #2844181 by maxocub, Alex Bukach, alexpott:...

  • alexpott committed 1fbf3db on 8.3.x
    Issue #2844181 by maxocub, Alex Bukach, alexpott:...

  • alexpott committed 15d68a7 on 8.2.x
    Issue #2844181 by maxocub, Alex Bukach, alexpott:...
Gábor Hojtsy’s picture

Issue tags: -sprint

Woot!

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

maxocub’s picture