Problem/Motivation

This bug can be exposed by applying the patch on #40 of https://drupal.org/node/1798738
After applying the patch, attempt to use the install UI and you'll immediately see:

Fatal error: Method Drupal\Core\Template\RenderWrapper::__toString() must not throw an exception

This is because an exception is being thrown by the patched code, but rather than bubbling up to the default exception handler as you'd expect it finds itself in a __toString function that it can't be thrown from (due to limitations of PHP internals).

We should be reporting the actual error that occurred, rather than an error that occurred while handling that error.

Proposed resolution

Option A - Improved error message

The most obvious solution would be to provide the error in a different format, for example by calling trigger_error with details from the exception.

Option B - Avoid __toString

In this instance, the RenderWrapper is being cast to a string by line 124 of twig.engine, which then delegates to render(). If we were to call render() directly then the exception will be caught by the default handler.

I don't know if there are any instances where we'd prefer to cast to a string than call render, or if there are other cases where we cast to a string. If we were to go down this route we'd probably want to remove the __toString method of RenderWrapper.

CommentFileSizeAuthor
#5 2086219-5a-avoid-tostring-exception-example-do-not-test.patch1.06 KBianthomas_uk
#1 2086219-1a-avoid-tostring-exception.patch737 bytesianthomas_uk
#1 2086219-1b-avoid-tostring-exception.patch452 bytesianthomas_uk
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

ianthomas_uk’s picture

ianthomas_uk
he/him
Primary language English
Location Brighton, UK
CreditAttribution: ianthomas_uk commented
FileSize
2086219-1b-avoid-tostring-exception.patch452 bytes
2086219-1a-avoid-tostring-exception.patch737 bytes

Here are patches with initial implementations of both options

ianthomas_uk’s picture

ianthomas_uk
he/him
Primary language English
Location Brighton, UK
CreditAttribution: ianthomas_uk commented
Status: Active » Needs review

Status: Needs review » Needs work

The last submitted patch, 2086219-1b-avoid-tostring-exception.patch, failed testing.

joelpittet’s picture

joelpittet
Primary language English
Location Vancouver
CreditAttribution: joelpittet commented

Just stumbled in here, I took another approach, just cast the output to a string. Maybe that could also be added?

#1825952-43: Turn on twig autoescape by default

It was needed to get this *working*.

Either way, I'm for this patch:) Nice work @ianthomas_uk

I'm for 1a, the one that passed:)

ianthomas_uk’s picture

ianthomas_uk
he/him
Primary language English
Location Brighton, UK
CreditAttribution: ianthomas_uk commented
Status: Needs work » Needs review
FileSize
2086219-5a-avoid-tostring-exception-example-do-not-test.patch1.06 KB 
+++ b/core/lib/Drupal/Core/Template/Markup.php
@@ -0,0 +1,51 @@
+  /**
+   * Implements the magic __toString() method.
+   */
+  public function __toString() {
+    return (string) $this->render();
+  }

I assume you're referring to this change (from #1825952)? I don't see how that helps, in my testing $this->render() can still throw an exception, which can't get out of __toString(). Also, why would render() not return a string? Wouldn't that be a bug?

I've attached an patch to demonstrate the new behaviour. Even if we did reduce calls to __toString() (option B), unless we remove it totally then we should still do option A. Patch for review/commit remains 1a.

joelpittet’s picture

joelpittet
Primary language English
Location Vancouver
CreditAttribution: joelpittet commented
Status: Needs review » Needs work

@ianthomas_uk I'm not sure where the non-string was coming from during my auto-escape escapades but you're right it is likely a bug.

I'd *SO* rather not even have class at all, it's a stop gap on the way to assetic bliss but to help remove the process layer yet still gather all the assets before aggregating them and compiling to a string. #1762204: Introduce Assetic compatibility layer for core's internal handling of assets

+++ b/core/lib/Drupal/Core/Template/RenderWrapper.php
@@ -52,7 +52,13 @@ public function __construct($callback, array $args = array()) {
+      trigger_error('Unable to re-throw exception. Message: "'.$e->getMessage().'"; Line: '.$trace[0]['line'].'; File: '.$trace[0]['file'], E_USER_ERROR);

The concatenation needs some spaces around it. I don't know if I could RTBC this as I don't know the structure of what the message should be saying and there are no similar trigger_error() calls in core to reference for the message formatting. But those coding standards need to be taken care of. @see https://drupal.org/coding-standards#concat

askibinski’s picture

askibinski CreditAttribution: askibinski commented

Just FYI, i got this error when using mymodules.libraries.yml and declaring an external js like this:

...
drupal.wurfl:
  version: VERSION
  js:
    http://wurfl.io/wurfl.js: { scope: header }

...

Removing "http:" (so starting with //) and clearing the cache removes the error.
Not sure whether this is a seperate issue.

joelpittet’s picture

joelpittet
Primary language English
Location Vancouver
CreditAttribution: joelpittet commented
Status: Needs work » Closed (cannot reproduce)

@askibinski RenderWrapper has been killed! woot and thank god! #2272279: Kill RenderWrapper class

I'm going to close this issue for now, let me know if the latest D8 helps this. If you are getting a different error because of the // protocol, I'd open another issue.

Cheers.