Get better error reporting from __toString

This was borne out of work done on #2560715: Prefer toString to __toString in renderVar for ToStringInterface - it might be nice to unit test the TranslationWrapper implementation.

+++ b/core/lib/Drupal/Component/Datetime/DateTimePlus.php
@@ -280,7 +280,13 @@ public function __construct($time = 'now', $timezone = NULL, $settings = array()
+      trigger_error('An exception was thrown while calling __toString on a ' . gettype($this) . ' in ' . $e->getFile() . ' on line ' . $e->getLine() . ': ' . $e->getMessage());

+++ b/core/lib/Drupal/Core/StringTranslation/TranslationWrapper.php
@@ -98,7 +98,13 @@ public function getOptions() {
+      trigger_error('An exception was thrown while calling __toString on a ' . gettype($this) . ' in ' . $e->getFile() . ' on line ' . $e->getLine() . ': ' . $e->getMessage());

Is there a reason to not use get_class() instead of gettype()?

@dawehner no reason, might as well use that

let's add a test

+++ b/core/tests/Drupal/Tests/Core/StringTranslation/TranslationWrapperTest.php
@@ -0,0 +1,72 @@
+ * Contains \Drupal\Tests\Component\Utility\TranslationWrapperTest.
...
+namespace Drupal\Tests\Component\Utility;

hmm that is wrong, should be Drupal\Tests\Core\StringTranslation

1. +++ b/core/lib/Drupal/Component/Datetime/DateTimePlus.php
   @@ -280,7 +280,13 @@ public function __construct($time = 'now', $timezone = NULL, $settings = array()
   +      trigger_error('An exception was thrown while calling __toString on a ' . get_class($this) . ' object in ' . $e->getFile() . ' on line ' . $e->getLine() . ': ' . $e->getMessage(), E_USER_NOTICE);
   +      return '';
   
   +++ b/core/lib/Drupal/Core/StringTranslation/TranslationWrapper.php
   @@ -98,7 +98,13 @@ public function getOptions() {
   +      trigger_error('An exception was thrown while calling __toString on a ' . get_class($this) . ' object in ' . $e->getFile() . ' on line ' . $e->getLine() . ': ' . $e->getMessage(), E_USER_NOTICE);
   +      return '';
   

   Can we make this a fatal and not do a return? Or does that cause problems?

2. +++ b/core/tests/Drupal/Tests/Core/StringTranslation/TranslationWrapperTest.php
   @@ -0,0 +1,72 @@
   +   * @covers ::__toString()
   

   No ()

Don't know... __toString() has to return a string, so maybe E_USER_ERROR + die()?

7.1, I'm confused.
If we do not catch the exception, PHP will emit a E_RECOVERABLE_ERROR, per http://php.net/manual/en/language.oop5.magic.php#language.oop5.magic.tos...
That's already fatal.
Why would we want to catch the exception and emit a new different fatal?

The exception would never reach the error handler... If we make it a trigger_error() it will...

Fatal error: Method Drupal\Core\StringTranslation\TranslationWrapper::__toString() must not throw an exception in /Applications/MAMP/htdocs/drupal/core/lib/Drupal/Core/Entity/EntityType.php on line 0

I thought we could just return an empty string from the __toString() whenever it is unable to create the string itself.

I agree with @lauriii. And we should codify that in the test.

@alexpott what was your reasoning for wanting it to fatal?

It's certainly possible (ie. trigger_error() with a E_USER_ERROR and hardcoding a special case in the Drupal error handler ($fatal = $recoverable || ($error_level == E_USER_ERROR && substr($caller['function'], -strlen('__toString()')) == '__toString()');), with a fallback to die() in the catch blcok in case we are using another error handler, but do we want this?

Just cleaning up the test a bit, prophecy makes it harder to mess up in case the wrong string is passed to translate().

Fuck PHPUnit, see https://github.com/sebastianbergmann/phpunit/issues/487

1. +++ b/core/lib/Drupal/Core/StringTranslation/TranslationWrapper.php
   @@ -98,7 +98,13 @@ public function getOptions() {
   +      return (string) $this->render();
   ...
   +      return '';
   

   I'm curious whether we should return SafeMarkup::format() instead, which doesn't fail but still renders something sort of usable.

2. +++ b/core/tests/Drupal/Tests/Core/StringTranslation/TranslationWrapperTest.php
   @@ -0,0 +1,73 @@
   +    set_error_handler([$this, 'errorHandler']);
   

   Let's document that here ... why we actually need it.

Is it not a fatal error when t() doesn't work when we expect it to... what is the point of soldiering on?

if we fallback to SafeMarkup::format() it would probably just display the untranslated strings if all that's broken is t() and if things are really bad we'd fail further down the line anyway... But I think a fatal may be fine as well, at least it would prevent bad things if someone did if ($input != t('Save the kittens')) { kill_the_kittens() }

I'm not convinced that dieing is a good idea at all. Why is the error not enough?

Without entering into whether it is better to fatal or continue on a failed t() call, if we want it to definitely be fatal we'd have to resort to hacks like this to trigger a fatal error with a custom message, as our error handler doesn't consider E_USER_ERRRORs to be fatal.

Tests in this patch will also fail because the die(). I think we'd have to move that to its own method and mock it, if we even want to keep it. We don't technically need it as I believe we use the Drupal error handler everywhere and we'd have fataled anyway before we ever reach the die().

Imo we should fatal because that exactly what would happen if we called ->render() and it threw an exception.

Thanks @chx for fixing that, if we want to fatal (which we would have done anyway in render()) this looks great now.

We don't enforce UpperCamel for traits do we?

Thank you chx!

Yeah I think this is now the best solution we can get. PHP is a sad world, this is for sure.

+++ b/core/lib/Drupal/Component/Utility/toStringTrait.php
@@ -0,0 +1,42 @@
+trait toStringTrait {

Yeah let's camel case that quickly ... sorry

Thank you!

Nice work all :)

Two nits that can be fixed on commit:

1. +++ b/core/lib/Drupal/Component/Utility/ToStringTrait.php
   @@ -0,0 +1,42 @@
   + * Contains \Drupal\Component\Utility\toStringTrait.
   

   ToStringTrait

2. +++ b/core/lib/Drupal/Component/Utility/ToStringTrait.php
   @@ -0,0 +1,42 @@
   +
   +
   ...
   +trait ToStringTrait {
   +
   +
   

   and a newline too much here

+++ b/core/lib/Drupal/Component/Utility/ToStringTrait.php
@@ -0,0 +1,40 @@
+/**
+ * Wraps __toString in a trait to avoid some fatals.
+ */
+trait ToStringTrait {
...
+  /**
+   * Implements the magic __toString() method.
+   */
+  public function __toString() {
...
+      return (string) $this->render();

We should document that this forces a method called "render" to be implemented by the class that uses this. @stefan.r had a good idea of adding an abstract render() method.

voila

+1

This issue addresses a major bug and is allowed per https://www.drupal.org/core/beta-changes. Committed f52b068 and pushed to 8.0.x. Thanks!

In a light of PHP 8.4 this error reporting need changes #3465827: Stop passing E_USER_ERROR to trigger_error() on PHP 8.4