Deprecate AssertHelperTrait::castSafeStrings() in favour of MarkupInterfaceComparator

Just a starter, out of curiosity.

assertIdentical (or, actually, assertSame) will fail when comparing arrays that have on one side strings and on the other MarkupInterface objects, since they are not 'identical'. I wonder whether we should be really be strict in the tests failing in #2, or using assertEquals that will let the new comparator kick in would be sufficient.

#5 OK, let's see what happens if we try to remove castSafeStrings from all things KernelTests.

Trying to find a way to allow MarkupInterfaceComparator work in unit test context.

IMO we would have to un-deprecate simpletest's AssertHelperTrait and copy back the castSafeStrings method to it, to get tests pass. Simpletest can't work with the comparator, so we can't have simpletest's and phpunit's castSafeString both deprecated. Possibly, we also need to have a simpletest's dedicated assertTitle method, since in the trait we need to remove the castSafeString method which will work for phpunit but not for simpletest.

Let's wait until the situation of Simpletest removal is more clear.

+++ b/core/modules/simpletest/src/AssertHelperTrait.php
@@ -2,20 +2,32 @@
-@trigger_error(__NAMESPACE__ . '\AssertHelperTrait is deprecated in Drupal 8.4.x. Will be removed before Drupal 9.0.0. Use Drupal\Tests\AssertHelperTrait instead. See https://www.drupal.org/node/2884454.', E_USER_DEPRECATED);
-
-use Drupal\Tests\AssertHelperTrait as BaseAssertHelperTrait;

Since simpletest hasn't been moved to contrib, I think it's still ok to deprecate the helper trait entirely, and remove all usages in core, then when it does go to contrib, that trait just doesn't need to go along with it...

Rerolled, simpletest related hunks no longer needed.

Fixed deprecation messages, and removed sorting of array before comparing with assertEquals that is not necessary in PHPUnit.

Needs a CR and then referencing it here.

Converted a new test class that was introduced in the meantime.

Added links to CR.

+++ b/core/lib/Drupal/Core/StringTranslation/TranslatableMarkup.php
@@ -187,7 +188,14 @@ public function getArguments() {
-      $this->translatedMarkup = $this->getStringTranslation()->translateString($this);
+      try {
+        $this->translatedMarkup = $this->getStringTranslation()->translateString($this);
+      }
+      catch (ContainerNotInitializedException $e) {
+        // In unit tests, there is no container initialized. In that case, we
+        // just return the untranslated string.
+        $this->translatedMarkup = $this->string;
+      }

This feels like we're overstepping. This could hide errors from trying to translate a string too early - ie. prior to the container being booted. With this change something that would cause an error now won't. And I'm not convinced this is the correct behaviour. I can remember times when errors in too translation has caused us to make necessary and good changes to code.

Maybe we can just fetch the untranslated string in the comparator if casting to string errors out.

@mondrake which test requires the code in #31?

And more pertinently why are things not breaking now?

#33 It was failing in #10 with a few Unit tests. And re. #34 why, I do not know, good question, will check.

Patch here doing #32.

The interdiff.

Rerolled

So #37 is stiil presenting the same failures of #10 - seem to be related to the ToStringTrait that lets the script die if an exception is thrown during __toString.

Sorry.

So #37 is certainly better than #39... and shows the same 2 failures of #10. Maybe the question is then why BatchBuilderTest and TableSelectTest fail but other unit tests that still rely on TranslatableMarkup do not.

Let's see if this is the right thread - passing $this->getStringTranslationStub() to TranslatableMarkup instances like other unit tests do. The reason why this was passing before (I think) is that now the comparator will always try to convert to strings both actual and expected.

+++ b/core/tests/Drupal/Tests/UnitTestCase.php
@@ -40,6 +41,10 @@
+
+    // Allow tests to compare MarkupInterface objects via assertEquals().
+    $this->registerComparator(new MarkupInterfaceComparator());

Ah I see where the fails are coming from. I'm not sure about this because it's introducing an interesting dependency on the container. This is going to affect contrib unit tests massively.

See this maybe?

So #44 does not work, either, because assertSame on objects require expected and actual to be the same object (obviously), but the sense of the test here is actually to check if the translated string is equal - which clashes with the fact that either the service container is not available, or that we cannot inject the stub string translation service on the SUT objects. Out of ideas at the moment.

@alexpott re #43, trying to follow-up on your point in #3082340-37: Replace assertEquals() overrides in PHPUnit Kernel, Functional and FunctionalJavascript tests with a MarkupInterfaceComparator implementation :)

Neat, thanks @longwave! +1 for RTBC, but cannot myself.

RTBC this between us?

Thanks, so it's going to be RTBU :) then.

LGTM, here we are PHPunitizing more the test codebase by removing usage of a trait that was needed in Simpletest to ensure equality comparison between strings when MarkupInterface objects are involved. In PHPUnit we can use comparators instead and one was introduced in D8.9 specifically for the task - thus making the trait redundant.

BTW - net of deprecation code, I believe this can be backported up to D8.9 if required.

1. +++ b/core/tests/Drupal/Tests/AssertHelperTrait.php
   @@ -17,8 +17,14 @@
   +   * @deprecated in drupal:9.1.0 and is removed from drupal:10.0.0. There is no
   +   *   replacement, just use assertEquals in tests.
   

   The title of this issue made me go, "What? No..." until I read the IS also. Let's mention in the deprecation message that the reason there's no replacement is that MarkupInterface objects can now be compared with strings by default in assertions. (Or however we want to word it; suggestions welcome.)

   Note that the current message is also comma-spliced. (The comma should be a semicolon, unless you are Jane Austen.) And the method name should have ().

2. +++ b/core/tests/Drupal/Tests/AssertHelperTrait.php
   @@ -17,8 +17,14 @@
   +    @trigger_error('AssertHelperTrait::castSafeStrings is deprecated in drupal:9.1.0 and is removed from drupal:10.0.0. There is no replacement, just use assertEquals in tests. See https://www.drupal.org/node/3123638', E_USER_DEPRECATED);
   
   +++ b/core/tests/Drupal/Tests/AssertHelperTraitTest.php
   @@ -8,12 +8,14 @@
   +   * @expectDeprecation AssertHelperTrait::castSafeStrings is deprecated in drupal:9.1.0 and is removed from drupal:10.0.0. There is no replacement, just use assertEquals in tests. See https://www.drupal.org/node/3123638
   

   Also, both method names need parens here. (And of course these should be updated to match the docblock.)

3. +++ b/core/tests/Drupal/Tests/Core/Batch/BatchBuilderTest.php
   @@ -90,10 +90,10 @@ public function testSetInitMessage() {
   -      ->setProgressMessage(new TranslatableMarkup('Batch in progress...'))
   +      ->setProgressMessage(new TranslatableMarkup('Batch in progress...', [], [], $this->getStringTranslationStub()))
   
   @@ -101,10 +101,10 @@ public function testSetProgressMessage() {
   -      ->setErrorMessage(new TranslatableMarkup('Oops. An error has occurred :('))
   +      ->setErrorMessage(new TranslatableMarkup('Oops. An error has occurred :(', [], [], $this->getStringTranslationStub()))
   

   wat

   I see #44 - #48 are about this; maybe we can add an inline comment?

Hi @xjm,

I have tried fixing the issues which you pointed except the last one, it was not clear for me that what kind of inline comment i should add. Rest two points are covered. Please take a look :)

Thanks,

Thank you, points from #53 addressed.

Committed 15de078 and pushed to 9.1.x. Thanks!